Gate CNI support behind the `cni` build tag #1767

dcermak · 2023-12-11T09:51:21Z

add cni build tag to libnetwork/cni
split libnetwork/network into multiple files so that cni support can be made optionally available

See also https://issues.redhat.com/browse/RUN-1943

rhatdan · 2023-12-11T10:26:05Z

/approve
LGTM
@Luap99 @mheon PTAL

openshift-ci · 2023-12-11T10:26:16Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Luap99

I think there are two cases to consider:
First of if the cni build tag is set it should keep behaving like it does today. Although I could also see a use case for dropping this awkward cni vs netavark detection logic and only keep reading the defaultNetworkBackend file and if the file does not exists (new users) assume empty == netavark. That would mean an update to 3.X to 5.X (with CNI) could break but I don't think that is a problem given that most 5.X build should not have cni enabled anyway.

Second if the cni build tag is not set then we have the network_backend key from the config, assuming it is set to netavark all is good and we can just use it, if it is set to cni we should return an error. However if it is empty then it reads the defaultNetworkBackend file and if the content is cni it is not clear to me that we should force an error here. This means the user (or distro) never explicitly configured anything and throwing a hard error because the users just kept updating from 3.X seems harsh to me. Assuming there are no CNI networks created and the user only ever used the default network then a update should not require manually intervention. A reboot would be enough to fix the old temporary CNI state to be cleanup up and stuff should work again. And there are also cases were people only run with --network host in which case this choice here shouldn't matter so breaking them will be unjustified (this is very common in the toolbox use case).

Lastly we need to fix the containers.conf man page to document these changes. Unfortunately I don't think we have the infra to properly build man pages based on build tags used. Thus the option is either to document both options or only document netavark only and then require distros that compile with cni support to patch the man pages. I would prefer the first option.

libnetwork/types/const_cni.go

Luap99 · 2023-12-11T10:36:36Z

libnetwork/network/interface.go

-		if val == string(types.CNI) {
-			return types.CNI, nil
-		}


This will break backwards compatibility pretty hard. I haven't made up my mind yet what exactly should happen but this alone is broken because it will break even if CNI build tag is specified you will reject this old cni configuration which should not happen.

That is why I have introduced the alternativeNetworkBackend function:
https://github.com/containers/common/pull/1767/files#diff-d9ace9efdaad94d582a127750587831dff5b06bd443107ba64557bd243bbcd72R57-R99

you will never end up in there because it now returns unknown network backend value "cni" ... driectly below here.

Ah, you're right, my bad!

libnetwork/network/non_cni_interface.go

Makefile

rhatdan · 2023-12-13T11:05:35Z

@Luap99 PTAL

Luap99

Sorry for the late reply, added some comments but I would like to have another look when I am back from the holiday in January.

Makefile

libnetwork/network/non_cni_interface.go

libnetwork/network/interface.go

ashley-cui

Thanks for the PR, @dcermak!

ashley-cui · 2023-12-18T20:03:41Z

libnetwork/network/interface.go


