package e2e tests in rpm #22395

edsantiago · 2024-04-16T19:17:33Z

Allow other parties to run e2e tests against an rpm-installed podman.

Much trickier than I'd predicted. Split into three commits. Please review those separately for your sanity.

Judgment call: I'm shoehorning these into the existing podman-tests rpm which until now has only had system tests. If there's any objection, or any strong argument for breaking out yet another new subpackage, please speak now.

e2e tests are now included in the podman-tests rpm

openshift-ci · 2024-04-16T19:17:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago

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~~ [edsantiago]

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

packit-as-a-service · 2024-04-16T19:24:24Z

Ephemeral COPR build failed. @containers/packit-build please check.

packit-as-a-service · 2024-04-16T19:24:25Z

Ephemeral COPR build failed. @containers/packit-build please check.

edsantiago

HTH

edsantiago · 2024-04-16T19:19:28Z

test/e2e/build_test.go

-		// Verify that id is correct
-		inspect := podmanTest.Podman([]string{"inspect", string(id)})
-		inspect.WaitWithDefaultTimeout()
-		data := inspect.InspectImageJSON()
-		Expect("sha256:" + data[0].ID).To(Equal(string(id)))


Judgment call: I see no reason to test this

edsantiago · 2024-04-16T19:20:23Z

test/e2e/build_test.go

-			podmanTest.StopRemoteService()
-			podmanTest.StartRemoteService()


Judgment call: I have no idea why this was necessary, and I deem it pointless. Nuked.

edsantiago · 2024-04-16T19:20:51Z

test/e2e/build_test.go

-			podmanTest.StopRemoteService()
-			podmanTest.StartRemoteService()
-		} else {
-			Skip("Only valid at remote test")


Judgment call: I see no reason to test this only in remote.

edsantiago · 2024-04-16T19:24:07Z

test/e2e/common_test.go

-	cwd, _ := os.Getwd()
-	INTEGRATION_ROOT = filepath.Join(cwd, "../../")


Any reason not to nuke this now-empty function?

no good to remove, and thanks to ab29ff2 it only needs to happen in one place.

edsantiago · 2024-04-16T19:24:43Z

test/e2e/common_test.go

@@ -1424,10 +1416,6 @@ func CopyDirectory(srcDir, dest string) error {
 			}
 		}

-		if err := os.Lchown(destPath, int(stat.Uid), int(stat.Gid)); err != nil {


Doesn't work rootless. I suspect this was a copy-pasteism.

edsantiago · 2024-04-16T19:25:50Z

test/e2e/login_logout_test.go

@@ -48,11 +48,10 @@ var _ = Describe("Podman login and logout", func() {

 		testImg = strings.Join([]string{server, "test-alpine"}, "/")

-		certDirPath = filepath.Join(os.Getenv("HOME"), ".config/containers/certs.d", server)


Yuk. Let's try not to do this.

edsantiago · 2024-04-16T19:27:33Z

test/e2e/login_logout_test.go

@@ -61,7 +60,7 @@ var _ = Describe("Podman login and logout", func() {
 			"-e", strings.Join([]string{"REGISTRY_HTTP_ADDR=0.0.0.0", strconv.Itoa(port)}, ":"), "--name", "registry", "-v",
 			strings.Join([]string{authPath, "/auth:Z"}, ":"), "-e", "REGISTRY_AUTH=htpasswd", "-e",
 			"REGISTRY_AUTH_HTPASSWD_REALM=Registry Realm", "-e", "REGISTRY_AUTH_HTPASSWD_PATH=/auth/htpasswd",
-			"-v", strings.Join([]string{certPath, "/certs:Z"}, ":"), "-e", "REGISTRY_HTTP_TLS_CERTIFICATE=/certs/domain.crt",
+			"-v", strings.Join([]string{certDirPath, "/certs:Z"}, ":"), "-e", "REGISTRY_HTTP_TLS_CERTIFICATE=/certs/domain.crt",


For ease of review: this was an oops. certPath is in cwd, which in rpm is /usr/share/podman/tests/e2e, which is not writable by rootless and cannot be relabeled. The intention here was obviously to volume mount the copy, it just sort of never happened that way and nobody ever noticed.

edsantiago · 2024-04-16T19:28:45Z

test/e2e/play_kube_test.go

@@ -4093,7 +4093,7 @@ o: {{ .Options.o }}`})
 	It("persistentVolumeClaim with source", func() {
 		fileName := "data"
 		expectedFileContent := "Test"
-		tarFilePath := filepath.Join(os.TempDir(), "podmanVolumeSource.tgz")
+		tarFilePath := filepath.Join(podmanTest.TempDir, "podmanVolumeSource.tgz")


Yuk. This was creating a file in $TMPDIR and never cleaning it up. Not just littering: this prevented running rootless after running as root.

edsantiago · 2024-04-16T19:29:53Z

test/e2e/run_volume_test.go

@@ -625,7 +625,7 @@ VOLUME /test/`, ALPINE)

 		session = podmanTest.Podman([]string{"run", "--rm", "-v", ".:/app:O", ALPINE, "ls", "/app"})
 		session.WaitWithDefaultTimeout()
-		Expect(session.OutputToString()).To(ContainSubstring(filepath.Base(CurrentSpecReport().FileName())))
+		Expect(session.OutputToString()).To(ContainSubstring(" quadlet "))


rpm package includes a compiled ginkgo binary, but no source files. I could or(source or binary), but this seems good enough.

edsantiago · 2024-04-16T20:17:45Z

Blue-robot failures look real:

+ ./test/tools/build/ginkgo build test/e2e
Failed to compile e2e:

go build github.com/containers/storage/pkg/devicemapper:
# pkg-config --cflags  -- devmapper
Package devmapper was not found in the pkg-config search path.
Perhaps you should add the directory containing `devmapper.pc'
to the PKG_CONFIG_PATH environment variable
Package 'devmapper', required by 'virtual:world', not found
pkg-config: exit status 1
# github.com/mattn/go-sqlite3
../../vendor/github.com/mattn/go-sqlite3/sqlite3.go:85:1: warning: â€˜_sqlite3_execâ€™ defined but not used [-Wunused-function]
   85 | _sqlite3_exec(sqlite3* db, const char* pcmd, long long* rowid, long long* changes)
      | ^~~~~~~~~~~~~

ginkgo build failed

...but it's not something I'm going to look into today.

Luap99 · 2024-04-17T12:47:32Z

test/e2e/common_test.go

-	cwd, _ := os.Getwd()
-	INTEGRATION_ROOT = filepath.Join(cwd, "../../")


This is breaking parallel runs, if you touch these things you should be familiar with how the parallel ginkgo nodes work.
The first function is only run on node 1 while the second one is run on all nodes, so yes this duplication is indeed required here.

One example 5eb99a0

Luap99 · 2024-04-17T12:48:25Z

test/e2e/common_test.go

-	cwd, _ := os.Getwd()
-	INTEGRATION_ROOT = filepath.Join(cwd, "../../")


no good to remove, and thanks to ab29ff2 it only needs to happen in one place.

Luap99 · 2024-04-17T12:51:24Z

test/e2e/common_test.go

+	INTEGRATION_ROOT = os.Getenv("PODMAN_INTEGRATION_ROOT")
+	if INTEGRATION_ROOT == "" {
+		cwd, _ := os.Getwd()
+		INTEGRATION_ROOT = filepath.Join(cwd, "../")
+	}


this should be set once per node in the second function of SynchronizedBeforeSuite() instead of doing the same logic for every single test

I totally agree, except, shouldn't that apply to all the settings here in this block also? It's not likely that $PODMAN_REMOTE_BINARY, QUADLET, OCI_RUNTIME, etc will change? May I leave that for a future cleanup?

I think it is better to leave this for a future cleanup

Luap99

Why do we have to distribute these e2e test pre-compiled, I have strong concerns that this will just end up a big maintenance headache. Why exactly must this be an rpm? What is preventing them from checking out the source and using the make target to run the tests?

Luap99 · 2024-04-17T12:58:56Z

rpm/podman.spec

+# The compiled set of ginkgo tests, and a helper script
+install -d -p %{buildroot}/%{_datadir}/%{name}/test/e2e
+cp test/e2e/e2e.test hack/podman-registry %{buildroot}/%{_datadir}/%{name}/test/e2e
+
+# Files and subdirectories used by those tests
+for testfiles in certs deny.json policy.json redhat_sigstore.yaml registries.conf; do
+    cp -pav test/$testfiles %{buildroot}/%{_datadir}/%{name}/test/e2e/
+done
+for subdir in build cdi config quadlet sign testdata; do
+    cp -pav test/e2e/$subdir %{buildroot}/%{_datadir}/%{name}/test/e2e/
+done


All these spec changes look rather unmaintainable to me. Adding a new file dependencies in different locations will break this easily, sure it shouldn't happen often but there is no way to guarantee that it keeps working.

Luap99 · 2024-04-17T13:02:33Z

rpm/podman.spec

+# Needed for podman-registry script
+export PATH=\$PATH:\$PODMAN_INTEGRATION_ROOT
+
+exec ./e2e.test --ginkgo.trace --ginkgo.v "\$@"


without proper build tags this has low changes of success, or maybe these need to bet set at compile time already?

The build tags are set at compile time; I've just pushed a change that should take care of that. I did not understand that yesterday.

Luap99 · 2024-04-17T13:03:34Z

Blue-robot failures look real:

+ ./test/tools/build/ginkgo build test/e2e
Failed to compile e2e:

go build github.com/containers/storage/pkg/devicemapper:
# pkg-config --cflags  -- devmapper
Package devmapper was not found in the pkg-config search path.
Perhaps you should add the directory containing `devmapper.pc'
to the PKG_CONFIG_PATH environment variable
Package 'devmapper', required by 'virtual:world', not found
pkg-config: exit status 1
# github.com/mattn/go-sqlite3
../../vendor/github.com/mattn/go-sqlite3/sqlite3.go:85:1: warning: â€˜_sqlite3_execâ€™ defined but not used [-Wunused-function]
   85 | _sqlite3_exec(sqlite3* db, const char* pcmd, long long* rowid, long long* changes)
      | ^~~~~~~~~~~~~

ginkgo build failed

...but it's not something I'm going to look into today.

MAke sure you are using the proper build tags, devmapper should not be used.

edsantiago · 2024-04-17T21:01:25Z

Why package these? That's the big question. I'm not completely sold on it; I just want to see if it can be done. And if it can, offer it to the FuSa people and see if they find it useful.

I do believe there is value in test packages that track released packages. For instance, two years from now someone is tracking down a failure in 5.2, installs test rpm on latest Fedora, it fails, that could point to a different component (something else got upgraded, maybe systemd or kernel). With an rpm-maintained test suite, it can be easier to bisect the responsible component. I'm a big fan of bundling tests with builds.

What I really hate is bundling a binary without sources. Yuk. That makes it much harder, probably impossible, for a future maintainer to instrument failing tests. I don't see this as a fixable problem, because Go compilers change so frequently and beyond our control.

Thank you for your feedback!

Luap99 · 2024-04-18T09:55:29Z

I do believe there is value in test packages that track released packages. For instance, two years from now someone is tracking down a failure in 5.2, installs test rpm on latest Fedora, it fails, that could point to a different component (something else got upgraded, maybe systemd or kernel). With an rpm-maintained test suite, it can be easier to bisect the responsible component. I'm a big fan of bundling tests with builds.

Fair but I guess this is where my disconnect is, for me as upstream developer I can just as well checkout v5.2 branch/tag and run the suite that way.

edsantiago · 2024-04-22T19:24:05Z

rpm-build jobs succeeded. I chased the rabbit down to a page with .repo files, set up the rawhide repo on a VM, and ran:

# dnf install podman-tests podman-remote slirp4netns
...
# /usr/share/podman/test/e2e/run-tests &> /var/tmp/e2e-tests.root.01.log
# echo $?
0    <---- yay!

edsantiago · 2024-04-22T20:21:13Z

Rootless:

# loginctl enable-linger fedora
# su - fedora
$ /usr/share/podman/test/e2e/run-tests &> /var/tmp/e2e-tests.rootless.01.log
$ echo $?
1
...failed the expected three tests, which I choose not to bother with right now:
Summarizing 3 Failures:
  [FAIL] podman system connection sshd and API services required [It] add ssh:// socket path using connection heuristic
  /builddir/build/BUILD/podman-5.1.0-dev/test/e2e/system_connection_test.go:350
  [FAIL] Podman run with --cgroup-parent [It] no --cgroup-parent
  /builddir/build/BUILD/podman-5.1.0-dev/test/e2e/run_cgroup_parent_test.go:45
  [FAIL] Podman systemd [It] podman run container with systemd PID1
  /builddir/build/BUILD/podman-5.1.0-dev/test/e2e/systemd_test.go:115

ypu · 2024-04-26T06:27:37Z

rpm/podman.spec

+# The executables we're testing are the ones delivered in RPM
+export PODMAN_BINARY=%{_bindir}/%{name}
+export PODMAN_REMOTE_BINARY=%{_bindir}/%{name}-remote
+export QUADLET_BINARY=%{_libexecdir}/%{name}/quadlet


Do we consider the parameters that related with different releases? Just like the runtime(runc/crun) in different releases? In our rpm testing we also setup NETWORK_BACKEND and OCI_RUNTIME for it.

Those envariables are evaluated at runtime, so yes, they should work as expected.

ypu · 2024-04-26T06:38:25Z

This is very helpful for user to run e2e related tests without setup a compile env especially for some env that leak of related packages and resources. But if we want to make sure all tests are passed, there still lots of work to do.

edsantiago · 2024-04-29T21:11:42Z

@containers/podman-maintainers I think this is ready

[EDIT: this is impossible to review in toto. Please be sure to review commit-by-commit]

Purpose: allow other parties (specifically, FuSa team) to run them. Exercise turned out to be much more complicated than expected, so I've broken it into three parts. Part 1: refactor INTEGRATION_ROOT - allow it to be obtained from environment variable. - remove triplicate definition - redefine it to point one directory level below where it was. This allows cleaning up every instance where it was used, removing a now-unnecessary "test" directory. Purpose of this cleanup is allowing e2e tests to be run from outside the git source tree. The next two commits will move closer toward that goal. Signed-off-by: Ed Santiago <santiago@redhat.com>

Part 2: specfile work: - add tests/e2e to existing podman-tests package - build a static ginkgo test binary - copy files needed by tests (registries, certs, build dirs) - create a run-tests script, because otherwise it'd be impossible for humans to figure out - fix tests that were assuming github source tree layout This commit allows e2e tests to pass as root. Signed-off-by: Ed Santiago <santiago@redhat.com>

Part 3: get tests passing. Mostly by fixing tests that assume that cwd is writable. I, um, have taken some liberties in fixing a couple of broken tests. Not all tests pass rootless. In particular, these three still fail: Podman run with --cgroup-parent [It] no --cgroup-parent Podman systemd [It] podman run container with systemd PID1 -- both with cgroup errors podman system connection sshd and API services required [It] add ssh:// socket path using connection heuristic -- assumes too much about rootless user's ssh setup I call this good enough. Signed-off-by: Ed Santiago <santiago@redhat.com>

openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Apr 16, 2024

openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Apr 16, 2024

edsantiago commented Apr 16, 2024

View reviewed changes

edsantiago force-pushed the package-e2e branch from 264f4f0 to b58a963 Compare April 16, 2024 19:42

edsantiago marked this pull request as draft April 16, 2024 21:10

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 16, 2024

edsantiago force-pushed the package-e2e branch 2 times, most recently from d3b9c75 to 9d25e9e Compare April 16, 2024 23:25

Luap99 reviewed Apr 17, 2024

View reviewed changes

edsantiago force-pushed the package-e2e branch from 2bdc67a to eec2320 Compare April 17, 2024 20:51

edsantiago force-pushed the package-e2e branch 3 times, most recently from 2489f41 to 184e201 Compare April 22, 2024 14:35

ypu reviewed Apr 26, 2024

View reviewed changes

edsantiago force-pushed the package-e2e branch 2 times, most recently from fbb7760 to 445ed42 Compare April 29, 2024 15:15

edsantiago marked this pull request as ready for review April 29, 2024 21:11

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2024

edsantiago added 3 commits May 8, 2024 18:41

edsantiago force-pushed the package-e2e branch from 445ed42 to f17b6de Compare May 9, 2024 00:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

package e2e tests in rpm #22395

package e2e tests in rpm #22395

edsantiago commented Apr 16, 2024 •

edited

openshift-ci bot commented Apr 16, 2024

packit-as-a-service bot commented Apr 16, 2024

packit-as-a-service bot commented Apr 16, 2024

edsantiago left a comment

edsantiago Apr 16, 2024

edsantiago Apr 16, 2024

edsantiago Apr 16, 2024

edsantiago Apr 16, 2024

Luap99 Apr 17, 2024

edsantiago Apr 16, 2024

edsantiago Apr 16, 2024

edsantiago Apr 16, 2024

edsantiago Apr 16, 2024

edsantiago Apr 16, 2024

edsantiago commented Apr 16, 2024

Luap99 Apr 17, 2024

Luap99 Apr 17, 2024

Luap99 Apr 17, 2024

edsantiago Apr 17, 2024

Luap99 Apr 18, 2024

Luap99 left a comment

Luap99 Apr 17, 2024

Luap99 Apr 17, 2024

edsantiago Apr 17, 2024

Luap99 commented Apr 17, 2024

edsantiago commented Apr 17, 2024

Luap99 commented Apr 18, 2024

edsantiago commented Apr 22, 2024

edsantiago commented Apr 22, 2024

ypu Apr 26, 2024

edsantiago Apr 29, 2024

ypu commented Apr 26, 2024

edsantiago commented Apr 29, 2024 •

edited

		podmanTest.StopRemoteService()
		podmanTest.StartRemoteService()

		cwd, _ := os.Getwd()
		INTEGRATION_ROOT = filepath.Join(cwd, "../../")

		@@ -48,11 +48,10 @@ var _ = Describe("Podman login and logout", func() {

		testImg = strings.Join([]string{server, "test-alpine"}, "/")

		certDirPath = filepath.Join(os.Getenv("HOME"), ".config/containers/certs.d", server)

package e2e tests in rpm #22395

Are you sure you want to change the base?

package e2e tests in rpm #22395

Conversation

edsantiago commented Apr 16, 2024 • edited

openshift-ci bot commented Apr 16, 2024

packit-as-a-service bot commented Apr 16, 2024

packit-as-a-service bot commented Apr 16, 2024

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 commented Apr 17, 2024

edsantiago commented Apr 17, 2024

Luap99 commented Apr 18, 2024

edsantiago commented Apr 22, 2024

edsantiago commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypu commented Apr 26, 2024

edsantiago commented Apr 29, 2024 • edited

edsantiago commented Apr 16, 2024 •

edited

edsantiago commented Apr 29, 2024 •

edited