Implement machine provider selection #17639

arixmkii · 2023-02-27T09:40:57Z

GetSystemDefaultProvider reworked to accept input parameter with the desired virtualization provider. All callers adjusted to read this from the config before requesting virtualization provider instance.

Additional environment variable CONTAINERS_MACHINE_PROVIDER is supported to override the config for testing purposes.

Motivation for this signature is that it could be later extended to not only using config, but also to accept CLI switches. It could be changed to have config reading inside platform method to remove it from every caller, but this will not significantly reduce code lines in the callers, because this would change the signature of the method in platform to report possible errors and then callers should still check and handle the errors.

[NO NEW TESTS NEEDED]

Does this PR introduce a user-facing change?

Enable Podman Machine provider configuration via config

arixmkii · 2023-02-27T09:41:42Z

NO NEW TESTS NEEDED, because there are no alternative providers ready to be used as of the moment of creating this PR.

arixmkii · 2023-02-27T10:08:20Z

I edited release note, because I realized that this thing is worth mentioning (the config option made itself available with the pull of common, but now it is actually exposed to user). ~~Please update the label.~~ (bot did it 🎉 )

Luap99 · 2023-02-27T14:41:34Z

@baude PTAL

TomSweeneyRedHat · 2023-03-01T23:21:12Z

cmd/podman/machine/platform_windows.go

+	case "WSL":
+		return wsl.GetWSLProvider()
+	case "QEMU":
+		logrus.Info("QEMU provider is work in progress https://github.com/containers/podman/issues/13006. Will use default provider.")


This line is good now, but this is the kind of line that can suffer bit rot in the not-too-distant future.

Agreed. This was ok for a concept, but I'm ok to change this. 👍

Removed QEMU part on Windows, will not be part of this PR. It was included only for demo purposes and now we have HyperV to play this role.

TomSweeneyRedHat · 2023-03-01T23:28:34Z

One small comment and I have restarted the flakey swagger test. Otherwise LGTM.

Luap99

@baude PTAL

cmd/podman/machine/info.go

cmd/podman/machine/platform.go

cmd/podman/machine/platform_windows.go

cmd/podman/machine/platform.go

baude · 2023-03-02T16:57:17Z

cmd/podman/machine/platform_windows.go

+	}
+	provider = strings.ToUpper(strings.TrimSpace(provider))
+	switch provider {
+	case "WSL":


I'm sitting on a PR where I strong typed all providers. My preference is we do something like that for sure. Should I yank that out of my PR and submit it?

I'm sitting on a PR where I strong typed all providers

I have no objections about this. Strong typed is often a better option. If this will be split between common and podman repos it would make things slightly more difficult for external contributors, but I believe that typed version is still better.

Reworked into typed version. I really don't like that I had to implement nullability with a pointer, because there is no Undefined value in the enum. Still thinking about reworking API into 2 separate functions - one taking enum parameter and another one w/o any parameters and then handling nullability in the private function.

baude · 2023-03-02T16:59:42Z

@arixmkii i'm going through a similar activity as you are here in work I am actively doing to add HyperV support. And as a team we discussed this issue and how it related to containers-common, essentially that containers-common is ignorant of "providers" and is only by OS ... where podman now basically needs both.

baude · 2023-03-02T17:02:41Z

Oh and worse with something like the operating system, we also have fcos and fedora. so:

Linux -> QEMU -> FCOS
Windows -> WSL -> Special Fedora
Windows -> QEMU -> ? but assume FCOS
Windows -> HyperV -> FCOS
Apple -> QEMU -> FCOS
Apple -> AppleHV -> ?FCOS?

arixmkii · 2023-03-02T17:18:07Z

Windows -> QEMU -> ? but assume FCOS

Yes it is FCOS.

Oh and worse with something like the operating system, we also have fcos and fedora. so:

And it is even worse. Working on QEMU implementation I had hard times to provide out of the box working defaults for machine init command as they are hardcoded based on OS and WSL an QEMU doesn't share the OS and the default user at least.

arixmkii · 2023-03-02T17:21:15Z

@baude I'm open to withdraw this one (and keep it for my rebuilds only for now) and wait for a solution provided by the Podman team as I see this feature is more suitable for internal team rather than to external community contributors. I'm still open to some code work or needed assistance, but I don't think this should be driven by external people (w/o extensive communications) as this is pretty much integral to the Podman. I did this only, because I believed that work on applehv was either on hold or halt due to the situation with vfkit and this is why I created implementation myself.

arixmkii · 2023-03-20T18:35:16Z

Rebased to latest main and updated. ~~Yet to test it locally (will update this comment, when it's done).~~

Update: tested on Windows build. Worked as expected - it is possible to configure provider using conf file and by overriding it using environment variable.

arixmkii · 2023-03-28T19:08:11Z

@baude Could you take a look if this fits how you see that feature for Hyper-v implementation?

baude · 2023-03-30T18:08:22Z

cmd/podman/machine/platform_windows.go

-	// TODO this needs to be changed back
-	if _, exists := os.LookupEnv("HYPERV"); exists {
-		return hyperv.GetVirtualizationProvider()
+func GetSystemProvider(vmType *machine.VMType) (machine.VirtProvider, error) {


What is the reason for the argument? Is there a case where we need it? or worst case, write it unexported with a exported caller?

Open extension point to support override passed via command line parameter. This is what I had in mind and why it is an exported func. I don't know if there will be a need/desire to have command line parameter, though.

Can remove it, if you see that simplification is more important and the use case is too artificial.

baude · 2023-03-30T18:09:16Z

cmd/podman/machine/platform_windows.go

+	resolvedVMType := machine.QemuVirt
+	if vmType != nil {
+		resolvedVMType = *vmType
+	} else {


in the same thought here, my understanding is that the containers.conf file would dictate the vm type. is that what you remember @Luap99 ?

If the config file is the only source of truth for this setting, this can be simplified.

that was the decision that was made .. i had hoped to also honor environment variables for development and testing. VMTYPE="qemu" ... wwyt about that too?

There is EnvVar support implemented as part of this PR https://github.com/containers/podman/pull/17639/files#diff-1f6113c3d215be90ffaa1c5cc73b7da30d7c6583c49cbae89c4132858478ebf6R25 So, there is an option for development purposes. I just wasn't aware if every configuration should have matching command line parameter override or not.

I will remove extra function from the PR and resubmit.

Parameter removed and related branching eliminated from the function.

arixmkii · 2023-04-01T20:46:15Z

Applied requested changes and rebased to latest main.

baude · 2023-04-23T19:54:43Z

looks much better ... would you be wiling to slap a few logrus.debug's in that shows which provider is being used? else, tyvm!

arixmkii · 2023-04-24T21:04:45Z

@baude Applied this diff to the same commit:

diff --git a/cmd/podman/machine/platform.go b/cmd/podman/machine/platform.go
index 9d0ee1ca7..67b554306 100644
--- a/cmd/podman/machine/platform.go
+++ b/cmd/podman/machine/platform.go
@@ -10,6 +10,7 @@ import (
        "github.com/containers/common/pkg/config"
        "github.com/containers/podman/v4/pkg/machine"
        "github.com/containers/podman/v4/pkg/machine/qemu"
+       "github.com/sirupsen/logrus"
 )
 
 func GetSystemProvider() (machine.VirtProvider, error) {
@@ -26,6 +27,7 @@ func GetSystemProvider() (machine.VirtProvider, error) {
                return nil, err
        }
 
+       logrus.Debugf("Using Podman machine with `%s` virtualization provider", resolvedVMType.String())
        switch resolvedVMType {
        case machine.QemuVirt:
                return qemu.GetVirtualizationProvider(), nil
diff --git a/cmd/podman/machine/platform_windows.go b/cmd/podman/machine/platform_windows.go
index a50f0efd0..88a5a3531 100644
--- a/cmd/podman/machine/platform_windows.go
+++ b/cmd/podman/machine/platform_windows.go
@@ -8,6 +8,7 @@ import (
        "github.com/containers/podman/v4/pkg/machine"
        "github.com/containers/podman/v4/pkg/machine/hyperv"
        "github.com/containers/podman/v4/pkg/machine/wsl"
+       "github.com/sirupsen/logrus"
 )
 
 func GetSystemProvider() (machine.VirtProvider, error) {
@@ -24,6 +25,7 @@ func GetSystemProvider() (machine.VirtProvider, error) {
                return nil, err
        }
 
+       logrus.Debugf("Using Podman machine with `%s` virtualization provider", resolvedVMType.String())
        switch resolvedVMType {
        case machine.WSLVirt:
                return wsl.GetWSLProvider(), nil

baude · 2023-04-24T21:12:00Z

/approve
LGTM

openshift-ci · 2023-04-24T21:12:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arixmkii, 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:

~~OWNERS~~ [baude]

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

GetSystemDefaultProvider reworked to fetch provider value from the config file. Additional environment variable CONTAINERS_MACHINE_PROVIDER is supported to override the config for testing purposes. Signed-off-by: Arthur Sengileyev <arthur.sengileyev@gmail.com>

arixmkii · 2023-04-28T14:52:34Z

Rebased to latest main. Had to change https://github.com/containers/podman/pull/17639/files#diff-f1ce6e45ecdde3df18c58e0a3a779f2736b14bf91edb510e868562588559b7a9L33 I didn't track the origin of this change, but this only worked before the getProvider introduced error possibility (returning error on misconfiguration).

rhatdan · 2023-05-01T11:55:00Z

/lgtm
Thanks @arixmkii

openshift-ci bot added the release-note-none label Feb 27, 2023

arixmkii force-pushed the provider-choice branch from cff7d69 to 80811c7 Compare February 27, 2023 09:51

openshift-ci bot added release-note and removed release-note-none labels Feb 27, 2023

TomSweeneyRedHat reviewed Mar 1, 2023

View reviewed changes

Luap99 requested changes Mar 2, 2023

View reviewed changes

cmd/podman/machine/info.go Outdated Show resolved Hide resolved

cmd/podman/machine/platform.go Outdated Show resolved Hide resolved

cmd/podman/machine/platform_windows.go Outdated Show resolved Hide resolved

cmd/podman/machine/platform.go Outdated Show resolved Hide resolved

baude reviewed Mar 2, 2023

View reviewed changes

arixmkii force-pushed the provider-choice branch from 80811c7 to 761832c Compare March 20, 2023 18:28

arixmkii force-pushed the provider-choice branch 4 times, most recently from 5410cf1 to 77588ee Compare March 20, 2023 20:18

baude reviewed Mar 30, 2023

View reviewed changes

arixmkii force-pushed the provider-choice branch from 77588ee to b76cfe0 Compare April 1, 2023 20:45

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2023

arixmkii force-pushed the provider-choice branch from b76cfe0 to a4490df Compare April 23, 2023 18:41

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2023

Luap99 mentioned this pull request Apr 24, 2023

Add user-mode networking feature to Windows/WSL #18303

Merged

arixmkii force-pushed the provider-choice branch from a4490df to af88ad6 Compare April 24, 2023 21:03

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2023

arixmkii force-pushed the provider-choice branch from af88ad6 to b5ef955 Compare April 28, 2023 14:49

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2023

openshift-ci bot assigned rhatdan May 1, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 1, 2023

openshift-merge-robot merged commit f11ba8d into containers:main May 1, 2023

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 Aug 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 2023

arixmkii deleted the provider-choice branch March 1, 2024 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement machine provider selection #17639

Implement machine provider selection #17639

arixmkii commented Feb 27, 2023 •

edited

Loading

arixmkii commented Feb 27, 2023

arixmkii commented Feb 27, 2023 •

edited

Loading

Luap99 commented Feb 27, 2023

TomSweeneyRedHat Mar 1, 2023

arixmkii Mar 2, 2023

arixmkii Mar 20, 2023

TomSweeneyRedHat commented Mar 1, 2023

Luap99 left a comment

baude Mar 2, 2023

arixmkii Mar 2, 2023

arixmkii Mar 20, 2023

baude commented Mar 2, 2023

baude commented Mar 2, 2023

arixmkii commented Mar 2, 2023

arixmkii commented Mar 2, 2023

arixmkii commented Mar 20, 2023 •

edited

Loading

arixmkii commented Mar 28, 2023

baude Mar 30, 2023

arixmkii Mar 30, 2023

baude Mar 30, 2023

arixmkii Mar 30, 2023

baude Mar 31, 2023

arixmkii Mar 31, 2023

arixmkii Apr 1, 2023

arixmkii commented Apr 1, 2023

baude commented Apr 23, 2023

arixmkii commented Apr 24, 2023

baude commented Apr 24, 2023

openshift-ci bot commented Apr 24, 2023

arixmkii commented Apr 28, 2023

rhatdan commented May 1, 2023

Implement machine provider selection #17639

Implement machine provider selection #17639

Conversation

arixmkii commented Feb 27, 2023 • edited Loading

Does this PR introduce a user-facing change?

arixmkii commented Feb 27, 2023

arixmkii commented Feb 27, 2023 • edited Loading

Luap99 commented Feb 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Mar 1, 2023

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baude commented Mar 2, 2023

baude commented Mar 2, 2023

arixmkii commented Mar 2, 2023

arixmkii commented Mar 2, 2023

arixmkii commented Mar 20, 2023 • edited Loading

arixmkii commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arixmkii commented Apr 1, 2023

baude commented Apr 23, 2023

arixmkii commented Apr 24, 2023

baude commented Apr 24, 2023

openshift-ci bot commented Apr 24, 2023

arixmkii commented Apr 28, 2023

rhatdan commented May 1, 2023

arixmkii commented Feb 27, 2023 •

edited

Loading

arixmkii commented Feb 27, 2023 •

edited

Loading

arixmkii commented Mar 20, 2023 •

edited

Loading