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

Hyper-V: Use Default Switch vSwitch by default #18

Merged
merged 2 commits into from
Sep 10, 2019

Conversation

laozc
Copy link

@laozc laozc commented Jul 17, 2019

This PR uses Default Switch as a fallback if the specified target is not found or not specified.

@afbjorklund
Copy link

Please open a PR upstream (https://github.com/docker/machine) as well, thanks.

@afbjorklund
Copy link

Thanks, docker#4732

Copy link

@blueelvis blueelvis left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution :) . Please look at the review comments.

drivers/hyperv/hyperv.go Outdated Show resolved Hide resolved
drivers/hyperv/hyperv.go Outdated Show resolved Hide resolved
@blueelvis
Copy link

@afbjorklund - Can we merge this once the comments are addressed? I will then change our package source to this for Hyper-V. Sounds good?

@blueelvis
Copy link

@laozc - The changes look good!

@afbjorklund - This looks good. Please merge it.

@tstromberg - If this is merged, should I switch the packages to point to this repository instead? We need to get testing infra working for this as well if we are to move to this repository going forward...

@tstromberg
Copy link

@blueelvis - Yes, we should update minikube's dependency pinning to make use of this feature. It's a huge win for users.

For minikube, I'm happy to test this ad-hoc for now while we work out the reliability issues around our Hyper-V machines (tests are now active but only succeed once per reboot). If you are asking about hooking the machine-drivers testing infrastructure to our machines, that isn't currently planned and would require some discussion.

@blueelvis
Copy link

@afbjorklund @tstromberg - Please merge this PR into the repo and I will configure our repository to use this instead of the main machine repo.

Regarding testing infrastructure, can't we have at least somewhat temporary mechanism to do it via Travis?

@tstromberg
Copy link

@blueelvis - Yes, getting Travis support for this repository would be entirely reasonable.

Copy link
Member

@gbraad gbraad left a comment

Choose a reason for hiding this comment

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

The switch should be identified by the ID and not using any string, else the Switch will not be located on localized platforms.

@gbraad
Copy link
Member

gbraad commented Sep 3, 2019

In older versions of Windows 10 the name Default Switch is localized and it will only properly work when the ID is used to get the switch. This ID never changes.

	hypervDefaultVirtualSwitchName = "Default Switch"
	hypervDefaultVirtualSwitchId   = "c08cb7b8-9b3c-408e-8e30-5e16a3aeb444"

When the localization issue got resolved I can't remember, but for fallback in Minishift and CRC we keep using the ID to get the vSwitch.

@afbjorklund
Copy link

Getting some mixed messages here, should it be merged or does it need further additions ?

As far as I know this repository doesn't even do releases, so I'm not sure if it will do checks ?

@tstromberg
Copy link

@laozc - Thanks for the update! It looks good, but would like @gbraad to give his blessing.

@gbraad
Copy link
Member

gbraad commented Sep 10, 2019

I would suggest to merge this, but add modifications when the mention issues show up. We have made a strong cut-off for CRC (the successor to Minishift) so we prevent this issue. But since Minikube does not have any version checks, you might end up people trying this on the Fall Creator's update and unable to select the correct virtual switch.

@gbraad gbraad merged commit 21bd2f5 into machine-drivers:master Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants