Skip to content
This repository has been archived by the owner on May 6, 2020. It is now read-only.

list: Don't fail if rootfs doesn't exist #907

Closed
wants to merge 1 commit into from
Closed

list: Don't fail if rootfs doesn't exist #907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,13 +323,15 @@ func getContainers(context *cli.Context) ([]fullContainerState, error) {
ociState := oci.StatusToOCIState(container)
staleAssets := getStaleAssets(currentHypervisorDetails, latestHypervisorDetails)

var owner string

uid, err := getDirOwner(container.RootFs)
if err != nil {
return nil, err
if err == nil {
owner = fmt.Sprintf("#%v", uid)
} else {
owner = "?"
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, mark it as WIP until we get some further info then OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wfm...

}

owner := fmt.Sprintf("#%v", uid)

s = append(s, fullContainerState{
containerState: containerState{
Version: ociState.Version,
Expand Down
64 changes: 64 additions & 0 deletions list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,70 @@ func TestListGetContainers(t *testing.T) {
assert.Equal(state, []fullContainerState(nil))
}

func TestListGetContainersMissingRootfs(t *testing.T) {
assert := assert.New(t)

tmpdir, err := ioutil.TempDir(testDir, "")
assert.NoError(err)
defer os.RemoveAll(tmpdir)

pod := &vcMock.Pod{
MockID: testPodID,
}

rootfs := filepath.Join(tmpdir, "rootfs")

container := vc.ContainerStatus{
ID: pod.ID(),
State: vc.State{
State: vc.StateRunning,
},
PID: 1,

// rootfs specified, but it doesn't exist
RootFs: rootfs,
}

testingImpl.ListPodFunc = func() ([]vc.PodStatus, error) {
return []vc.PodStatus{
{
ID: pod.ID(),
ContainersStatus: []vc.ContainerStatus{
container,
},
},
}, nil
}

defer func() {
testingImpl.ListPodFunc = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

dump question why this is need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}()

app := cli.NewApp()
ctx := cli.NewContext(app, nil, nil)
app.Name = "foo"

runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true)

assert.NoError(err)

ctx.App.Metadata = map[string]interface{}{
"runtimeConfig": runtimeConfig,
}

states, err := getContainers(ctx)
assert.NoError(err)

assert.True(len(states) == 1)

state := states[0]

assert.Equal(state.Rootfs, container.RootFs)

// since rootfs doesn't exist
assert.Equal(state.Owner, "?")
}

func TestListGetContainersPodWithoutContainers(t *testing.T) {
assert := assert.New(t)

Expand Down