-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite Rename backend in a more atomic fashion #9575
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon 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 |
I need to think of a few tests for this after lunch - we unblocked renaming containers with active exec sessions, for example. |
Move the core of renaming logic into the DB. This guarantees a lot more atomicity than we have right now (our current solution, removing the container from the DB and re-creating it, is *VERY* not atomic and prone to leaving a corrupted state behind if things go wrong. Moving things into the DB allows us to remove most, but not all, of this - there's still a potential scenario where the c/storage rename fails but the Podman rename succeeds, and we end up with a mismatched state. Signed-off-by: Matthew Heon <[email protected]>
Test added |
Looks like this is going green. @containers/podman-maintainers PTAL |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
One question: Right now we do not update update the dnsname plugin dns name. Do you think we should call network reload as last step? (Not for this PR)
/lgtm |
Tide is blocked on something, I'll check for what. @Luap99 A full reload is a little expensive, it would be better if we could just update dnsname - we need some reworking of it anyways to fix the flaw where it can only handle a single network the container is connected to, we can handle this at the same time when we get around to that. |
Move the core of renaming logic into the DB. This guarantees a lot more atomicity than we have right now (our current solution, removing the container from the DB and re-creating it, is VERY not atomic and prone to leaving a corrupted state behind if things go wrong. Moving things into the DB allows us to remove most, but not all, of this - there's still a potential scenario where the c/storage rename fails but the Podman rename succeeds, and we end up with a mismatched state.