checkpoint restore: fix --ignore-static-ip/mac #16800
Conversation
|
[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
libpod/container_internal_common.go
Outdated
| // TODO: we should properly make this in one db call instead to prevent partial states on interupt | ||
| if err := c.runtime.state.NetworkDisconnect(c, net); err != nil { | ||
| return nil, 0, fmt.Errorf("failed to rewrite network config: %w", err) | ||
| } | ||
| if err := c.runtime.state.NetworkConnect(c, net, opts); err != nil { | ||
| return nil, 0, fmt.Errorf("failed to rewrite network config: %w", err) | ||
| } |
There was a problem hiding this comment.
@mheon PTAL, should I create a function to make this in a single db call?
There was a problem hiding this comment.
Is the intent here just to mutate the options of the given network? If so, a NetworkModify call could be added. I can't think of any adverse side effects for doing so...
That said, is there a reason this has to be done here, instead of setting the options correctly when we first restore the container / add it to the DB?
There was a problem hiding this comment.
That said, is there a reason this has to be done here, instead of setting the options correctly when we first restore the container / add it to the DB?
podman container checkpoint/restore keeps the cotnainer in the db, so we have to change the db content
If you look at the code I removed, the restore --import path already ignores the mac/ip correctly, moving it here makes sure all cases work as expected.
With the 4.0 network rewrite I introduced a regression in 094e1d7. 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 containers#16666 Signed-off-by: Paul Holzinger <[email protected]>
Luap99
force-pushed
the
restore-static-ip
branch
from
fde243a to
45a40bf
Compare
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
With the 4.0 network rewrite I introduced a regression in 094e1d7. 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
Does this PR introduce a user-facing change?