feat: Add "Containers" propery in the "nerdctl network inspect" command. #4052
Conversation
|
Hey @tushar5526 From a first cursory check, it looks good. Let's have tests, though. Suggesting you use the new test framework for that (example here: https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/container/container_inspect_linux_test.go#L521-L549 - and doc here: https://github.com/containerd/nerdctl/blob/main/docs/testing/tools.md). |
| { | ||
| Description: "Verify that only active containers appear in the network inspect output", | ||
| Setup: func(data test.Data, helpers test.Helpers) { | ||
| helpers.Ensure("network", "create", "nginx-network-1") |
There was a problem hiding this comment.
nginx-network-1 is not guaranteed to be free and other tests using that same name may collide.
To avoid this, use data.Identifier("nginx-network-1") (or data.Identifier("123") or whatever suits you).
There was a problem hiding this comment.
Makes sense, switched to using data.Identifier
| Description: "Verify that only active containers appear in the network inspect output", | ||
| Setup: func(data test.Data, helpers test.Helpers) { | ||
| helpers.Ensure("network", "create", "nginx-network-1") | ||
| helpers.Ensure("network", "create", "nginx-network-2") |
There was a problem hiding this comment.
Same - use data.Identifier to get unique identifiers for any resource.
|
@tushar5526 can you cherry-pick the commit from this PR and add it to your PR here? Would like to see what happens for windows with your test + this commit, that updates to a more recent version of wincni |
pkg/cmd/network/inspect.go
Outdated
|
|
||
| nativeContainer, err := containerinspector.Inspect(ctx, container) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
I would probably continue instead of error out here.
You do not have locking on containers, so, just because it was there above does not mean it is still here, and that should be fine.
pkg/cmd/network/inspect.go
Outdated
| var containers []*native.Container | ||
|
|
||
| for _, container := range filteredContainers { | ||
| cStatus, err := containerutil.ContainerStatus(ctx, container) |
There was a problem hiding this comment.
Isn't containerinspector.Inspect (which you call a few lines below) already doing that for you?
Just inspect first, then use nativeContainer.Process.Status to do your check?
There was a problem hiding this comment.
Yes, you are correct. It should have been that way. I keep getting confused with two implementations of Container in native and containerd.
I am not sure either. @tushar5526 what about you come up with something that you think makes the most sense? (and the closest to what containerd returns) |
|
|
@tushar5526 #4055 got merged (thanks @AkihiroSuda for the fast merge). You can just rebase against latest main. |
tushar5526
force-pushed
the
add-containers-to-inspect-output
branch
from
d1ef131 to
048efe7
Compare
pkg/cmd/network/inspect.go
Outdated
| if err != nil { | ||
| continue | ||
| } | ||
| if nativeContainer.Process.Status.Status != containerd.Running { |
There was a problem hiding this comment.
.Process may be nil - or .Process.Status - so, I guess you need to check for that.
Signed-off-by: Tushar Gupta <[email protected]>
…t" output Signed-off-by: Tushar Gupta <[email protected]>
* move to using data.Identifier to generate unique-keys * refactor to use "inspect" to fetch the container status Signed-off-by: Tushar Gupta <[email protected]>
Signed-off-by: Tushar Gupta <[email protected]>
tushar5526
force-pushed
the
add-containers-to-inspect-output
branch
from
bbcd9f6 to
f7b3a1d
Compare
|
@apostasie, is it expected that a bunch of tests will fail? The errors in them does not seem related to my changes. |
We do currently have three key flaky offenders. Here are the associated log messages + tickets :
#3513 should hopefully soon get wacked by #4062 - once this is in, I'll look into the buildg one. Meanwhile, just look for these messages ^ - if you see them in a On your last run (at this time), I believe you just met the last two - "Mister buildg is late", and "Mister CNI forgot his conflist somewhere" 😆 |
Closes #2998
I am creating this MP (tests need to be added) to get an initial quick review on the design. I am also not sure what the output is supposed to be for the
--mode nativemode.I'm new to Go and the surrounding ecosystem, so I might be overlooking better ways to implement this.