Libnetwork: Refactor to use deterministic network order#689
Libnetwork: Refactor to use deterministic network order#689Luap99 merged 1 commit intocontainers:mainfrom
Conversation
mheon
force-pushed
the
ordered_networks
branch
3 times, most recently
from
4a56159 to
ede40e6
Compare
|
Packit jobs failed. @containers/packit-build please check. |
|
Going to add my commit to build NV from source to see if the tests start passing |
|
Abandoning that plan since c/common doesn't actually have a Cirrus setup script and I don't feel like hacking one out that will just be ripped out once we get a proper NV build out. This can stay draft until we have a fresh NV ready to test with in the VMs. |
|
#705 is merged |
mheon
force-pushed
the
ordered_networks
branch
3 times, most recently
from
a1d6eed to
c0ad2cd
Compare
|
containers/buildah#6722 for Buildah testing Dropping WIP, though I do need to get everything rebased |
mheon
changed the title
|
Buildah and Podman both rebased. Should be good to go if those go green. |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
I know nothing about whether this should happen or the wider context, looking just at the immediate implementation.
| // Networks contains all networks with the PerNetworkOptions. | ||
| // The map should contain at least one element. | ||
| Networks map[string]PerNetworkOptions `json:"networks"` | ||
| Networks []NamedPerNetworkOptions `json:"networks"` |
There was a problem hiding this comment.
What is encoding/decoding this JSON? Do we need to worry about compatibility?
There was a problem hiding this comment.
… this seems to be passed to a netavark process. Do we need to worry about the two being out of sync?
There was a problem hiding this comment.
We will have to add a hard dependency on a new version of Netavark as part of this - we need the latest version (at present) to support the new JSON format.
There was a problem hiding this comment.
This will need Netavark v2 yes, it will be a packing requirement (and yeah people will need to build it locally from main until we release v2). Netavark v2 itself is backwards compatible with the old podman json schema so even with the current nv you can still use a podman without this patch here so it is not that bad, only one way requirement.
Replace map[string]PerNetworkOptions - which does not convey a deterministic order as to when networks are processed - with []NamedPerNetworkOptions, which has a defined iteration order. As we lose the key of the map (network named), we have to stick it in the struct (NamedPerNetworkOptions, which is a PerNetworkOptions with a string added). This should be compatible with the new Netavark JSON format added in containers/netavark#1369 by @Luap99 Signed-off-by: Matthew Heon <matthew.heon@pm.me>
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
ACK, I’ll leave merging timing/decision to someone more familiar with the network stack and the dependencies / packaging constraints.
|
Podman side does not yet pass CI, I defer to @mheon to tell me when the vendor updates are good to go. Until then this should not be merged. |
|
Hopefully tomorrow, just wrapping up some c/common and buildah vendoring issues in Podman. The change itself is tested and working. |
|
Podman and Buildah both green. We are good to review and merge. |
3 participants
Replace map[string]PerNetworkOptions - which does not convey a deterministic order as to when networks are processed - with []NamedPerNetworkOptions, which has a defined iteration order. As we lose the key of the map (network named), we have to stick it in the struct (NamedPerNetworkOptions, which is a
PerNetworkOptions with a string added).
This should be compatible with the new Netavark JSON format added in containers/netavark#1369 by @Luap99
This is NOT fully tested, but since I needed to push to vendor into Podman/Buildah to make that testing happen, I figured I'd open a draft PR.