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.

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

Fix snapshot key error #358

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Fix snapshot key error #358

merged 1 commit into from
Mar 20, 2024

Conversation

shivabohemian
Copy link

When obtaining the container information fails, the key for deleting the snapshot is incorrect.

Description

Motivation and Context

  • I have raised an issue to propose this change this is required

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Commits:

  • I've read the CONTRIBUTION guide
  • My commit message has a body and describe how this was tested and why it is required.
  • I have signed-off my commits with git commit -s for the Developer Certificate of Origin (DCO)

Code:

  • My code follows the code style of this project.
  • I have added tests to cover my changes.

Docs:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

When obtaining the container information fails, the key for deleting the snapshot is incorrect.

Signed-off-by: wangqiang <shivaqiang@qq.com>
@@ -57,7 +57,7 @@ func Remove(ctx context.Context, client *containerd.Client, name string) error {

} else {
service := client.SnapshotService("")
key := name + "snapshot"
key := name + "-snapshot"
Copy link
Member

Choose a reason for hiding this comment

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

Can you link to where this key is used in the codebase?

Please see the contributing guide. Where is the issue that shows how to reproduce and observe this problem?

Copy link
Author

Choose a reason for hiding this comment

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

Ok, you can look here and here.
In some cases, deleting a snapshot with the wrong key will fail, resulting in the failure to create the faas container the next time.

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Needs more info and an issue to reproduce the problem.

@alexellis
Copy link
Member

The solution looks correct so I'll merge it, but we need issues to be created before PRs, and proof of the issue, plus proof of the fix.

@alexellis alexellis merged commit dd31784 into openfaas:master Mar 20, 2024
1 check passed
@alexellis
Copy link
Member

Feel free to test again, and see if your snapshots are being cleared up:

https://github.com/openfaas/faasd/releases/tag/0.18.7

@shivabohemian
Copy link
Author

shivabohemian commented Mar 20, 2024

After I made changes to the code and tested it, things are indeed running normally now. Previously, an error like the one below would occur and it would require manual deletion of the snapshot in order to start successfully.
Unexpected status: 400, message: unable to create container: nats, error: snapshot "nats-snapshot": already exists.

@alexellis
Copy link
Member

That's great, thanks for picking this up.

What kinds of things are you using faasd for?

Would you like to meet us at the weekly call and chat more? https://docs.openfaas.com/community/#weekly-office-hours

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants