Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ordering of --network flags is disregarded #12850

Open
dpward opened this issue Jan 13, 2022 · 34 comments
Open

Ordering of --network flags is disregarded #12850

dpward opened this issue Jan 13, 2022 · 34 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. network Networking related issue or feature

Comments

@dpward
Copy link
Contributor

dpward commented Jan 13, 2022

/kind bug

When creating a container, multiple CNI networks can be connected to it, using this syntax (or alternatives):

podman container create --network=cni-podman2 --network=cni-podman3 --network=cni-podman1

However, the ordering is disregarded when parsing the network flags. This happens because it returns the network names as keys in an unordered map. So the networks are always added to the container in alphabetical order, i.e.

  • eth0 will be created for cni-podman1
  • eth1 will be created for cni-podman2
  • eth2 will be created for cni-podman3

The expected behavior is that

  • eth0 will be created for cni-podman2
  • eth1 will be created for cni-podman3
  • eth2 will be created for cni-podman1

The most obvious way to accomplish this is to add a fourth return value from ParseNetworkFlags(), which is a slice containing the network names in the order they are parsed. (However, there may be other solutions.)

cc: @Luap99

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 13, 2022
@Luap99
Copy link
Member

Luap99 commented Jan 13, 2022

Is that actually an issue, there was never a guarantee that the interface name is consistent with the ordering of the network names. That this worked before is more of an implementation detail than a feature.

I don't think we should fix this but if you really need predictable interface names you can use --network net1:interface_name=eth0 and so on.

@dpward
Copy link
Contributor Author

dpward commented Jan 13, 2022

This has never worked, but I think we should fix this. It's possible to re-assign the interface names as you suggested, but it is still reasonable to expect that the interfaces are added to the container in the order given.

Does the solution I described seem like the best approach here? Rather than changing the existing types?

@Luap99
Copy link
Member

Luap99 commented Jan 13, 2022

You can already set the interface name in the types.PerNetworkOptions struct so you do not need to alter the function signature at all. But I still do not think we want this. What is your actual use case that would require this?

@dpward
Copy link
Contributor Author

dpward commented Jan 13, 2022

You can do that - but still if you don't, we would expect the interface ordering on the command line to be preserved. This affects the ifindex as well. (We are running routing software in containers.)

Could you please be a bit more clear about why this is a bad idea, rather than something that simply is not implemented?

Generally speaking, we can infer from other command line options to podman that ordering matters. Notice the handling of --env-file and --hooks-dir.

@Luap99
Copy link
Member

Luap99 commented Jan 13, 2022

It's not a bad idea I just don't think that guaranteeing this is worth the extra maintenance in the long run. The naming thing can be fixed easily and we could add a test to prevent regressions but this will not fix the ifindex. In the backend we use maps so if we loop over the map the ordering is not deterministic.

@dpward
Copy link
Contributor Author

dpward commented Jan 13, 2022

It would be very useful to me. I've examined the backend implementation. All we would need to do is to have a slice containing the interface names in the correct order, and iterate over that to obtain the values from the map in the order we want.

Does that sound reasonable if I prepare a PR?

@Luap99
Copy link
Member

Luap99 commented Jan 13, 2022

Are you talking about only the naming or also the ifindex?

If you need the stable ifindex, what would be the correct order, interface names sorted ascending?
I am not sure were you looked to implement this but if you do so you have to do it for both cni and netavark.
What happens with podman network connect/disconnect?
I still do not think the added complexity is worth the extra maintenance.

@Luap99
Copy link
Member

Luap99 commented Jan 13, 2022

@mheon WDYT?

@Luap99 Luap99 added the network Networking related issue or feature label Jan 13, 2022
@dpward
Copy link
Contributor Author

dpward commented Jan 13, 2022

I'm talking about just adding the interfaces in the order they appear on the command line:

  • If we don't specify the interface names, then the first network automatically becomes eth0, the second becomes eth1, etc.
  • As a side effect of this, the interface for the first network will have a lower ifindex than the interface for the second network, etc.

@mheon
Copy link
Member

mheon commented Jan 13, 2022

Would the ability to manually set interface names (e.g. --net cni-podman1:ifname=eth0 --net cni-podman2:ifname=eth1 ...) be acceptable? I'm a little iffy on making ordering in the CLI a critical part of the interface, but I definitely see how consistent interface naming is useful

@dpward
Copy link
Contributor Author

dpward commented Jan 13, 2022

This wouldn't achieve the same effect (see previous comment).

Here's one example. The ip link command displays interfaces in ifindex order. If we add interfaces to the container in the order they appear on the command line, then this command would list the interfaces that order. For containers with dozens of interfaces, this makes a difference. There is no way to achieve this using --net cni-podman1:ifname=eth0.

(The exception here is host-device networks, which will preserve the existing ifindex from the host.)

@dpward
Copy link
Contributor Author

dpward commented Jan 13, 2022

As I mentioned earlier, we already make command line ordering critical for other things: --env-file, --hooks-dir, etc. So this wouldn't be new territory.

@Luap99
Copy link
Member

Luap99 commented Jan 14, 2022

I am not sure how you want to fix this but it is not possible to keep the cli order in the backend since it is stored as a map. You could sort by the interface name but this would still not match the cli ordering if I would use --network net1:interface_name=eth1 --network net2:interface_name=eth0.

Just the naming change is simple if that is enough for you.

@mheon
Copy link
Member

mheon commented Jan 14, 2022

I suppose we could automatically add interface_name in order when we first parse into the maps (if it's not explicitly set)

@Luap99
Copy link
Member

Luap99 commented Jan 14, 2022

@mheon Yes that is easy but he also wants the ifindex to be in the same order and this depends on the order in which the interfaces are created

@mheon
Copy link
Member

mheon commented Jan 14, 2022

Ahh. Yeah, that would be more difficult.

@dpward
Copy link
Contributor Author

dpward commented Jan 14, 2022

In the libpod API, we already have the cni_networks attribute, which is an ordered list of network names to be used in the container. If we specify that, then we can add the networks in that order.

The documentation says that the new Networks attribute is supposed to be used in favor of that, although cni_networks remains for compatibility. I'm actually a bit surprised we wound up with both Networks and network_options here as the result: those are both maps, where the key is the network name, and the value contains options specific to that network. It seems like it would have made more sense to use network_options to carry the attributes found in Networks; or to provide a new type that is designed to replace network_options and not cni_networks. (I realize I haven't been following the development discussions here)

@Luap99
Copy link
Member

Luap99 commented Jan 14, 2022

The API level doesn't matter here, we are talking about the backend. It is stored as map in the db so there is no way to keep the order. We would need to change the types and db to make this work which is not worth the effort also podman 4.0 is about to be released very soon so there is no time to change this.

cni_networks is for backwards compatibility only
network_options is only used for slirp4netns at the moment, I could have used that for the networks and there is no particular reason why I didn't (maybe that would have made more sense in hindsight I don't know)

Note that just looking at ip link output is not an argument for me. We can make the interface naming change but the ifindex things is not a simple as you think.

@dpward
Copy link
Contributor Author

dpward commented Jan 14, 2022

I realize it's a bit more involved than at first look, but I'd still like to figure out what a workable solution would be here looking ahead to future releases, so that we can add interfaces in command line order.

To provide another example, let's say I have two host-device networks. If attaching the first one fails, then podman won't attempt to attach the second. When these networks are returned to the host, they don't have the same configuration they had originally. Having determinism in the order that interfaces are added to the container would again be useful here. Just renaming the interfaces after the fact overlooks these kinds of details that may matter in certain cases.

@daiaji
Copy link

daiaji commented Jan 22, 2022

This should be helpful for using soft routing in containers.

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Feb 22, 2022

@Luap99 Is this still an issue?

@Luap99
Copy link
Member

Luap99 commented Feb 22, 2022

Yes

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Mar 25, 2022

@Luap99 What do you think about doing something like the following patch and retaining the order that the user added the objects.

order.patch.txt

@Luap99
Copy link
Member

Luap99 commented Mar 25, 2022

I am not a huge fan of adding a new field for this, this will everything more complicated, this can be set by api users so we would need to check for invalid user input.
IMO if we want to make it deterministic we should sort by the interface name, then we can apply the names in the frontent in the order which --network was given. But adding the names in the frontent would also conflict with #13174

@rhatdan
Copy link
Member

rhatdan commented Mar 25, 2022

Let discuss this on Monday with @mheon at standup.

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Apr 25, 2022

@Luap99 @mheon We never followed up on this. What do you want to do with this one?

@mheon
Copy link
Member

mheon commented Apr 25, 2022

I'm OK with fixing but IMO this is low priority. Exact way to implement still requires some discussion.

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Jul 28, 2023

Since this was low priority and no one ever acted on it, I am going to close. Reopen if you want to work on it.

@rhatdan rhatdan closed this as completed Jul 28, 2023
@dpward
Copy link
Contributor Author

dpward commented Jul 28, 2023

@rhatdan Please keep this open. I still very much have a need for this.
My organization has active support agreements from Red Hat, if we need to coordinate this through our TAM.

@Luap99
Copy link
Member

Luap99 commented Jul 31, 2023

I agree that is a valid feature request, it is just that work for it seems rather high.
If you have support agreements then please use them.

@Luap99 Luap99 reopened this Jul 31, 2023
@Luap99 Luap99 added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/bug Categorizes issue or PR as related to a bug. labels Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. network Networking related issue or feature
Projects
None yet
Development

No branches or pull requests

5 participants