-
Notifications
You must be signed in to change notification settings - Fork 574
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
overlord/snapstate/backend: some copydata improvements #3611
Conversation
The state machinery needs that every handler either succeeds or fails entirely. Currently copy-snap-data has a few half-failure modes; this addresses that. In particular, copying data has multiple steps that can fail independently. This makes it so that when one of them fails, the previous ones are cleaned up (undone), and anything that had been trashed in the same operation is brought back from the trash. This is easy to check, currently: simply snap refresh <some snap> --revision=<the current revision it's on> and you'll see the operation fail, but also you'll notice that if the snap had data dirs they're gone, moved to .old. This PR also enables this operation to succeed, by explicitly checking for it in both directions.
Codecov Report
@@ Coverage Diff @@
## master #3611 +/- ##
==========================================
+ Coverage 74.9% 74.99% +0.08%
==========================================
Files 380 383 +3
Lines 32952 33237 +285
==========================================
+ Hits 24682 24925 +243
- Misses 6477 6504 +27
- Partials 1793 1808 +15
Continue to review full report at Codecov.
|
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.
+1, nice, one suggestion for a TODO comment
if err := os.RemoveAll(newDir); err != nil { | ||
logger.Noticef("while undoing creation of new data directory %q: %v", newDir, err) | ||
} | ||
if err := untrash(newDir); err != nil { |
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.
❤️, nice
// try to fix that; hope for the best | ||
if e := untrash(newPath); e != nil { | ||
// oh noes | ||
msg += fmt.Sprintf("; and when trying to restore the old data directory: %v", e) |
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.
// TODO: issue a warning to the user that data was lost
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.
question but otherwise lgtm
@@ -40,13 +40,20 @@ func (b Backend) CopySnapData(newSnap, oldSnap *snap.Info, meter progress.Meter) | |||
|
|||
if oldSnap == nil { | |||
return os.MkdirAll(newSnap.DataDir(), 0755) | |||
} else if oldSnap.Revision == newSnap.Revision { |
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.
should we turn this into a NOP much higher ?
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.
Probably, but that doesn't make this wrong?
The state machinery needs that every handler either succeeds or fails
entirely. Currently copy-snap-data has a few half-failure modes; this
addresses that.
In particular, copying data has multiple steps that can fail
independently. This makes it so that when one of them fails, the
previous ones are cleaned up (undone), and anything that had been
trashed in the same operation is brought back from the trash.
This is easy to check, currently: simply
and you'll see the operation fail, but also you'll notice that if the
snap had data dirs they're gone, moved to .old. This PR also enables
this operation to succeed, by explicitly checking for it in both
directions.