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

Broken snapshotter state on SIGINT #1440

Open
sondavidb opened this issue Oct 20, 2023 · 3 comments · May be fixed by #1526
Open

Broken snapshotter state on SIGINT #1440

sondavidb opened this issue Oct 20, 2023 · 3 comments · May be fixed by #1526

Comments

@sondavidb
Copy link

sondavidb commented Oct 20, 2023 •
edited

When calling SIGINT on the daemon, it cannot be loaded into a working state without some extra effort. This is because SIGINT triggers the cleanup flag, which wipes the content store. However, the metadata is never synced to update this state. This leaves me with two questions.

First, looking through previous issues I found #703 (comment) made note of the snapshotter removing its snapshot directories on cleanup. Could I ask for an explanation for this behavior? I would imagine that one would not want to re-pull images after restarting the daemon.

Second, could this broken state be fixed by updating the metadata before line 662 in Close() in snapshot.go? I tried to do it myself but couldn't figure out a way to get the snapshot keys. However, since cleanup wipes the container store clean regardless, as a proof of concept, I called os.Remove() right before line 662, like so:

    if err := os.Remove(filepath.Join(o.root, "metadata.db")); err != nil {
        log.G(ctx).WithError(err).Warn("failed to wipe metadata")
    }
    return o.ms.Close()

This lets the daemon start in a mostly working state, but you must still manually remove and re-pull the image, as the image is still in the containerd content store. This could probably fixed by natively interacting with the db via the metadata package. Hence, this is a pretty inelegant solution IMO.

EDIT: I found that Walk gets all the keys, so calling that then calling Remove on each of them removes them from the metadata. However, it still leaves it in the containerd content store, so we still have the same issue of being unable to re-pull the image unless you remove it manually and pull again.

I can open a PR with these changes if people want this, as it removes the need to set allow_invalid_mounts_on_restart=true to start the daemon to remove the snapshot. It's still imperfect for the above reason but it would be a nice QOL change IMO.
@ktock
Copy link
Member

ktock commented Oct 24, 2023

Could I ask for an explanation for this behavior? I would imagine that one would not want to re-pull images after restarting the daemon.

For avoiding leaving empty snapshot directory that looks valid but the emptiness is actually not the correct contents as the snapshots. You can use non-SIGINT signals for avoiding this behaviour.

I found that Walk gets all the keys, so calling that then calling Remove on each of them removes them from the metadata. However, it still leaves it in the containerd content store, so we still have the same issue of being unable to re-pull the image unless you remove it manually and pull again.
I can open a PR with these changes if people want this, as it removes the need to set allow_invalid_mounts_on_restart=true to start the daemon to remove the snapshot. It's still imperfect for the above reason but it would be a nice QOL change IMO.

Thanks for the suggestion. So user still need some manual operation for keeping consistency between containerd & snapshotter metadata db? It would be great if we have docs about how to fix it manually.

@sondavidb
Copy link
Author

sondavidb commented Oct 24, 2023 •
edited

Thanks for the reply!

For avoiding leaving empty snapshot directory that looks valid but the emptiness is actually not the correct contents as the snapshots. You can use non-SIGINT signals for avoiding this behavior.

I understand, I'm more trying to understand if this is intentional behavior, or if this is something we would want to change.

Thanks for the suggestion. So user still need some manual operation for keeping consistency between containerd & snapshotter metadata db? It would be great if we have docs about how to fix it manually.

Correct. Assuming we want to keep current behavior of removing the snapshots on cleanup, if we update the metadata when the daemon is killed with SIGINT, the daemon can start without changing allow_invalid_mounts_on_restart in the TOML. However, the user then must remove the image if they want to re-pull it, as the containerd store will believe the image already exists, but all other functionality should be normal.

For an explanation: This behavior is caused by the cleanup functions wiping the snapshots in the stargz content store and the change not being reflected in metadata.db. This could either be fixed by either updating the metadata after removing the directories (though, as mentioned above, this is only a partial fix), or not removing the directories at all.

Based on your feedback, I'd be happy to open a PR with some changes. Currently I am looking at three options:

  • Doc-only change of how to return the daemon to a working state after SIGINT.
  • Update cleanup function on SIGINT to update metadata.db so that the daemon can start up in a semi-functioning state, without the need to set the TOML variable. This change will also include documentation to get daemon back to a fully working state.
  • Update cleanup function on SIGINT so that directories are not removed. This would allow the daemon to get back to a fully working state on SIGINT, and thus would fully solve the issue, but would also change what appears to be intentional behavior.

Please let me know which one would be the most preferred. Apologies for the long comment. Thanks again!

@sondavidb sondavidb linked a pull request Jan 13, 2024 that will close this issue
Open
@sondavidb
Copy link
Author

Hi, just wanted to bump this issue again.

From what I've gathered, we are suggesting people to use SIGTERM to kill the daemon to avoid this problematic behavior. This keeps the uncomrpessed snapshot files on disk, as well as keeps the mounts. I think it would be best to ship a fix where SIGINT will unmount the FUSE file systems but still keep snapshot files on disk. This would imitate already existing behavior, but have the added benefit of not having to manually remove FUSE mounts.

I've created a PR at #1526 to implement this, but feel free to close/suggest modifications to this behavior. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Keep directories when SIGINT sent to daemon sondavidb/stargz-snapshotter
2 participants
@ktock @sondavidb