fix unit test using strings.Contains

[gabibeyer]

The Expect function does not return a result of True or False
depending on the value of the first instance, but instead requires
a comparison using ".To(", so change to use ".To(ContainSubsring("

Signed-off-by: gabi beyer [email protected]

[openshift-ci-robot]

Hi @gabibeyer. Thanks for your PR.

I'm waiting for a containers or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mheon]

/approve
/ok-to-test

Changes LGTM

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabibeyer, mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhatdan]

/lgtm
/hold
Feel free to hold cancel, when the tests pass.

[TomSweeneyRedHat]

LGTM, thanks @gabibeyer

[gabibeyer]

My pleasure! I found a few of the same type of bug in other tests, would you like me to add them to this PR, or open a new one?

[TomSweeneyRedHat]

I'd vote for a separate PR personally, but it's a six of one, half dozen of another type of decision. If you need to touch up this one for whatever reason, then feel free to add them here.

[gabibeyer]

Thank you @TomSweeneyRedHat

@baude I think this test may not be working as originally intended, or the mounts are not being cleaned up correctly. Is it possible the conmon process doing the cleanup may need more time between the stop call and reading the mount output?

[mheon]

I'm actually dealing with a very similar case in #3931 - my solution was https://github.com/containers/libpod/blob/951ecb3e816ed12dee7e3c148393777fce10e6a0/test/e2e/run_volume_test.go#L230-L238

Basically, I force a manual run of podman container cleanup after podman stop. The cleanup process normally runs in the background, but we can spawn our own copy of it to force cleanup to occur.

[rhatdan]

I suggest you do a

		stop := podmanTest.Podman([]string{"stop", cid})
		stop.WaitWithDefaultTimeout()
		Expect(stop.ExitCode()).To(Equal(0))

[gabibeyer]

Thank you @mheon, I'll try that out. Would that defeat the purpose of having a 'test mount cleanup', if it needs to be forced?

[mheon]

@gabibeyer Somewhat... but at the same time, I don't think we can necessarily avoid it. There's no easy way to verify that podman container cleanup completed in the background, except polling the mounts to see if cleanup happened yet?

Alternatively - we could replace podman stop with podman rm --force - that includes cleanup as part of it, so it's guaranteed to have run by the time rm --force is done. Arguable also defeats part of the test, though.

[rhatdan]

Wouldn't podman wait be better it should wait to get an exit code and then you know that the container has stopped.

[mheon]

Doesn't guarantee the cleanup process ran, though - Conmon only spawns that after the container is stopped.

[gabibeyer]

@mheon is it expected for a running container to automatically mount its rootfs to a location for the host to be read? Or does it require the mount call, like the other mount tests? I think this test is doing the former, and possibly that is why it is failing, but I'm not confident on the process.

[gabibeyer]

Also, thank you for the detailed response, that makes sense!

[gabibeyer]

Oh, maybe this test failure is because rootless uses vfs instead of overlay?

[mheon]

You know, I don't think we even expect this test to work on rootless.

@giuseppe We don't expect anything in mounts from fuse-overlay, right?

@gabibeyer Sorry for the delay, but I think you can skip this if rootless

[giuseppe]

@giuseppe We don't expect anything in mounts from fuse-overlay, right?

yes correct, there is nothing mounted in the host mount namespace.

You can find the fuse-overlayfs mount once you do podman unshare and join the user namespace+mount namespace used by the rootless containers

[mheon]

These test failures are actually my fault - rebase to master and it should be fixed

[rhatdan]

@gabibeyer Please rebase and repush and we can get this merged. We want to start testing a release on Tomorrow.

[mheon]

/hold cancel
/lgtm