From 45a40bf587e5d9743f5eb03d113691e4a354c7e3 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Fri, 9 Dec 2022 16:59:39 +0100 Subject: [PATCH] checkpoint restore: fix --ignore-static-ip/mac With the 4.0 network rewrite I introduced a regression in 094e1d70dee1. It only covered the case where a checkpoint is restored via --import. The normal restore path was not covered since the static ip/mac are now part in an extra db bucket. This commit fixes that by changing the config in the db. Note that there were no test for --ignore-static-ip/mac so I added a big system test which should cover all cases (even the ones that already work). This is not exactly pretty but I don't have to enough time to come up with something better at the moment. Fixes #16666 Signed-off-by: Paul Holzinger --- libpod/boltdb_state.go | 17 ++- libpod/container_internal_common.go | 21 ++- libpod/state.go | 2 + pkg/checkpoint/checkpoint_restore.go | 12 -- test/system/520-checkpoint.bats | 183 +++++++++++++++++++++++++++ 5 files changed, 220 insertions(+), 15 deletions(-) diff --git a/libpod/boltdb_state.go b/libpod/boltdb_state.go index 77c2348925..db48171d63 100644 --- a/libpod/boltdb_state.go +++ b/libpod/boltdb_state.go @@ -1232,6 +1232,16 @@ func (s *BoltState) GetNetworks(ctr *Container) (map[string]types.PerNetworkOpti // NetworkConnect adds the given container to the given network. If aliases are // specified, those will be added to the given network. func (s *BoltState) NetworkConnect(ctr *Container, network string, opts types.PerNetworkOptions) error { + return s.networkModify(ctr, network, opts, true) +} + +// NetworkModify will allow you to set new options on an existing connected network +func (s *BoltState) NetworkModify(ctr *Container, network string, opts types.PerNetworkOptions) error { + return s.networkModify(ctr, network, opts, false) +} + +// networkModify allows you to modify or add a new network, to add a new network use the new bool +func (s *BoltState) networkModify(ctr *Container, network string, opts types.PerNetworkOptions, new bool) error { if !s.valid { return define.ErrDBClosed } @@ -1278,11 +1288,14 @@ func (s *BoltState) NetworkConnect(ctr *Container, network string, opts types.Pe return fmt.Errorf("container %s does not have a network bucket: %w", ctr.ID(), define.ErrNoSuchNetwork) } netConnected := ctrNetworksBkt.Get([]byte(network)) - if netConnected != nil { + + if new && netConnected != nil { return fmt.Errorf("container %s is already connected to network %q: %w", ctr.ID(), network, define.ErrNetworkConnected) + } else if !new && netConnected == nil { + return fmt.Errorf("container %s is not connected to network %q: %w", ctr.ID(), network, define.ErrNoSuchNetwork) } - // Add the network + // Modify/Add the network if err := ctrNetworksBkt.Put([]byte(network), optBytes); err != nil { return fmt.Errorf("adding container %s to network %s in DB: %w", ctr.ID(), network, err) } diff --git a/libpod/container_internal_common.go b/libpod/container_internal_common.go index bfb5d233e4..38b5887897 100644 --- a/libpod/container_internal_common.go +++ b/libpod/container_internal_common.go @@ -1255,6 +1255,25 @@ func (c *Container) restore(ctx context.Context, options ContainerCheckpointOpti c.state.RestoreLog = path.Join(c.bundlePath(), "restore.log") c.state.CheckpointPath = c.CheckpointPath() + if options.IgnoreStaticIP || options.IgnoreStaticMAC { + networks, err := c.networks() + if err != nil { + return nil, 0, err + } + + for net, opts := range networks { + if options.IgnoreStaticIP { + opts.StaticIPs = nil + } + if options.IgnoreStaticMAC { + opts.StaticMAC = nil + } + if err := c.runtime.state.NetworkModify(c, net, opts); err != nil { + return nil, 0, fmt.Errorf("failed to rewrite network config: %w", err) + } + } + } + // Read network configuration from checkpoint var netStatus map[string]types.StatusBlock _, err := metadata.ReadJSONFile(&netStatus, c.bundlePath(), metadata.NetworkStatusFile) @@ -1282,7 +1301,7 @@ func (c *Container) restore(ctx context.Context, options ContainerCheckpointOpti perNetOpts.StaticIPs = nil for name, netInt := range netStatus[network].Interfaces { perNetOpts.InterfaceName = name - if !options.IgnoreStaticIP { + if !options.IgnoreStaticMAC { perNetOpts.StaticMAC = netInt.MacAddress } if !options.IgnoreStaticIP { diff --git a/libpod/state.go b/libpod/state.go index 5fa8dc257b..6548a6828a 100644 --- a/libpod/state.go +++ b/libpod/state.go @@ -104,6 +104,8 @@ type State interface { //nolint:interfacebloat GetNetworks(ctr *Container) (map[string]types.PerNetworkOptions, error) // Add the container to the given network with the given options NetworkConnect(ctr *Container, network string, opts types.PerNetworkOptions) error + // Modify the container network with the given options. + NetworkModify(ctr *Container, network string, opts types.PerNetworkOptions) error // Remove the container from the given network, removing all aliases for // the container in that network in the process. NetworkDisconnect(ctr *Container, network string) error diff --git a/pkg/checkpoint/checkpoint_restore.go b/pkg/checkpoint/checkpoint_restore.go index c28ff3110e..f461363551 100644 --- a/pkg/checkpoint/checkpoint_restore.go +++ b/pkg/checkpoint/checkpoint_restore.go @@ -81,18 +81,6 @@ func CRImportCheckpoint(ctx context.Context, runtime *libpod.Runtime, restoreOpt } } - if restoreOptions.IgnoreStaticIP || restoreOptions.IgnoreStaticMAC { - for net, opts := range ctrConfig.Networks { - if restoreOptions.IgnoreStaticIP { - opts.StaticIPs = nil - } - if restoreOptions.IgnoreStaticMAC { - opts.StaticMAC = nil - } - ctrConfig.Networks[net] = opts - } - } - ctrID := ctrConfig.ID newName := false diff --git a/test/system/520-checkpoint.bats b/test/system/520-checkpoint.bats index ebb060b21a..dc49871749 100644 --- a/test/system/520-checkpoint.bats +++ b/test/system/520-checkpoint.bats @@ -220,4 +220,187 @@ function teardown() { trim=$(sed -z -e 's/[\r\n]\+//g' <<<"$output") is "$trim" "READY123123" "File lock restored" } + +@test "podman checkpoint/restore ip and mac handling" { + # Refer to https://github.com/containers/podman/issues/16666#issuecomment-1337860545 + # for the correct behavior, this should cover all cases listed there. + local netname=net-$(random_string) + local subnet="$(random_rfc1918_subnet)" + run_podman network create --subnet "$subnet.0/24" $netname + + run_podman run -d --network $netname $IMAGE sleep inf + cid="$output" + # get current ip and mac + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").IPAddress}}" + ip1="$output" + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" + mac1="$output" + + run_podman container checkpoint $cid + is "$output" "$cid" + run_podman container restore $cid + is "$output" "$cid" + + # now get mac and ip after restore they should be the same + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").IPAddress}}" + ip2="$output" + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" + mac2="$output" + + assert "$ip2" == "$ip1" "ip after restore should match" + assert "$mac2" == "$mac1" "mac after restore should match" + + # restart the container we should get a new ip/mac because they are not static + run_podman restart $cid + + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").IPAddress}}" + ip3="$output" + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" + mac3="$output" + + # the ip/mac should be different this time + assert "$ip3" != "$ip1" "ip after restart should be different" + assert "$mac3" != "$mac1" "mac after restart should be different" + + # restore with --ignore-static-ip/mac + run_podman container checkpoint $cid + is "$output" "$cid" + run_podman container restore --ignore-static-ip --ignore-static-mac $cid + is "$output" "$cid" + + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").IPAddress}}" + ip4="$output" + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" + mac4="$output" + + # the ip/mac should be different this time + assert "$ip4" != "$ip3" "ip after restore --ignore-static-ip should be different" + assert "$mac4" != "$mac3" "mac after restore --ignore-static-mac should be different" + + local archive=$PODMAN_TMPDIR/checkpoint.tar.gz + + # now checkpoint and export the container + run_podman container checkpoint --export "$archive" $cid + is "$output" "$cid" + # remove container + run_podman rm -t 0 -f $cid + + # restore it without new name should keep the ip/mac, we also get a new container id + run_podman container restore --import "$archive" + cid="$output" + + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").IPAddress}}" + ip5="$output" + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" + mac5="$output" + assert "$ip5" == "$ip4" "ip after restore --import should match" + assert "$mac5" == "$mac4" "mac after restore --import should match" + + run_podman rm -t 0 -f $cid + + # now restore it again but with --name this time, it should not keep the + # mac and ip to allow restoring the same container with different names + # at the same time + run_podman container restore --import "$archive" --name "newcon" + cid="$output" + + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").IPAddress}}" + ip6="$output" + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" + mac6="$output" + assert "$ip6" != "$ip5" "ip after restore --import --name should be different" + assert "$mac6" != "$mac5" "mac after restore --import --name should be different" + + run_podman rm -t 0 -f $cid + + # now create a container with a static mac and ip + local static_ip="$subnet.2" + local static_mac="92:d0:c6:0a:29:38" + run_podman run -d --network "$netname:ip=$static_ip,mac=$static_mac" $IMAGE sleep inf + cid="$output" + + run_podman container checkpoint $cid + is "$output" "$cid" + run_podman container restore --ignore-static-ip --ignore-static-mac $cid + is "$output" "$cid" + + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").IPAddress}}" + ip7="$output" + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" + mac7="$output" + assert "$ip7" != "$static_ip" "static ip after restore --ignore-static-ip should be different" + assert "$mac7" != "$static_mac" "static mac after restore --ignore-static-mac should be different" + + # restart the container to make sure the change is actually persistent in the config and not just set for restore + run_podman restart $cid + + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").IPAddress}}" + ip8="$output" + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" + mac8="$output" + assert "$ip8" != "$static_ip" "static ip after restore --ignore-static-ip and restart should be different" + assert "$mac8" != "$static_mac" "static mac after restore --ignore-static-mac and restart should be different" + assert "$ip8" != "$ip7" "static ip after restore --ignore-static-ip and restart should be different" + assert "$mac8" != "$ip" "static mac after restore --ignore-static-mac and restart should be different" + + run_podman rm -t 0 -f $cid + + # now create container again and try the same again with --export and --import + run_podman run -d --network "$netname:ip=$static_ip,mac=$static_mac" $IMAGE sleep inf + cid="$output" + + run_podman container checkpoint --export "$archive" $cid + is "$output" "$cid" + # remove container + run_podman rm -t 0 -f $cid + + # restore normal should keep static ip + run_podman container restore --import "$archive" + cid="$output" + + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").IPAddress}}" + ip9="$output" + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" + mac9="$output" + assert "$ip9" == "$static_ip" "static ip after restore --import should match" + assert "$mac9" == "$static_mac" "static mac after restore --import should match" + + # restart the container to make sure the change is actually persistent in the config and not just set for restore + run_podman restart $cid + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").IPAddress}}" + ip10="$output" + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" + mac10="$output" + assert "$ip10" == "$static_ip" "static ip after restore --import and restart should match" + assert "$mac10" == "$static_mac" "static mac after restore --import and restart should match" + + run_podman rm -t 0 -f $cid + + # restore normal without keeping static ip/mac + run_podman container restore --ignore-static-ip --ignore-static-mac --import "$archive" + cid="$output" + + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").IPAddress}}" + ip11="$output" + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" + mac11="$output" + assert "$ip11" != "$static_ip" "static ip after restore --import --ignore-static-ip should be different" + assert "$mac11" != "$static_mac" "static mac after restore --import --ignore-static-mac should be different" + + # restart the container to make sure the change is actually persistent in the config and not just set for restore + run_podman restart $cid + + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").IPAddress}}" + ip12="$output" + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" + mac12="$output" + assert "$ip12" != "$static_ip" "static ip after restore --import --ignore-static-ip and restart should be different" + assert "$mac12" != "$static_mac" "static mac after restore --ignore-static-mac and restart should be different" + assert "$ip12" != "$ip11" "static ip after restore --import --ignore-static-ip and restart should be different" + assert "$mac12" != "$ip11" "static mac after restore --ignore-static-mac and restart should be different" + + run_podman rm -t 0 -f $cid + run_podman network rm $netname +} + # vim: filetype=sh