Move to deterministic network setup order#28275
Conversation
github-actions
Bot
added
the
kind/api-change
mheon
added
the
bloat_approved
|
Cockpit tests failed for commit dc79633. @martinpitt, @jelly, @mvollmer please check. |
|
Cockpit tests failed for commit e8cecc1. @martinpitt, @jelly, @mvollmer please check. |
|
Cockpit tests failed for commit 7374fa3. @martinpitt, @jelly, @mvollmer please check. |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
1 similar comment
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
Cockpit tests failed for commit 7aa0d2f. @martinpitt, @jelly, @mvollmer please check. |
|
Cockpit tests failed for commit a267d1d. @martinpitt, @jelly, @mvollmer please check. |
|
Cockpit tests failed for commit 704a026. @martinpitt, @jelly, @mvollmer please check. |
|
|
||
| # Absurd sed oneliner provided by Google Gemini | ||
| local ctrname=ctr-$(safename) | ||
| run_podman run --name $ctrname --net "$netname1:interface_name=first" --net "$netname2:interface_name=second" $IMAGE sh -c "ip link | sed -nE 's/^([0-9]+): ([^:]+):.*/\1 \2/p' | sed 's/@.*//'" |
There was a problem hiding this comment.
something like
run_podman run ... quay.io/libpod/testimage:20241011 ip -j link
run -0 jq -r '.[] | (.ifindex|tostring) + " " + .ifname' <<<"$output"
assert ...
might be cleaner and actually understandable what is being parsed compared to the regex
There was a problem hiding this comment.
If you don't mind I'd prefer to leave as-is and avoid the stacked run calls - maybe I could add a comment indicating what the Sed bit does?
There was a problem hiding this comment.
I don't care to much, I just fine reading jq language much easier than regex
| } else { | ||
| delete(newCfg.Networks, network) | ||
| // Modifying an existing network, modify existing array in place | ||
| index := slices.IndexFunc(newCfg.Networks, finderFunc) |
There was a problem hiding this comment.
my point was more than you do the IndexFunc once where you call ContainsFunc(), and then instead of ok you match index >= 0 so we do not need to iterate the slice multiple times.
At the vey least this makes it obvious that IndexFunc() must return a valid match here, because a naive reader could ask what happens if there is no match and we then index -1 int he slice and panic.
but sure not to important right now, though lint complains and wants a switch here
There was a problem hiding this comment.
Yeah, I can see that being a better solution, I'll do it
mheon
changed the title
|
c/common and Buildah merged, ready for review + merge |
|
@containers/podman-maintainers PTAL |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
1 similar comment
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
| Networks map[string]types.PerNetworkOptions `json:"newNetworks,omitempty"` | ||
| Networks []types.NamedPerNetworkOptions `json:"orderedNetworks,omitempty"` | ||
| // LegacyNetworks is deprecated and should not be used. | ||
| // It is identify to Networks aside from being unordered. |
There was a problem hiding this comment.
Should be here, depreciation notice? https://go.dev/wiki/Deprecated
There was a problem hiding this comment.
I mean this is internal code which we do not support externally, if we call it deprecated we must //nolint the callers who still need so I doubt it matters.
There was a problem hiding this comment.
Sure, my idea was to avoid accidental usage in the future. Even though it's just internal.
| opts.Networks = map[string]types.PerNetworkOptions{ | ||
| netName: networks[netName], | ||
|
|
||
| for _, net := range networks { |
There was a problem hiding this comment.
I think this is the second search of the same network.
| // this value is only used for container create. | ||
| // Added in podman 4.0, previously NetworksDeprecated was used. Make | ||
| // Added in podman 6.0, previously LegacyNetworks was used. Make | ||
| // sure to not change the json tags. |
There was a problem hiding this comment.
This can be removed or moved to LegacyNetworks.
|
This was implemented by containers/netavark podman-container-tools#1369; this commit completes the process by wiring it into Podman. We now respect the CLI order for configured networks - if a user passes `--net net1,net2` we guarantee that net1 will be configured before net2. For containers created before this patch, we don't retain enough information to configure networks in CLI order, so we use alphabetical order instead to still guarantee consistency. No breaking API changes have been made, but we do add a new field to supplement the existing map to (optionally) provide ordering information. The Podman CLI will always pass this. Existing applications that do not will, again, receive] deterministic ordering based on an alphabetical sort of network names. This requires the latest version of Netavark to work properly. Signed-off-by: Matthew Heon <matthew.heon@pm.me>
These are two new Buildah flags that we need to wire into Podman (both local and remote) and document, with the interesting note that one requires the other and a check needed to be added for that. Also: secret parsing was tightened up in Buildah, and was breaking the remote build tests. Rewire it to use the new parser Buildah made, which ends up simplifying the code considerably. Tests are back to passing afterwards. Signed-off-by: Matthew Heon <matthew.heon@pm.me>
|
Should I take care of that fix, or is @mheon doing it? I just don't want to forget about it. Let me know. |
|
I would assume @mheon should do the work but I let you sync on that, I guess he won't mind if you do it. |
Cleanup from deterministic network order PR #28275
4 participants
This was implemented by containers/netavark #1369; this commit completes the process by wiring it into Podman. We now respect the CLI order for configured networks - if a user passes
--net net1,net2we guarantee that net1 will be configured before net2.For containers created before this patch, we don't retain enough information to configure networks in CLI order, so we use alphabetical order instead to still guarantee consistency.
No breaking API changes have been made, but we do add a new field to supplement the existing map to (optionally) provide ordering information. The Podman CLI will always pass this. Existing applications that do not will, again, receive] deterministic ordering based on an alphabetical sort of network names.
Draft as this required Buildah and c/common changes be merged first. Also requires a new Netavark in our CI VMs; I have a hack in there to build from source right now.
Does this PR introduce a user-facing change?