Make sure containers are deleted correctly#225
Make sure containers are deleted correctly#225kzys merged 1 commit intofirecracker-microvm:masterfrom
Conversation
|
I initially thought that the issue itself has been resolved, but it seems not.
|
|
Now I think our code was fine but the tests were wrong. Let's see. I'm going to work on the comments after CIs are being green. |
| containerCount = 1 | ||
| exitAfterAllTasksDeleted = true | ||
|
|
||
| log.Infof("will start a single-task VM %s since no VMID has been provided", s.vmID) |
251278d to
68d2306
Compare
|
All tests are passing finally! |
sipsma
left a comment
There was a problem hiding this comment.
One minor nit comment and I additionally think the commits should be squashed into 1 before merging, but otherwise looks good!
| return nil, err | ||
| } | ||
|
|
||
| if req.ExecID != "" { |
There was a problem hiding this comment.
nit: I'd add a comment explaining why we skip deletion in this case
Before this change, we were only deleting the bundle directory on the deletion of a VM. Due to that, we had symlink conflicts when - We have multiple containers on a single VM - The containers have the same name Closes firecracker-microvm#65. Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
cni/cmd/tc-redirect-tap: fix dropped test error
* Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. (firecracker-microvm#263) * Fix Benchmark Goroutine (firecracker-microvm#259) * Jailer configuration API cleanup and improved logging with Debug log level (firecracker-microvm#255) * Firecracker is internally has an instance ID, but the SDK didn't have the way to configure the ID. This change connects Config.VMID to the instance ID. (firecracker-microvm#253) * Fixed error that was not being test against in `TestWait` (firecracker-microvm#251) * Fixes issue where socket path may not be defined since the config file has yet to be loaded (firecracker-microvm#230) * Fixed error that was not being test against in `TestNewPlugin` (firecracker-microvm#225) * Download Firecracker 0.21.1 and its jailer from Makefile (firecracker-microvm#218) Signed-off-by: xibz <impactbchang@gmail.com>
Issue #, if available:
#65
Description of changes:
While we cannot repro #65 anymore, we'd like to have some tests to
prevent regression.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.