-		aardvarkBin, _ := conf.FindHelperBinary(aardvarkBinary, false)
+func defaultNetworkBackend(store storage.Store, conf *config.Config) (backend types.NetworkBackend, err error) {


I feel like this function would be a good candidate to have different build tagged versions - it would alleviate the need for the CNI supported const. The CNI unsupported version would just return netavark and do the update check/warn, while the supported version would go through the logic

Either this, or even one level above, the NetworkBackend() function, could be the diverging point for the buildtag. I think either of these might work better.

that is a good idea, although I am not sure the additional duplication of code is worth it.
Either way is fine for me so I wouldn't block on this.

libnetwork/network/netavark_interface.go

libnetwork/network/cni_interface.go

rhatdan · 2023-12-20T11:16:41Z

@ashley-cui PTANL

ashley-cui · 2023-12-20T15:50:40Z

Open comment still applies, but I'm open to discussion on it. Would like @Luap99 to take another look after the holidays though.

rhatdan · 2024-01-03T16:14:29Z

@Luap99 waiting on you.

Luap99 · 2024-01-03T17:40:29Z

I try to take a look tomorrow, regardless this needs buildah/podman test PR first to ensure CI is passing there before merging here. I guess this would mostly be disabling all the CNI tests there. Or at least temporarily add the CNI build tag there to ensure it continues working and we can rip it out later (I don't want to force you to deal with all of this work).

ashley-cui · 2024-01-03T19:34:20Z

I can take a look at the podman/buildah portion

Luap99

FYI you have to rebase as you have a conflict due #1780. Should be easy to solve as you only need to drop the old go build tags.

Makefile

libnetwork/types/const.go

Luap99 · 2024-01-05T11:12:01Z

@ashley-cui Can you open the podman and buildah test PRs. I don't think there should be any problems with buildah and we can just build test without the cni tag there but in podman will need some test changes as well. The open question is do we want to continue to test CNI in podman CI?

rhatdan · 2024-01-17T02:09:58Z

@dcermak @Luap99 @ashley-cui Where do we stand on this one?

ashley-cui · 2024-01-17T03:38:56Z

Getting back onto the podman side of things after last week - will get back to this tomorrow.

ashley-cui · 2024-01-22T14:14:54Z

libnetwork/network/interface.go

 		}
+		return "", fmt.Errorf("unknown network backend value %q in %q", val, file)


I think this line is tripping our podman tests.

This test is running netavark with the netavark only build.

To help in debugging: the failure is in f39 rootless and only in two of the e2e tests: add ssh socket path and kube play using a user namespace, and both failures look like this:

→ Enter [It] (test name) Error: Error: failed to get default network backend: unknown network backend value "" in "[share]/containers/storage/defaultNetworkBackend" ... No future change is possible. Bailing out early after 0.741s.

I have no idea what "No future change is possible" means, it seems to be coming from ginkgo itself. All other e2e tests pass (1985 of them). I have no idea what is special about those two tests. I'm just posting direct links in hopes that someone else can understand the failure.

@edsantiago The test run that @ashley-cui linked includes a lot more test failures. Could it be that there is some leftover polluted cache maybe?

The fact that it says unknown network backend value "" seems to suggest there is a conditioning in which it writes (or reads) an empty file which seems very bad.

@dcermak @edsantiago FWIW I changed the test pr to always build CNI, and it looks like all the rootless runs are failing. I think this file should only be created by this network backend code? I can try to help dig a little failure but @Luap99 's diagnosis seems correct.

@ashley-cui could you please rebase the test PRs? I've fixed the issue that @Luap99 found

Luap99 · 2024-01-24T17:09:20Z

libnetwork/network/interface.go

+	// cache the network backend to make sure always the same one will be used
+	defer writeBackendToFile(backend)


I think the issue is that this backend arg is evaluated here right away and not in the defer context as last step so backend ends up being empty all the time.

Given you restructured the function anyway you can just move this call directly before the return and not call it with defer.

What a silly mistake, thank you for spotting it!

I have removed the defer and adjusted the code. Now with the defer gone, we could actually properly treat the error if the defaultNetworkBackend file cannot be written. Although I am honestly unsure whether we should not just continue the current approach and just log it. Worst case: the autodetection will run again next time.

WDYT?

logging is fine

ashley-cui · 2024-01-25T14:04:01Z

Looks like the Podman CI stuff passed - the other failures are unrelated to this PR, but need some fixing. We'll have to wait for those to be fixed to merge this. Thanks for your patience with this PR!

LGTM

Luap99 · 2024-01-25T14:20:10Z

linter is complaining

ashley-cui · 2024-01-29T20:49:45Z

@dcermak Thanks so much for your patience! Could you rebase once more, and I'll kick off the podman tests one more time as we just fixed out podman CI. Then, this should be good to go!

- add cni build tag to libnetwork/cni - split libnetwork/network into multiple files so that cni support can be made optionally available - add -cni build targets to Makefile and build for amd64 with and without cni - add a simple upgrade mechanism if the user never set the network backend explicitly - add cni build tag to .golangci.yml to prevent false positives See also https://issues.redhat.com/browse/RUN-1943 Signed-off-by: Dan Čermák <dcermak@suse.com>

dcermak · 2024-01-30T08:19:04Z

@dcermak Thanks so much for your patience! Could you rebase once more, and I'll kick off the podman tests one more time as we just fixed out podman CI. Then, this should be good to go!

Done

ashley-cui · 2024-01-31T04:18:15Z

containers/podman#21179 is green.

LGTM from my side, and ready to merge.
@containers/podman-maintainers PTAL

rhatdan · 2024-01-31T11:29:59Z

/lgtm

openshift-ci bot added the approved label Dec 11, 2023

Luap99 requested changes Dec 11, 2023

View reviewed changes

dcermak force-pushed the cni-build-tag branch 3 times, most recently from e9dfdf7 to 3c3e841 Compare December 11, 2023 15:28

Luap99 reviewed Dec 15, 2023

View reviewed changes

ashley-cui reviewed Dec 18, 2023

View reviewed changes

dcermak force-pushed the cni-build-tag branch 3 times, most recently from b0a482e to 57b951f Compare December 19, 2023 16:11

Luap99 reviewed Jan 5, 2024

View reviewed changes

Makefile Show resolved Hide resolved

libnetwork/types/const.go Outdated Show resolved Hide resolved

This was referenced Jan 5, 2024

[DO NOT MERGE] Test vendor c/common CNI build tag containers/podman#21179

Closed

[DO NOT MERGE] Test vendor c/common CNI build tag containers/buildah#5250

Closed

openshift-merge-robot added the needs-rebase label Jan 8, 2024

dcermak force-pushed the cni-build-tag branch from 57b951f to 2aa8030 Compare January 8, 2024 10:56

openshift-merge-robot removed the needs-rebase label Jan 8, 2024

dcermak force-pushed the cni-build-tag branch from 2aa8030 to 41dde62 Compare January 8, 2024 10:59

dcermak force-pushed the cni-build-tag branch from 41dde62 to 8ce464a Compare January 18, 2024 12:42

ashley-cui reviewed Jan 22, 2024

View reviewed changes

Luap99 reviewed Jan 24, 2024

View reviewed changes

dcermak force-pushed the cni-build-tag branch from 8ce464a to 33c65f4 Compare January 24, 2024 18:37

dcermak force-pushed the cni-build-tag branch from 33c65f4 to a261c1c Compare January 25, 2024 16:16

dcermak force-pushed the cni-build-tag branch from a261c1c to 609a9be Compare January 30, 2024 08:18

ashley-cui added do-not-merge/hold and removed do-not-merge/hold labels Jan 31, 2024

openshift-ci bot assigned rhatdan Jan 31, 2024

openshift-ci bot added the lgtm label Jan 31, 2024

openshift-merge-bot bot merged commit 2f09d12 into containers:main Jan 31, 2024
7 checks passed

dcermak deleted the cni-build-tag branch February 6, 2024 06:49

lsm5 mentioned this pull request Feb 6, 2024

[CI:BUILD] rpm: remove dnsname containers/podman#20790

Merged


		aardvarkBin, _ := conf.FindHelperBinary(aardvarkBinary, false)
		func defaultNetworkBackend(store storage.Store, conf *config.Config) (backend types.NetworkBackend, err error) {

		}
		return "", fmt.Errorf("unknown network backend value %q in %q", val, file)

		// cache the network backend to make sure always the same one will be used
		defer writeBackendToFile(backend)

Gate CNI support behind the cni build tag #1767

Gate CNI support behind the cni build tag #1767

Conversation

dcermak commented Dec 11, 2023

rhatdan commented Dec 11, 2023

openshift-ci bot commented Dec 11, 2023

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Dec 13, 2023

Luap99 left a comment

Choose a reason for hiding this comment

ashley-cui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Dec 20, 2023

ashley-cui commented Dec 20, 2023

rhatdan commented Jan 3, 2024

Luap99 commented Jan 3, 2024

ashley-cui commented Jan 3, 2024

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 commented Jan 5, 2024

rhatdan commented Jan 17, 2024

ashley-cui commented Jan 17, 2024

ashley-cui Jan 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashley-cui commented Jan 25, 2024 • edited

Luap99 commented Jan 25, 2024

ashley-cui commented Jan 29, 2024

dcermak commented Jan 30, 2024

ashley-cui commented Jan 31, 2024 • edited

rhatdan commented Jan 31, 2024

Gate CNI support behind the `cni` build tag #1767

Gate CNI support behind the `cni` build tag #1767

ashley-cui Jan 22, 2024 •

edited

ashley-cui commented Jan 25, 2024 •

edited

ashley-cui commented Jan 31, 2024 •

edited