list: Don't fail if rootfs doesn't exist #907
Conversation
If it is not possible to determine the owner of the rootfs, do not fail the entire `list` operation, just mark the container owner field with a question mark. Fixes clearcontainers#906. Signed-off-by: James O. D. Hunt <[email protected]>
|
lgtm |
| } | ||
|
|
||
| defer func() { | ||
| testingImpl.ListPodFunc = nil |
There was a problem hiding this comment.
dump question why this is need?
There was a problem hiding this comment.
This resets the virtcontainers mock handler for ListPod(). See https://github.com/containers/virtcontainers/blob/master/pkg/vcMock/mock.go#L91..L99.
By resetting the handler to nil, we guarantee that any other test that invokes any virtcontainers API calls which either directly or indirectly call ListPod() will fail with an error from the virtcontainers mock package. The reason for all this being that some virtcontainers types are private, but the runtime's unit tests need to create particular test scenarios using those private data types. They also need to simulate previous calls to particular sequences of virtcontainers calls. Since the runtime uses the virtcontainers API heavily, we have a problem as we cannot contrive such scenarios using the main virtcontainers API calls (the runtime tests might not even have sufficient privs to run some of the virtcontainers API calls). As such, virtcontainers provides two implementations of its API - the "proper" / official one and a mock one that can be used for testing the caller of the API itself, as here.
| if err == nil { | ||
| owner = fmt.Sprintf("#%v", uid) | ||
| } else { | ||
| owner = "?" |
There was a problem hiding this comment.
This is an error that started to happen recently, I think we should be able always know the owner , so being this a not usual case can you log a warning and probably and log the error to probably know if the file did not exist ?
There was a problem hiding this comment.
Yeah, I'm having second thoughts about whether we should do this now we've confirmed the CI failure (clearcontainers/jenkins#32) is Fedora only.
What do you think @sameo, @grahamwhaley ? FTR, runc is "semi-lenient": it does not validate whether the container rootfs exists as it doesn't really need to care about that:
Nominally, we do care because we use the container rootfs ownership to show who owns the container. However, we should really be employing a strategy similar to runc which sets the container ownership to the owner of the container directory below runc's state directory. But cc-runtime cannot do that as it doesn't know where the virtcontainers root directory is (implementation detail).
As such, this is all slightly academic as cc-runtime list will always show the owner UID 0 currently.
The advantage of not failing on rootfs lookup is that list will show atleast partial information. Maybe we should hold off deciding until we understand the F26 failure further.
There was a problem hiding this comment.
sure, mark it as WIP until we get some further info then OK?
|
kubernetes qa-passed 👍 |
|
Any update on this, I think is safe close it, or merge it but add a warning to our log. |
|
Let's close it for now as it no longer seems to be required. |
If it is not possible to determine the owner of the rootfs, do not fail
the entire
listoperation, just mark the container owner field with aquestion mark.
Fixes #906.
Signed-off-by: James O. D. Hunt [email protected]