podman network create: race: error reading */conflist: ENOENT

[edsantiago]

There seems to be a race condition causing flakes when multiple tests run 'podman network create/rm' at once:

  podman network remove with two networks
...
Running: podman [options] network create net2
Error: in /etc/cni/net.d/testNetTwoCNI2.conflist: error reading /etc/cni/net.d/testNetTwoCNI2.conflist: open /etc/cni/net.d/testNetTwoCNI2.conflist: no such file or directory

Probable cause: podman inspect container two CNI networks test running at just the right time, and running its network rm at just the wrong moment.

Log: from PR 7803

[edsantiago]

(I'm not going to bother trying to reproduce this. I'm gambling that it's trivial to find and fix the race. If it isn't, please LMK and I'll dive into it further).

[mheon]

It should be possible to get the tests to use separate CNI config directories - have a per-test directory and a global one with the main configs. Might be worth investigating to prevent these kinds of issues entirely

…

On Mon, Sep 28, 2020, 15:24 Ed Santiago ***@***.***> wrote: (I'm not going to bother trying to reproduce this. I'm gambling that it's trivial to find and fix the race. If it isn't, please LMK and I'll dive into it further). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7807 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCAOCQG3TN4HVLR5DNDSIDPG7ANCNFSM4R44NPOA> .

[edsantiago]

@mheon by "should be possible" do you mean "we currently have existing functionality, e.g. a --confdir option or envariable, that the tests can use"? Or "it should be possible to add such functionality"?

[mheon]

The functionality exists but isn't wired into the CLI last I checked (config file only). Should not be difficult to add but would require code changes.

…

On Mon, Sep 28, 2020, 15:44 Ed Santiago ***@***.***> wrote: @mheon <https://github.com/mheon> by "should be possible" do you mean "we currently have existing functionality, e.g. a --confdir option or envariable, that the tests can use"? Or "it should be possible to add such functionality"? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7807 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCGLIIS4RQPKT6DN3FDSIDRSLANCNFSM4R44NPOA> .

[Luap99]

Actually we support this already: podman --cni-config-dir ~/test network ls

[rhatdan]

@edsantiago Is that what you want?

[edsantiago]

@rhatdan I haven't looked into it as closely at it deserves. My impression is that it might help with CI flakes but still leave us exposed to real bugs: I do suspect there's a real race condition in which (perhaps):

1. process 1 (podman network create) reads the conflist dir; then
2. something removes one of those files; then
3. process 1 tries to read one of the conflist files it read in step 1, and fails.

One possibility is for podman network create to lock the cni dir (yuk); another might be to ignore ENOENT. But again, this is just pure guesswork on my part.

[Luap99]

podman network create is indeed very racy.

Just try this:

$ podman network create & podman network create
/etc/cni/net.d/cni-podman18.conflist
/etc/cni/net.d/cni-podman18.conflist

As you can see both calls created the same config. This happens about 1 out of 4 tries on my pc, both rootfull and rootless.

Looking at the code there are definitely some race conditions. The root cause is properly this function:

podman/pkg/network/files.go

Lines 29 to 46 in f48b163

	// LoadCNIConfsFromDir loads all the CNI configurations from a dir
	func LoadCNIConfsFromDir(dir string) ([]*libcni.NetworkConfigList, error) {
	files, err := libcni.ConfFiles(dir, []string{".conflist"})
	if err != nil {
	return nil, err
	}
	sort.Strings(files)
	configs := make([]*libcni.NetworkConfigList, 0, len(files))
	for _, confFile := range files {
	conf, err := libcni.ConfListFromFile(confFile)
	if err != nil {
	return nil, errors.Wrapf(err, "in %s", confFile)
	}
	configs = append(configs, conf)
	}
	return configs, nil
	}

For some reason it is called three times when you run podman network create. This alone is very redundant.

I believe this is also the cause of this flake: #7583
If you create two networks at the same time they can get the same interface name assigned which then fails if we try to run it.

Unfortunately I don't think I have enough knowledge and time to properly fix this.

[rhatdan]

Random idea?
Does the 18 matter? Could we just randomize the number, to make the conflict much less likely?

[Luap99]

Well this is also the network interface name. I personally prefer numbered interface names in my ip addr output instead of random suffixes.

[mheon]

We definitely need a global lock for major CNI operations. I'm hesitant to add one for container network setup due to performance concerns, but creating and deleting CNI networks needs to be locked.

[baude]

I'll add this one to my queue ... i have one in front of it and somewhat heavy (compared to other days) meeting day.