network db rewrite #12534
Merged
network db rewrite #12534
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
This only contains the backend stuff for now. I just want to see if the tests still work. |
|
@mheon FYI |
Luap99
added
4.0
breaking-change
Luap99
force-pushed
the
network-db
branch
12 times, most recently
from
d9c902a to
040cdb1
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@TomSweeneyRedHat PTAL at the docs. |
|
|
||
| Remember that the MAC address in an Ethernet network must be unique. | ||
| The IPv6 link-local address will be based on the device's MAC address | ||
| according to RFC4862. | ||
|
|
||
| To use more than one network together with a static mac address use the *mac* option described under **--network**. |
There was a problem hiding this comment.
If you take the IP suggestion above, do similar here.
The new network db structure stores everything in the networks bucket. Previously some network settings were not written the the network bucket and only stored in the container config. Instead of the old format which used the container ID as value in the networks buckets we now use the PerNetworkoptions struct there. To migrate existing users we use the state.GetNetworks() function. If it fails to read the new format it will automatically migrate the old config format to the new one. This is allows a flawless migration path. Signed-off-by: Paul Holzinger <[email protected]>
We do not need to return a extra bool. Signed-off-by: Paul Holzinger <[email protected]>
Make sure we create new containers in the db with the correct structure. Also remove some unneeded code for alias handling. We no longer need this functions. The specgen format has not been changed for now. Signed-off-by: Paul Holzinger <[email protected]>
Network connect now supports setting a static ipv4, ipv6 and mac address for the container network. The options are added to the cli and api. Fixes containers#9883 Signed-off-by: Paul Holzinger <[email protected]>
The swagger api docs used the extra Body struct as part of the request which is wrong. We just want the plain type. Signed-off-by: Paul Holzinger <[email protected]>
Add the new networks format to specgen. For api users cni_networks is still supported to make migration easier however the static ip and mac fields are removed. Signed-off-by: Paul Holzinger <[email protected]>
Rework the --network parse logic to support multiple networks with specific network configuration settings. --network can now be set multiple times. For bridge network mode the following options have been added: - **alias=name**: Add network-scoped alias for the container. - **ip=IPv4**: Specify a static ipv4 address for this container. - **ip=IPv6**: Specify a static ipv6 address for this container. - **mac=MAC**: Specify a static mac address address for this container. - **interface_name**: Specify a name for the created network interface inside the container. So now you can set --network bridge:ip=10.88.0.10,mac=44:33:22:11:00:99 for the default bridge network as well as for network names. This is better than using --ip because we can set the ip per network without any confusion which network the ip address should be assigned to. The --ip, --mac-address and --network-alias options are still supported but --ip or --mac-address can only be set when only one network is set. This limitation already existed previously. The ability to specify a custom network interface name is new Fixes containers#11534 Signed-off-by: Paul Holzinger <[email protected]>
Allow the same --network options for play kube as for podman run/create. Signed-off-by: Paul Holzinger <[email protected]>
It is important that we store the current networks from the db in the config. Also make sure to properly handle aliases and ignore static ip/mac addresses. Signed-off-by: Paul Holzinger <[email protected]>
Because we cannot reqad the networking mode in the frontent because we should always use the server default we have to parse the mac and ip address to the server via a default network. Now when the server reads the default nsmode it has to reject the provided networks when the mode is not set to bridge. Signed-off-by: Paul Holzinger <[email protected]>
|
@TomSweeneyRedHat Thanks. I applied your changes |
|
@mheon do your final review of migration code and merge |
mheon
reviewed
Dec 14, 2021
| // this value is only used for container create. | ||
| // Added in podman 4.0, previously NetworksDeprecated was used. Make | ||
| // sure to not change the json tags. | ||
| Networks map[string]types.PerNetworkOptions `json:"newNetworks,omitempty"` |
There was a problem hiding this comment.
Do we need this if we always use c.networks() instead?
There was a problem hiding this comment.
we need it in order to add networks during container creation with WithNetNS()
There was a problem hiding this comment.
the other option would be to add networks as function parameter for all container create function calls but this would make the diff even bigger
There was a problem hiding this comment.
Ahhh. OK, I can see how this works.
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
network db rewrite: migrate existing settings
The new network db structure stores everything in the networks bucket.
Previously some network settings were not written the the network bucket
and only stored in the container config.
Instead of the old format which used the container ID as value in the
networks buckets we now use the PerNetworkoptions struct there.
To migrate existing users we use the state.GetNetworks() function. If it
failes to read the new format it will automatically migrate the old
config format to the new one. This is allows a flawless migration path.
remove unneeded return value from c.Networks()
We do not need to return a extra bool.
network db: add new structure to container create
Make sure we create new containers in the db with the correct structure.
Also remove some unneeded code for alias handling. We no longer need this
functions.
How to verify it
Which issue(s) this PR fixes:
Special notes for your reviewer:
look at the individual commits instead of the full diff at once