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.

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

Prevent multiple machines running on different providers on darwin #25139

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

baude
Copy link
Member

@baude baude commented Jan 27, 2025

Prevent two podman machines running on darwin

As issue #25112 points out, it was possible to start a machine on one of the darwin providers and then switch providers and start another one with a different name. This PR firstly prevents that use which is a forbidden use case.

Secondarily, performed some minor cleanup on the error messages being used so that the error would be specific to this condition.

This bug fix is for darwin only. In the case of Windows, we probably need to answer the question I raised in #24067 first, which is whether we want to stop allowing WSL to run multiple machines.

Fixes #25112

Does this PR introduce a user-facing change?

Fix bug that allowed users to run a machine on both the `applehv` and `libkrun` providers on darwin

@baude baude added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed release-note labels Jan 27, 2025
Copy link
Contributor

openshift-ci bot commented Jan 27, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude

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 Jan 27, 2025
@baude baude added No New Tests Allow PR to proceed without adding regression tests and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 27, 2025
@baude
Copy link
Member Author

baude commented Jan 27, 2025

the no-new-tests may be only temporary ... will decide when I remove the WIP label.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2025
@openshift-ci openshift-ci bot added release-note and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 28, 2025
@baude baude force-pushed the issue25112 branch 5 times, most recently from 7095a15 to 837dab0 Compare January 28, 2025 21:12
@baude
Copy link
Member Author

baude commented Jan 29, 2025

/cherry-pick v5.4

@openshift-cherrypick-robot
Copy link
Collaborator

@baude: once the present PR merges, I will cherry-pick it on top of v5.4 in a new PR and assign it to you.

In response to this:

/cherry-pick v5.4

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-sigs/prow repository.

@mheon
Copy link
Member

mheon commented Jan 29, 2025

LGTM though a test would be lovely

@baude
Copy link
Member Author

baude commented Jan 29, 2025

in looking at the tests, i see no easy way to do this without having a distinct possibility of really dorking up things. to me, the juice isnt worth the work, though if people really want ...

pkg/machine/define/errors.go Outdated Show resolved Hide resolved
pkg/machine/shim/host.go Outdated Show resolved Hide resolved
baude added 2 commits January 29, 2025 12:12
The Kind() exported function is unused in our code; moreover, the function cannot be accurate because in the case of darwin, applehv and libkrun use the same config in the struct and therefore, we cannot identify the provider via that method.

Signed-off-by: Brent Baude <[email protected]>
A function in the reset code does not return an error.  Simply removing the error variable and check for the condition (which was always false or nil)

Signed-off-by: Brent Baude <[email protected]>
As issue containers#25112 points out, it was possible to start a machine on one of the darwin providers and then switch providers and start another one with a different name.  This PR firstly prevents that use which is a forbidden use case.

Secondarily, performed some minor cleanup on the error messages being used so that the error would be specific to this condition.

This bug fix is for darwin only.  In the case of Windows, we probably need to answer the question I raised in containers#24067 first, which is whether we want to stop allowing WSL to run multiple machines.

Fixes containers#25112

Signed-off-by: Brent Baude <[email protected]>
@TomSweeneyRedHat
Copy link
Member

@baude could you change the tite to be a bit more descriptive please? There won't be a whole lot of context in three years about that issue number.

@TomSweeneyRedHat
Copy link
Member

LGTM
once thet test get hip

@baude baude changed the title Issue25112 Prevent multiple machines running on different providers on darwin Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. machine No New Tests Allow PR to proceed without adding regression tests release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Two podman machines can be run at the same time on macos
5 participants