Skip to content
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.

Sign up for GitHub

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

Jump to bottom

(Alternative to 1148): Don't blindly reuse state from a previous layer when re-creating it #1140

Merged
merged 1 commit into from May 2, 2022
Merged

(Alternative to 1148): Don't blindly reuse state from a previous layer when re-creating it #1140

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions drivers/overlay/overlay.go
View file
Open in desktop
Expand Up @@ -939,6 +939,16 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, disable
rootUID = int(st.UID())
rootGID = int(st.GID())
}

if _, err := system.Lstat(dir); err == nil {
logrus.Warnf("Trying to create a layer %#v while directory %q already exists; removing it first", id, dir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We default to showing warnings. Is this something we want normal users to see by default? Or should we drop this down to info?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that, this indicates an unexpected abort of a c/storage operation, and it is definitely something we want to have captured in any logs at the default level for postmortems if this cleanup is insufficient and/or incorrect and harmful.

I’m not so sure that it needs to be printed by default on a TTY during interactive use; at that point, there are unlikely to be any logs recorded, and the thing either works or it doesn’t. OTOH, it’s also a situation we shouldn’t be regularly getting into, so I don’t think that a warning should hurt.

Alternatively, #1148 should make this case unreachable assuming there are no bugs — and in that case we either don’t need this PR, or we can happily leave it on a warning level because it is clearly very unexpected. If I’m reading @nalind’s comments right, it seems preferable to have both this PR and #1148.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok leave it at Warning level.

// Don’t just os.RemoveAll(dir) here; d.Remove also removes the link in linkDir,
// so that we can’t end up with two symlinks in linkDir pointing to the same layer.
if err := d.Remove(id); err != nil {
return errors.Wrapf(err, "removing a pre-existing layer directory %q", dir)
}
}

if err := idtools.MkdirAllAndChownNew(dir, 0700, idPair); err != nil {
return err
}
Expand Down