Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change tmpDir when running e2e localmachine test on macOS #22362

Merged
merged 1 commit into from
May 16, 2024
Merged

Change tmpDir when running e2e localmachine test on macOS #22362

merged 1 commit into from
May 16, 2024

Conversation

tnk4on
Copy link
Contributor

@tnk4on tnk4on commented Apr 12, 2024

Fix #22360

Does this PR introduce a user-facing change?

Fixed an error starting Podman machine when running e2e localmachine tests on macOS.

The fixes are only to the e2e test file and Doc, no additional tests.

@packit-as-a-service Packit-as-a-Service
Copy link

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

1 similar comment
@packit-as-a-service Packit-as-a-Service
Copy link

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

ashley-cui
ashley-cui reviewed Apr 15, 2024
View reviewed changes
Copy link
Member

@ashley-cui ashley-cui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tnk4on!
I believe our macOS CI sets TMPDIR, so it makes sense that the tests are failing. I think instead, we should only set tmpdir on macOS to /private/tmp if TMPDIR is not set, rather than overriding it.

@tnk4on
Copy link
Contributor Author

tnk4on commented Apr 16, 2024

@ashley-cui
I think the macOS CI Fail is another cause as it was happening with other PRs. The podman machine actually started without timeout until halfway through.

I added code to check $TMPDIR. Please check.

ashley-cui
ashley-cui reviewed Apr 16, 2024
View reviewed changes
pkg/machine/e2e/machine_test.go Outdated

func getTmpDir(value string) string {
if runtime.GOOS == "darwin" {
if len(value) >= 22 || value == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would still prefer that we only change the tmpdir if it is not set. If the user/runner of tests sets a path len > 22, I think we should fail with an error, and not secretly change the path without notice.

macOS tests are passing now!

@tnk4on tnk4on force-pushed the e2e-macos-tmpdir branch 3 times, most recently from 6e1d2b4 to 4699dec Compare April 16, 2024 15:35
@ashley-cui
Copy link
Member

LGTM, restarted flaking tests, thanks!

@ashley-cui
Copy link
Member

Please rebase to pick up the bud test fail, thanks!

@containers/podman-maintainers PTAL

@tnk4on tnk4on force-pushed the e2e-macos-tmpdir branch 2 times, most recently from 37c4cf7 to 4fcfc3f Compare April 30, 2024 13:58
@tnk4on
Copy link
Contributor Author

tnk4on commented May 1, 2024

@ashley-cui I added some code. Please check.

If TMPDIR=“”, it cannot connect to gvproxy.sock.

  > Enter [It] Volume ops - /Users/shtanaka/dev2/podman/pkg/machine/e2e/basic_test.go:73 @ 05/01/24 09:53:43.085
  /Users/shtanaka/dev2/podman/bin/darwin/podman machine init --disk-size 11 --image /private/tmp/podman_test234588004/podman-machine-daily.aarch64.applehv.raw --now 7f8b5007b0ea
  Machine init complete
  Starting machine "7f8b5007b0ea"
  Error: unable to connect to "gvproxy" socket at "podman/7f8b5007b0ea-gvproxy.sock"
  [FAILED] Expected
      <int>: 125
  to match exit code:
      <int>: 0
  In [It] at: /Users/shtanaka/dev2/podman/pkg/machine/e2e/basic_test.go:95 @ 05/01/24 09:54:26.226

so I changed it to return an error.

Also, if TMPDIR is a directory that is not mounted on the podman machine, the bind mount test (e.g. "Volume ops". at basic_test.go) will return an error.
Therefore, added check if TMPDIR is included in Machine.Volumes.

ashley-cui
ashley-cui reviewed May 1, 2024
View reviewed changes
Copy link
Member

@ashley-cui ashley-cui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than two small comments, LGTM. Thanks!

pkg/machine/e2e/machine_test.go Outdated
case runtime.GOOS != "darwin":
tmpDir = value
case len(value) >= 22:
return "", errors.New("path length should be less than 22 characters")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, so the user knows what path it is:

Suggested change
return "", errors.New("path length should be less than 22 characters")
return "", errors.New("$TMPDIR path length should be less than 22 characters")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small change and fixed

pkg/machine/e2e/machine_test.go
case len(value) >= 22:
return "", errors.New("path length should be less than 22 characters")
case value == "":
return "", errors.New("TMPDIR cannot be empty. Set to directory mounted on podman machine (e.g. /private/tmp)")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with setting TMPDIR automatically to /private/tmp in this instance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be another Issue.

% export TMPDIR=""
% podman machine init --now
Looking up Podman Machine image at quay.io/podman/machine-os:5.1 to create VM
Getting image source signatures
Copying blob 5ab0b6fcee8a done   | 
Copying config 44136fa355 done   | 
Writing manifest to image destination
5ab0b6fcee8aba3f303ad47032c77ffa6894a25f8bc3645b3567860cd7dddf39
Extracting compressed file: podman-machine-default-arm64.raw: done  
Machine init complete
Starting machine "podman-machine-default"
Error: unable to connect to "gvproxy" socket at "podman/podman-machine-default-gvproxy.sock"

The following code will set an empty path if TMPDIR=“”. So the gvproxy path will be inappropriate.
If unset TMPDIR is executed, /tmp is set and it works.

podman/pkg/machine/env/dir_darwin.go

Lines 5 to 11 in c9644eb

func getRuntimeDir() (string, error) {
tmpDir, ok := os.LookupEnv("TMPDIR")
if !ok {
tmpDir = "/tmp"
}
return tmpDir, nil
}

@tnk4on
Change tmpDir for macOS
590fec7 
Signed-off-by: Shion Tanaka <shtanaka@redhat.com>
@ashley-cui
Copy link
Member

LGTM @containers/podman-maintainers PTAL

@rhatdan
Copy link
Member

rhatdan commented May 16, 2024

/approve
/lgtm
Thanks @tnk4on

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, tnk4on

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 1afeb13 into containers:main May 16, 2024
91 checks passed
@tnk4on tnk4on deleted the e2e-macos-tmpdir branch May 16, 2024 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashley-cui ashley-cui ashley-cui left review comments

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. machine release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[mac]e2e:Podman machine cannot run if default $TMPDIR
3 participants
@tnk4on @ashley-cui @rhatdan