Disable hyperv dynamic memory for hyperv driver

[shahiddev]

Disables hyper-v dynamic memory

Fixes issues #2331, #2326, #1766

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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. I understand the commands that are listed here.

[minikube-bot]

Can one of the admins verify this patch?

[shahiddev]

/assign @jimmidyson

[dlorenc]

@minikube-bot OK to test

[dlorenc]

Hey,

Unfortunately we can't make changes directly to this vendor directory, they'll get overwritten on the next update of our dependencies.

Some ideas:

• Move the shell command to do the disabling up into minikube itself
• Get this fix applied upstream in https://github.com/docker/machine

[shahiddev]

Ah, silly mistake.
OK, let me take another look at this

[shahiddev]

I've got a PR to add support for the flag in Docker Machine merged now.
Does the vendor directory get updated on each release automatically or as part of someones PR?

[shahiddev]

@dlorenc The latest Docker Machine now has my PR merged in it (https://github.com/docker/machine/releases/tag/v0.15.0)
How do I incorporate the updates into Minikube, are the vendor deps updated routinely as part of the release process or do I need to create a PR to do this?

[tstromberg]

I just found this PR and it looks great!

Would it be possible to resolve the conflicts and merge it? I can also recreate it in a new PR, but I would rather let you keep the credit for the change.

Personally, I strongly prefer to disable dynamic memory by default, so that it behaves the same as other VM managers supported by minikube.

[shahiddev]

Hi @tstromberg I had tried to get the Docker Machine changes I made into the minikube project but there was some uncertainty around whether they wanted to keep on using Docker Machine, hence why things went quiet. I'd have been happy to try and surface those docker machine changes if that was settled?

[tstromberg]

shahiddev@ - minikube will continue to use Docker Machine for the foreseeable future.

If this PR still requires a change to docker/machine, you can propose it to the fork we are using: https://github.com/machine-drivers/machine/blob/master/drivers/hyperv/hyperv.go - I'm a maintainer there and would be happy to review your change there.

Please let me know if I can help you in any way to get this merged.

[shahiddev]

I think I managed to get my docker machine changes into the final release of Docker Machine before it went into maintenance so those should be in the fork. I'll try and get this sorted over the coming days depending on how busy Kubecon EU is :)

[tstromberg]

I think I managed to get my docker machine changes into the final release of Docker Machine before it went into maintenance so those should be in the fork. I'll try and get this sorted over the coming days depending on how busy Kubecon EU is :)

Sounds great. Enjoy Barcelona!

[tstromberg]

I happened to be looking at
https://github.com/machine-drivers/machine/blob/master/drivers/hyperv/hyperv.go - and it seems that DisableDynamicMemory is in fact an option.

I suggest that we disable just disable by default, as dynamically sized memory won't ever play well with Kubernetes job scheduling.

[shahiddev]

I will re-open the PR, initially I wanted to reset my fork to the current upstream given the majority of the changes I had in the original PR weren't required and it was nearly a year behind upstream

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shahiddev
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jimmidyson

If they are not already assigned, you can assign the PR to them by writing /assign @jimmidyson in a comment when ready.

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

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

[shahiddev]

I happened to be looking at
https://github.com/machine-drivers/machine/blob/master/drivers/hyperv/hyperv.go - and it seems that DisableDynamicMemory is in fact an option.

I suggest that we disable just disable by default, as dynamically sized memory won't ever play well with Kubernetes job scheduling.

As per your suggestion @tstromberg I opted to keep it simple and just set the disable hyper-v dynamic memory flag I had exposed on Docker Machine (rather than adding an additional command line flag and wiring that up to the flag).

[tstromberg]

Nice! Thank you for making this change!