Libnetwork: Add IgnoreIfExists flag to network create method

[ygalblum]

Having this flag is very useful when using scripts or systemd unit files

Signed-off-by: Ygal Blum ygal.blum@gmail.com

[ygalblum]

This PR relates to the comment about Quadlet's support for .network files: containers/podman#16688 (comment)

It is the first step in adding this flag to Podman, the same way it was added for Volumes here: containers/podman#16243.

[ygalblum]

@mheon, @Luap99 can you please take a look?

[vrothberg]

LGTM

[Luap99]

This cannot not be added to the network struct. The network struct is used all over the place (including netavark) , this option only applies to network create.

Can we catch the ErrNetworkExists error in podman instead?

[ygalblum]

This cannot not be added to the network struct. The network struct is used all over the place (including netavark) , this option only applies to network create.

Can we catch the ErrNetworkExists error in podman instead?

@Luap99 I'll split my answer into two.
First, the reason I did this here and not in Podman was because I wanted to mimic the behavior of Volume create which also handles this flag at the same level. I don't mind limiting it to Podman.
Second, regarding netavark I can see that it does pretty much the same. So, if this the concern, I make the same change there.

[Luap99]

Yes in theory we can handle it in libnetwork, but this a requires the change to be implement for every network backend which is possible but I don't really consider this a task the network backend should do.
But more importantly this type contains information about a network. This option has nothing to do with a network, it should never be returned in network list/inspect or send to netavark so the only other option is to break the interface and add a new parameter to the NetworkCreate call.

The important part is that the error is checked on the podman server side and not remote client. It does not need to happen inside libnetwork. Podman already blocks certain names here: https://github.com/containers/podman/blob/3f76f29adb070b3481843d33f75d05b8c8aebdfa/pkg/domain/infra/abi/network.go#L142-L145
I think the error can be check there.

I now also start to remember that I tried to do this a long time ago and it was decided against it, containers/podman#8842

Instead we agreed on podman network exits || podman network create

[ygalblum]

Podman already blocks certain names here:

Yes, I saw it too. But, again went with the layer in which it is handled for Volumes. I don't mind closing this PR and make the check there.

it was decided against it

I see that the discussion started from both Network and Volume and for Volumes it was since added. So, why not add it for Network? I think that from the users' persprctive, having Podman support the noop is much simpler and cleaner than making them write complex commands

[Luap99]

I see that the discussion started from both Network and Volume and for Volumes it was since added. So, why not add it for Network? I think that from the users' persprctive, having Podman support the noop is much simpler and cleaner than making them write complex commands

Yes I am totally ok with adding --ignore, I just wanted to link old discussions here.

Yes, I saw it too. But, again went with the layer in which it is handled for Volumes. I don't mind closing this PR and make the check there.

The only way to add it here is to break the interface to add a new parameter (or a new function). We do not guarantee a stable interface here AFAIK so that could work if you prefer to do it here.

[mheon]

C/common isn't 1.0 yet, so we can, but @nalind usually gets annoyed when we break Buildah because things changed. One extra parameter shouldn't be that bad, though, and I don't know if Buildah actually creates networks - I think it just consumes them.

[Luap99]

and I don't know if Buildah actually creates networks - I think it just consumes them.

Correct, buildah would not be effected by this.

[ygalblum]

@Luap99 @mheon I've changed the implementation by adding a new parameter to the API and implemented it for both CNI and Netavark.
The reason I kept it here is that this is where the checks are done inside a lock

[vrothberg]

LGTM

[rhatdan]

@Luap99 PTAL

[Luap99]

one small comment, otherwise LGTM

[Luap99]

Also before merging we need a ready to go podman PR with this change vendored to make sure we do not block others people work since you break the interface.

[vrothberg]

Also before merging we need a ready to go podman PR with this change vendored to make sure we do not block others people work since you break the interface.

Great call, Paul. I keep forgetting to ask for that unfortunately.

@ygalblum, you can open the PR at Podman and vendor this (unmerged) PR here. You can use go mod edit -replace github.com/containers/common=$yourFork@$yourBranch.

[ygalblum]

@vrothberg is this containers/podman#16740 what you mean?

[vrothberg]

@vrothberg is this containers/podman#16740 what you mean?

yes, thank you! Looks like you need to run make vendor and repush in the Podman PR

[rhatdan]

/lgtm
/hold
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, ygalblum

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ygalblum]

@vrothberg, @Luap99 I don't want to mess up Podman, so, please let me know what should the next steps be w.r.t this PR and the one in Podman

[vrothberg]

@vrothberg, @Luap99 I don't want to mess up Podman, so, please let me know what should the next steps be w.r.t this PR and the one in Podman

Since containers/podman#16740 is green, you can drop a /hold cancel here and then vendor in github.com/containers/common@main into the Podman PR.

[ygalblum]

/hold cancel