e2e: Allow ImageCacheDir to be changed with an environment variable #17089

sstosh · 2023-01-12T05:39:40Z

We can change the ImageCacheDir with TMPDIR.
Reduce dependency on /tmp for e2e tests.
When a rootless user, the cache directory is failed to
remove due to a permission. Therefore, we use
podman unshare rm to remove the cache.

Signed-off-by: Toshiki Sonoda [email protected]

Does this PR introduce a user-facing change?

None

openshift-ci · 2023-01-12T05:39:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sstosh
Once this PR has been reviewed and has the lgtm label, please assign flouthoc for approval by writing /assign @flouthoc in a comment. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

vrothberg

Thanks! Changes LGTM with one question.

@edsantiago PTAL

vrothberg · 2023-01-17T12:21:09Z

test/e2e/save_test.go

@@ -188,6 +188,7 @@ var _ = Describe("Podman save", func() {

 		cmd = exec.Command("cp", "sign/key.gpg", "/tmp/key.gpg")
 		Expect(cmd.Run()).To(Succeed())
+		defer os.Remove("/tmp/key.gpg")


Should that also be under cachedir?

We need to fix the keyPath value from /tmp/key.gpg to cachePath("key.gpg") in e2e/sign/policy.json.

podman/test/e2e/sign/policy.json

Line 13 in a702d44

"keyPath": "/tmp/key.gpg"

I haven't come up with a best way, so I remove the key.gpg by defer.

edsantiago

I kind of maybe see what problem this is trying to solve, but I think this might not be the best way to do it. Blocking because of two concerns.

edsantiago · 2023-01-17T13:29:11Z

test/e2e/common_test.go

@@ -844,9 +847,27 @@ func populateCache(podman *PodmanTestIntegration) {
 	fmt.Printf("-----------------------------\n")
 }

+func cachePath(path string) string {


I'm failing to see how this will use $PODMAN_TEST_IMAGE_CACHE_DIR. Should it?

I'm bit a confused.
ImageCacheDir is hard corded (/tmp/podman/imagecachedir).

podman/test/e2e/config.go

Line 16 in a702d44

ImageCacheDir = "/tmp/podman/imagecachedir"

On the other hand, $PODMAN_TEST_IMAGE_CACHE_DIR also exists and used.

podman/test/e2e/common_test.go

Lines 344 to 357 in a702d44

func imageTarPath(image string) string {

cacheDir := os.Getenv("PODMAN_TEST_IMAGE_CACHE_DIR")

if cacheDir == "" {

cacheDir = os.Getenv("TMPDIR")

if cacheDir == "" {

cacheDir = "/tmp"

}

}

// e.g., registry.com/fubar:latest -> registry.com-fubar-latest.tar

imageCacheName := strings.ReplaceAll(strings.ReplaceAll(image, ":", "-"), "/", "-") + ".tar"

return filepath.Join(cacheDir, imageCacheName)

}

I don't know if it is necessary to separate ImageCacheDir and $PODMAN_TEST_IMAGE_CACHE_DIR.
If unnecessary, I think that $PODMAN_TEST_IMAGE_CACHE_DIR can be removed.

Sorry for taking so long to respond. I think you've hit on the main issue: $PODMAN_TEST_IMAGE_CACHE_DIR has nothing whatsoever to do with ImageCacheDir. It's a confusing similarity. I would suggest leaving the existing $PODMAN_TEST_IMAGE_CACHE_DIR code untouched. The reason that exists is that ginkgo tests pull enormous images, and that takes half an hour on my slow network. I need to cache those locally.

The rest of your PR, though, still needs touchup. Here's what I suggest:

leave imageTarPath() untouched

Remove lines 845-849, so cachePath() always returns $TMPDIR/arg (defaulting to /tmp) (as you do now)

Bonus points if you can get rid of "" requirement, so callers can do cachePath() instead of cachePath(""). The former is more readable.

in removeCache(), always remove cachePath("imagedir"). Right now you don't remove it if $TMPDIR is set, and that seems like a logic bug.

bonus points if you find a way to remove using podman unshare or some magic namespace code, because that code always always always fails rootless.

Thanks again for your patience and for your contribution.

I checked that the cache directory is failed to remove due to a permission.
When a rootless user, remove the cache with podman unshare rm -rf <cacheDir>.

podman/test/e2e/common_test.go

Lines 564 to 574 in 63c6a1a

// Nuke tempdir

if isRootless() {

cmd := exec.Command(p.PodmanBinary, "unshare", "rm", "-rf", p.TempDir)

if err := cmd.Run(); err != nil {

fmt.Fprintf(os.Stderr, "%v\n", err)

}

} else {

if err := os.RemoveAll(p.TempDir); err != nil {

fmt.Printf("%q\n", err)

}

}

podman/test/e2e/common_test.go

Lines 873 to 884 in 63c6a1a

// Remove cache dirs

imageDir := cachePath("imagedir")

if isRootless() {

cmd := exec.Command(p.PodmanBinary, "unshare", "rm", "-rf", imageDir)

if err := cmd.Run(); err != nil {

fmt.Fprintf(os.Stderr, "%v\n", err)

}

} else {

if err := os.RemoveAll(imageDir); err != nil {

fmt.Printf("%q\n", err)

}

}

edsantiago · 2023-01-17T13:30:19Z

test/e2e/common_test.go

+		imageCacheDir = os.Getenv("PODMAN_TEST_IMAGE_CACHE_DIR")
+	}
+
+	if err := os.RemoveAll(imageCacheDir); err != nil {


I'm not too happy with the possibility of someone (inadvertently or not) setting PODMAN_TEST_IMAGE_CACHE_DIR=/tmp (or /etc or even /) and having that be wiped

I changed a condition for remove a cache directory.

edsantiago · 2023-01-19T22:52:16Z

test/e2e/create_test.go

+		nonExistentFile := cachePath("nonexistent")
+		session := podmanTest.Podman([]string{"create", "--authfile", nonExistentFile, "--name=foo", ALPINE})


I think this, and the other /tmp/nonexistent changes, are overkill.

I reverted cachePath("noexistent") to /tmp/noexistent.

- We can change the ImageCacheDir with `TMPDIR`. - Reduce dependency on `/tmp` for e2e tests. - When a rootless user, the cache directory is failed to remove due to a permission. Therefore, we use `podman unshare rm` to remove the cache. Signed-off-by: Toshiki Sonoda <[email protected]>

edsantiago · 2023-01-23T19:40:40Z

I'm really sorry, but I can't review this. I've tried three times, Friday and today, and it's too convoluted. This is not your fault -- our test setup overloads too many things in and too many names (image, cache, tmp) -- but the result is, I can't understand this, can't confirm that it has no unintended consequences, and can't approve it.

May I request that you break this down into smaller PRs? Something like:

1) temp/scratch (not cache) files

Add a new function, tempFilePath() or scratchFilePath() or anything that does not say "cache":

func tempFilePath(path string) string {
	return filepath.Join(os.TempDir(), path)
        // even better would be creating a one-time `os.MkdirTemp("", "podman-e2e-tests.*")
        // but that would again lead to a too-complicated PR. Once the code is cleaned up,
        // we can look into that as a future improvement.
}

...and use that new function in checkpoint_test.go and push_test.go. I find it really important to get rid of the word "cache" in these contexts.

(In case someone suggests os.CreateTemp(): no go. That creates the file; we want a path to an uncreated file. The randomization/security/race aspects of CreateTemp() are not important in this test context).

2) image cache

Leave ImageCacheDir as a global, but make it start as nil, and initialize it at runtime to filepath.join(os.TempDir(), "imagecachedir"), Then, in mount_rootless_test.go, do the substring check against ImageCacheDir.

That leaves the delete code, which still scares the heck out of me, but I think with these two changes the delete code will be easier to understand.

Alternatively, perhaps someone else in @containers/podman-maintainers can understand and review and approve this PR as it is. But again, I'm sorry, I cannot.

sstosh · 2023-01-24T01:39:34Z

Thank you for your review and suggestions.
I'll open the other three PRs later such as below:

e2e: Add tempFilePath() for refactoring
e2e: Refactoring ImageCacheDir
e2e: Remove the cache with "podman unshare rm" when a rootless user

This PR become longer, so I'll close it once.

openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 12, 2023

sstosh force-pushed the e2e-image-cache branch 7 times, most recently from 70f62ed to b197be5 Compare January 16, 2023 00:26

vrothberg reviewed Jan 17, 2023

View reviewed changes

edsantiago suggested changes Jan 17, 2023

View reviewed changes

sstosh force-pushed the e2e-image-cache branch 3 times, most recently from c34b1e9 to 647fc4a Compare January 18, 2023 02:58

edsantiago reviewed Jan 19, 2023

View reviewed changes

sstosh force-pushed the e2e-image-cache branch 4 times, most recently from 3b265fd to ab0e361 Compare January 20, 2023 06:37

sstosh marked this pull request as ready for review January 20, 2023 07:53

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 Jan 20, 2023

sstosh force-pushed the e2e-image-cache branch from ab0e361 to 63c6a1a Compare January 20, 2023 08:02

sstosh closed this Jan 24, 2023

This was referenced Jan 25, 2023

e2e: reduce dependency on /tmp for e2e tests #17208

Merged

e2e: Remove the cache with "podman unshare rm" when a rootless user #17245

Merged

e2e: Avoid hard-coding ImageCacheDir #17243

Merged

sstosh deleted the e2e-image-cache branch January 30, 2023 14:22

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 13, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: Allow ImageCacheDir to be changed with an environment variable #17089

e2e: Allow ImageCacheDir to be changed with an environment variable #17089

sstosh commented Jan 12, 2023 •

edited

Loading

openshift-ci bot commented Jan 12, 2023

vrothberg left a comment

vrothberg Jan 17, 2023

sstosh Jan 18, 2023 •

edited

Loading

edsantiago left a comment

edsantiago Jan 17, 2023

sstosh Jan 18, 2023 •

edited

Loading

edsantiago Jan 19, 2023

sstosh Jan 20, 2023 •

edited

Loading

edsantiago Jan 17, 2023

sstosh Jan 18, 2023

edsantiago Jan 19, 2023

sstosh Jan 20, 2023

edsantiago commented Jan 23, 2023

sstosh commented Jan 24, 2023

	func imageTarPath(image string) string {
	cacheDir := os.Getenv("PODMAN_TEST_IMAGE_CACHE_DIR")
	if cacheDir == "" {
	cacheDir = os.Getenv("TMPDIR")
	if cacheDir == "" {
	cacheDir = "/tmp"
	}
	}

	// e.g., registry.com/fubar:latest -> registry.com-fubar-latest.tar
	imageCacheName := strings.ReplaceAll(strings.ReplaceAll(image, ":", "-"), "/", "-") + ".tar"

	return filepath.Join(cacheDir, imageCacheName)
	}

	// Nuke tempdir
	if isRootless() {
	cmd := exec.Command(p.PodmanBinary, "unshare", "rm", "-rf", p.TempDir)
	if err := cmd.Run(); err != nil {
	fmt.Fprintf(os.Stderr, "%v\n", err)
	}
	} else {
	if err := os.RemoveAll(p.TempDir); err != nil {
	fmt.Printf("%q\n", err)
	}
	}

	// Remove cache dirs
	imageDir := cachePath("imagedir")
	if isRootless() {
	cmd := exec.Command(p.PodmanBinary, "unshare", "rm", "-rf", imageDir)
	if err := cmd.Run(); err != nil {
	fmt.Fprintf(os.Stderr, "%v\n", err)
	}
	} else {
	if err := os.RemoveAll(imageDir); err != nil {
	fmt.Printf("%q\n", err)
	}
	}

		nonExistentFile := cachePath("nonexistent")
		session := podmanTest.Podman([]string{"create", "--authfile", nonExistentFile, "--name=foo", ALPINE})

e2e: Allow ImageCacheDir to be changed with an environment variable #17089

e2e: Allow ImageCacheDir to be changed with an environment variable #17089

Conversation

sstosh commented Jan 12, 2023 • edited Loading

Does this PR introduce a user-facing change?

openshift-ci bot commented Jan 12, 2023

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Jan 17, 2023

Choose a reason for hiding this comment

sstosh Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago Jan 17, 2023

Choose a reason for hiding this comment

sstosh Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

edsantiago Jan 19, 2023

Choose a reason for hiding this comment

sstosh Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

edsantiago Jan 17, 2023

Choose a reason for hiding this comment

sstosh Jan 18, 2023

Choose a reason for hiding this comment

edsantiago Jan 19, 2023

Choose a reason for hiding this comment

sstosh Jan 20, 2023

Choose a reason for hiding this comment

edsantiago commented Jan 23, 2023

1) temp/scratch (not cache) files

2) image cache

sstosh commented Jan 24, 2023

sstosh commented Jan 12, 2023 •

edited

Loading

sstosh Jan 18, 2023 •

edited

Loading

sstosh Jan 18, 2023 •

edited

Loading

sstosh Jan 20, 2023 •

edited

Loading