fix: make tests work on fedora #269

Al-Pragliola · 2024-08-14T10:19:32Z

fixes #268

Description

I set Privileged to true to test containers because SELinux prevented containers from reading metadata from the conn_config.pb file, which caused the container to use the in-memory database that does not persist data instead of the SQLite one, causing all tests to fail.

I also improved podman detection on systems that do not require a machine by adding a check to the DOCKER_HOST environment variable.

It's worth noting that I tried other ways to get it to work, both disabling Ryuk and trying to set different SELinux policies by following the testcontainers podman guide, but none of them worked.

How Has This Been Tested?

I tested this on my local machine by running make test

Merge criteria:

The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

tarilabs

thanks @Al-Pragliola for as well including documentation notes!

/lgtm

and planning to try it out locally asap to cross-check!
also if anyone else has chance to try it out on their system in the meantime :)

tarilabs · 2024-08-19T08:18:14Z

internal/testutils/test_container_utils.go

@@ -164,5 +166,5 @@ func tryDetectPodmanRunning() bool {
 	if err != nil {
 		return false
 	}
-	return info.Host.MachineState == "Running"
+	return info.Host.MachineState == "Running" || strings.Contains(os.Getenv("DOCKER_HOST"), "podman.sock")


I believe worth noting that this addition leverage a podman configuration which is not "docker compatibility".

The reason for the OR is on Linux systems that do not need a machine to run containers.

tarilabs · 2024-08-19T09:31:36Z

I'm hitting a rosetta error: unhandled auxillary vector type 28 and unable to double-check this PR locally atm.

btw @lampajr you were on Fedora too, did you needed this changes? 🤔 or maybe with docker engine it was not required ?

isinyaaa

LGTM! but those instructions should apply to any OS in my experience.

Except of course for starting the socket, but that could be a footnote IMO, should be common sense on Linux.

isinyaaa · 2024-08-19T13:53:52Z

CONTRIBUTING.md

+export DOCKER_HOST=unix://$XDG_RUNTIME_DIR/podman/podman.sock
+export TESTCONTAINERS_RYUK_PRIVILEGED=true


Actually, I have to do the same things to get podman working with testcontainers on MacOS, but I usually export

DOCKER_HOST=unix://$(podman machine inspect --format '{{.ConnectionInfo.PodmanSocket.Path}}')

I wonder if it resolves to the same thing on Linux 👀

tarilabs · 2024-08-20T12:06:39Z

fwiw works for me on Mac OSX back again with:

TESTCONTAINERS_RYUK_DISABLED=true make test-nocache

but not with

TESTCONTAINERS_RYUK_PRIVILEGED=true make test-nocache

but I'm also convinced is unrelated to this PR, confirming

/lgtm

lampajr · 2024-08-21T09:07:37Z

btw @lampajr you were on Fedora too, did you needed this changes? 🤔 or maybe with docker engine it was not required ?

Yeah exactly I am on Fedora and I can confirm that with docker I did not have to add these changes (at least so far, I can give it another try in a week when I'll come back)

lampajr · 2024-08-27T12:29:22Z

Yeah exactly I am on Fedora and I can confirm that with docker I did not have to add these changes (at least so far, I can give it another try in a week when I'll come back)

I can confirm this, with Docker none of those exports is required.

fwiw works for me on Mac OSX back again with:
TESTCONTAINERS_RYUK_DISABLED=true make test-nocache
but not with
TESTCONTAINERS_RYUK_PRIVILEGED=true make test-nocache
but I'm also convinced is unrelated to this PR, confirming

/lgtm

I just gave it a try and I can confirm the same results in my Fedora40: using TESTCONTAINERS_RYUK_DISABLED=true make test-nocache it works but using TESTCONTAINERS_RYUK_PRIVILEGED=true it does not.

I think it might be worth to mention it in the CONTRIBUTING.md as users might experience the same issues (given that I don't know why it is working differently)

tarilabs · 2024-08-27T12:34:30Z

CONTRIBUTING.md

+
+```sh
+export DOCKER_HOST=unix://$XDG_RUNTIME_DIR/podman/podman.sock
+export TESTCONTAINERS_RYUK_PRIVILEGED=true


@Al-Pragliola can you kindly double-check if it's about TESTCONTAINERS_RYUK_PRIVILEGED or TESTCONTAINERS_RYUK_DISABLED, please?

see also: #269 (comment)

I removed the export to privileged, I may have had an unclean state when I tested the feature, it works now just with the change I made in the code and the export to DOCKER_HOST.

btw it works with either of them set to true or false

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

tarilabs

thank you for re-checking @Al-Pragliola and for this doc and DX improvement!

/lgtm
/approve

google-oss-prow · 2024-09-11T17:24:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: isinyaaa, tarilabs

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

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

google-oss-prow bot requested review from andreyvelich and Tomcli August 14, 2024 10:19

google-oss-prow bot added the size/M label Aug 14, 2024

Al-Pragliola added 2 commits August 14, 2024 12:20

fix: make tests work on fedora

650eece

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

chore: add more info about contributing on fedora

c21d526

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

Al-Pragliola force-pushed the fix/make-tests-work-on-fedora branch from 65f06f9 to c21d526 Compare August 14, 2024 10:20

google-oss-prow bot added size/S and removed size/M labels Aug 14, 2024

tarilabs reviewed Aug 14, 2024

View reviewed changes

google-oss-prow bot assigned tarilabs Aug 14, 2024

google-oss-prow bot added the lgtm label Aug 14, 2024

tarilabs reviewed Aug 19, 2024

View reviewed changes

isinyaaa approved these changes Aug 19, 2024

View reviewed changes

tarilabs reviewed Aug 27, 2024

View reviewed changes

fix: remove export to ryuk privileged as not necessary

50c6fab

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

google-oss-prow bot removed the lgtm label Sep 11, 2024

github-actions bot added the Area/Go REST server label Sep 11, 2024

tarilabs approved these changes Sep 11, 2024

View reviewed changes

google-oss-prow bot added the lgtm label Sep 11, 2024

google-oss-prow bot added the approved label Sep 11, 2024

google-oss-prow bot merged commit 9e024d2 into kubeflow:main Sep 11, 2024
15 checks passed

tarilabs mentioned this pull request Sep 12, 2024

periodic sync upstream KF to midstream ODH opendatahub-io/model-registry#120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make tests work on fedora #269

fix: make tests work on fedora #269

Al-Pragliola commented Aug 14, 2024

tarilabs left a comment

tarilabs Aug 19, 2024

Al-Pragliola Sep 11, 2024

tarilabs commented Aug 19, 2024

isinyaaa left a comment •

edited

Loading

isinyaaa Aug 19, 2024

tarilabs commented Aug 20, 2024

lampajr commented Aug 21, 2024 •

edited

Loading

lampajr commented Aug 27, 2024

tarilabs Aug 27, 2024

Al-Pragliola Sep 11, 2024

Al-Pragliola Sep 11, 2024

tarilabs left a comment

google-oss-prow bot commented Sep 11, 2024

		export DOCKER_HOST=unix://$XDG_RUNTIME_DIR/podman/podman.sock
		export TESTCONTAINERS_RYUK_PRIVILEGED=true

fix: make tests work on fedora #269

fix: make tests work on fedora #269

Conversation

Al-Pragliola commented Aug 14, 2024

Description

How Has This Been Tested?

Merge criteria:

tarilabs left a comment

Choose a reason for hiding this comment

tarilabs Aug 19, 2024

Choose a reason for hiding this comment

Al-Pragliola Sep 11, 2024

Choose a reason for hiding this comment

tarilabs commented Aug 19, 2024

isinyaaa left a comment • edited Loading

Choose a reason for hiding this comment

isinyaaa Aug 19, 2024

Choose a reason for hiding this comment

tarilabs commented Aug 20, 2024

lampajr commented Aug 21, 2024 • edited Loading

lampajr commented Aug 27, 2024

tarilabs Aug 27, 2024

Choose a reason for hiding this comment

Al-Pragliola Sep 11, 2024

Choose a reason for hiding this comment

Al-Pragliola Sep 11, 2024

Choose a reason for hiding this comment

tarilabs left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Sep 11, 2024

isinyaaa left a comment •

edited

Loading

lampajr commented Aug 21, 2024 •

edited

Loading