-
Notifications
You must be signed in to change notification settings - Fork 68
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
Ignore missing config.v2.json files on migration #303
base: master
Are you sure you want to change the base?
Conversation
An error occurred whilst building your landr site preview:
|
Totally untested as of now, therefore keeping PR as a draft. |
[alexgg] This has attached https://jel.ly.fish/9aaf5f96-265c-4c52-9e59-074e973b53f8 |
I added a test and some error handling but not quite ready to merge this as my tests aren't working when I run individual tests. The migrate_test.go tests pass when I run all the tests: But these all fail with package errors and GOROOT/GOPATH errors so I need to sort that out. |
Hey @zoobot ! I think you need to include the
|
That . worked! :) thanks @lmbarros |
Adding this here as reference for all the changes to make this happen: And the improvement: |
Update on this @lmbarros : Was hoping to get a rpi3 to test on, Ross is sending one. Making changes today: On migration failure due to missing config.v2.json: |
We have seen some cases in which the file `/var/lib/docker/containers/<UUID>/config.v2.json` was not found for certain UUIDs. This causes the whole migration to fail and, worse, causes our migration cleanup code to fail. A missing `config.v2.json` indicates that directory does not contain a valid container, so we should not even try to migrate them. That's what this commit does: it makes the migration ignore directories with a missing `config.v2.json`. I don't think we should have directories like this in the first place, this is probably the side effect of some other issue. That said, Docker itself ignores these directories (after logging a warning) during startup, so here are just bringing the Engine in line with the standard behavior. Signed-off-by: Leandro Motta Barros <leandro@balena.io> Change-type: patch
8162043
to
b71b5df
Compare
@lmbarros Should we wait until I have a RP3 device or QEMU to test this on or roll it out with the current go test? Thanks for looking |
pkg/storagemigration/failcleanup.go
Outdated
@@ -36,5 +31,23 @@ func failCleanup(root string) error { | |||
return err | |||
} | |||
|
|||
err = SwitchAllContainersStorageDriver(root, "aufs") |
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.
If any of the removeDirIfExists()
calls above fails, we'd return before reaching this call here, and I believe this could leave the device in an inconsistent state. (Specifically: if any containers were successfully migrated, they'd be tagged as being overlay2 containers, but the corresponding overlay2 data might have been deleted.)
pkg/storagemigration/failcleanup.go
Outdated
return nil | ||
} | ||
|
||
func failCleanupContainer(root string, containerID string) error { |
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.
I think this is not being currently called anywhere.
Also there's a typo in the changelog entry: "Rollback to ausf..." |
I often do this because I am paranoid and like the ritual, but no, not necessarily. In fact, I think it's generally more important to have a good automated test case (either here or in meta-balena) capable of ensuring that we really fixed the issue. |
Add test for missing config.v2.json Changelog-entry: Rollback to aufs and cleanup overlay2 in case of missing config.v2.json Change-type: patch Signed-off-by: zoobot <rommims@gmail.com>
b71b5df
to
3915950
Compare
TODO: One thing we should do in this PR: if every attempt of making the migration fails, simply remove all images and containers and let the device redownload everything. (This is actually how we designed the migration, but not the observed behavior.)
We have seen some cases in which the file
/var/lib/docker/containers/<UUID>/config.v2.json
was not found forcertain UUIDs. This causes the whole migration to fail and, worse,
causes our migration cleanup code to fail.
A missing
config.v2.json
indicates that directory does not contain avalid container, so we should not even try to migrate them. That's what
this commit does: it makes the migration ignore directories with a
missing
config.v2.json
.I don't think we should have directories like this in the first place,
this is probably the side effect of some other issue. That said, Docker
itself ignores these directories (after logging a warning) during
startup, so here are just bringing the Engine in line with the standard
behavior.
Resolves #301
- What I did
- How I did it
- How to verify it
- Description for the changelog