Try a temporary directory if the user cache fails #1800

skitt · 2022-02-10T13:48:39Z

Since commit 58c17f6 ("addr.Suggest should lock a file instead of
memory"), pkg/internal/testing/addr/manager.go’s init() function tries
to create a directory using either os.UserCacheDir() or os.TempDir(),
whichever succeeds first. In many build environments, $HOME is
non-empty but points to an unusable directory; in such cases,
os.UserCacheDir() returns an unusable directory, which causes init()
to panic.

This changes init() to first try os.UserCacheDir(), including creating
the desired directory; if that fails, the whole operation is tried
again with os.TempDir().

Fixes: #1799
Signed-off-by: Stephen Kitt skitt@redhat.com

k8s-ci-robot · 2022-02-10T13:48:46Z

Welcome @skitt!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-02-10T13:48:47Z

Hi @skitt. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

vincepri · 2022-02-10T15:52:51Z

/ok-to-test

alvaroaleman · 2022-02-16T03:37:02Z

pkg/internal/testing/addr/manager.go

 	if err != nil {
-		baseDir = os.TempDir()
+		// Either we didn't get a cache directory, or we can't use it
+		cacheDir, err = tryCacheInDir(os.TempDir())


I would actually vote for trying os.TempDir first, because a) it is more likely to succeed and more importantly b) it gets cleared upon reboot, which is not the case for UserCacheDir in most envs I think?

Yup, UserCacheDir doesn’t get cleaned up. I wanted to mostly preserve existing behaviour in this PR; were I to change that, I’d actually argue that UserCacheDir isn’t really appropriate. It is defined as

There is a single base directory relative to which user-specific non-essential (cached) data should be written.

(Plus there’s the general idea in XDG that it’s for “real” users in a desktop environment...)

For lock-style files as used here, it seems to me that the appropriate directories are either /tmp (as used historically, e.g. for X11 locks or SSH agent sockets) or /run (but Go doesn’t wrap that).

Hm yeah, I am really kinda surprised at seeing this here and wondering why that was done in the first place. Maybe os.Tempdir on MacOS is problematic? I don't have a Mac so I can't test that (quick google didn't show something, but that doesn't mean there is no issue). Do you remember @vincepri ?

Other than that this looks fine, except that the commit and pr title should be prefixed with :bug: as documented in the PR template, could you please add that? We also have a check that should verify that, but it appears it is not working.

Tempdir on Mac isn't really reliable so we switched to the CacheDir instead

pkg/internal/testing/addr/manager.go

Since commit 58c17f6 ("addr.Suggest should lock a file instead of memory"), pkg/internal/testing/addr/manager.go’s init() function tries to create a directory using either os.UserCacheDir() or os.TempDir(), whichever succeeds first. In many build environments, $HOME is non-empty but points to an unusable directory; in such cases, os.UserCacheDir() returns an unusable directory, which causes init() to panic. This changes init() to first try os.UserCacheDir(), including creating the desired directory; if that fails, the whole operation is tried again with os.TempDir(). Signed-off-by: Stephen Kitt <skitt@redhat.com>

vincepri

/approve
/lgtm

k8s-ci-robot · 2022-04-05T16:23:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skitt, vincepri

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:

~~pkg/internal/testing/OWNERS~~ [vincepri]

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

skitt · 2022-04-05T16:30:35Z

Thanks!

k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 10, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 10, 2022

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 10, 2022

k8s-ci-robot requested review from mengqiy and varshaprasad96 February 10, 2022 13:48

skitt force-pushed the unusable-cache-dir branch from 8104aa8 to 27df424 Compare February 10, 2022 13:51

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Feb 10, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 10, 2022

alvaroaleman reviewed Feb 16, 2022

View reviewed changes

vincepri reviewed Feb 17, 2022

View reviewed changes

pkg/internal/testing/addr/manager.go Show resolved Hide resolved

skitt mentioned this pull request Mar 11, 2022

Bump the addon-framework to 0.2.0 stolostron/submariner-addon#279

Closed

vincepri mentioned this pull request Apr 4, 2022

🐛 Fallback on temp directory if user cache directory has issues #1849

Closed

skitt force-pushed the unusable-cache-dir branch from 27df424 to a5708a1 Compare April 5, 2022 07:29

vincepri approved these changes Apr 5, 2022

View reviewed changes

k8s-ci-robot assigned vincepri Apr 5, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2022

k8s-ci-robot merged commit adc9fa9 into kubernetes-sigs:master Apr 5, 2022

k8s-ci-robot added this to the v0.10.x milestone Apr 5, 2022

skitt deleted the unusable-cache-dir branch April 5, 2022 16:30

tpantelis mentioned this pull request Jun 10, 2022

Bump open-cluster-management.io/addon-framework from 0.1.0 to 0.3.0 stolostron/submariner-addon#389

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try a temporary directory if the user cache fails #1800

Try a temporary directory if the user cache fails #1800

skitt commented Feb 10, 2022

k8s-ci-robot commented Feb 10, 2022

k8s-ci-robot commented Feb 10, 2022

vincepri commented Feb 10, 2022

alvaroaleman Feb 16, 2022

skitt Feb 16, 2022

alvaroaleman Feb 16, 2022

vincepri Feb 17, 2022

vincepri left a comment

k8s-ci-robot commented Apr 5, 2022

skitt commented Apr 5, 2022

Try a temporary directory if the user cache fails #1800

Try a temporary directory if the user cache fails #1800

Conversation

skitt commented Feb 10, 2022

k8s-ci-robot commented Feb 10, 2022

k8s-ci-robot commented Feb 10, 2022

vincepri commented Feb 10, 2022

alvaroaleman Feb 16, 2022

Choose a reason for hiding this comment

skitt Feb 16, 2022

Choose a reason for hiding this comment

alvaroaleman Feb 16, 2022

Choose a reason for hiding this comment

vincepri Feb 17, 2022

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 5, 2022

skitt commented Apr 5, 2022