Skip to content

Support for container stub drive reuse #339

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

Conversation

Zyqsempai
Copy link
Contributor

Signed-off-by: bpopovschi [email protected]

Issue #243

Added possibility to reuse stub drives of just deleted containers:

  • The Unmount() method was added to agent/drive_handler to support unmounting used stub drives.
  • The Release() method was added to aggregate all steps we need to release used stub.
  • Modified task Delete() method to use new Release() method.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Zyqsempai Zyqsempai force-pushed the 243-add-release-method-for-stub-drives branch from a4021a5 to 2e9a5c2 Compare November 21, 2019 11:35
@Zyqsempai
Copy link
Contributor Author

@sipsma PTAL and maybe you will have an idea how to fix test, it looks like hostPath is missing, but I have no clue how to set it in that test.

Copy link
Contributor

@sipsma sipsma left a comment

Choose a reason for hiding this comment

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

Apologies for the delayed review, I was on vacation the end of last week.

I took a first pass look and left some comments. I'm going to take another look after thinking about the corner cases surrounding what we should do if Release fails and will get back to on your question about the test at that time.

Copy link
Contributor

@sipsma sipsma left a comment

Choose a reason for hiding this comment

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

Left a few more comments, for some reason the formatting error I commented on below are preventing the tests from running now (./drive_handler.go:112:15: Errorf format %s reads arg #1, but call has 0 args), so I'm not able to see the error you were asking about earlier. Once that's fixed I'll be able to take a look again though.

@Zyqsempai Zyqsempai force-pushed the 243-add-release-method-for-stub-drives branch 3 times, most recently from 352bb02 to 3638150 Compare November 27, 2019 15:38
Copy link
Contributor

@sipsma sipsma left a comment

Choose a reason for hiding this comment

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

Looking good! A few more comments but on a high-level everything is coming together. One more thing to keep in mind is that we'll want some integ-test coverage for this. Luckily, we already have an integ test, TestCreateTooManyContainers, that verifies the previous behavior of failing when you try to create a 3rd container after creating 2 containers in a 2-container VM and deleting one. So, you should be able to just update that test (and change its name) to just verify that now the behavior actually works. Let us know if you run into any trouble with updating that or have questions about the integ tests.

@Zyqsempai
Copy link
Contributor Author

@sipsma PTAL

Copy link
Contributor

@sipsma sipsma left a comment

Choose a reason for hiding this comment

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

LGTM! Please do a rebase on master as I think this is a a decent chunk of commits behind (but I'm not foreseeing any major conflicts), after that should be good to approve

@Zyqsempai Zyqsempai force-pushed the 243-add-release-method-for-stub-drives branch from e2eb4ac to c6343c3 Compare December 4, 2019 20:41
@Zyqsempai
Copy link
Contributor Author

@sipsma Done!

Copy link
Contributor

@sipsma sipsma left a comment

Choose a reason for hiding this comment

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

Thanks again!

Copy link
Contributor

@kzys kzys left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

nmeyerhans pushed a commit that referenced this pull request Dec 4, 2019
#339

Signed-off-by: Noah Meyerhans <[email protected]>
@nmeyerhans nmeyerhans merged commit c6343c3 into firecracker-microvm:master Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants