-
Notifications
You must be signed in to change notification settings - Fork 611
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
Add Windows HyperV container mode #1848
Add Windows HyperV container mode #1848
Conversation
9af4997
to
504dd91
Compare
Code changes look fine to me. I was expecting we would need to turn on Line 33 in 27b1fef
@dcantah could you take a look at the test to see if that is the right check? |
We should be fine without 1.7 as we're manually toggling the hyper-v field. You'd need 1.7 for Hyper-V through CRI however 😬 |
Cc @kevpar |
504dd91
to
f08eb5f
Compare
f08eb5f
to
6af7881
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Signed-off-by: Dan Ardelean <dardelean@cloudbasesolutions.com>
6af7881
to
7ad50eb
Compare
I had a merge conflict with #1880 and I had to rebase. |
isolation, err := cmd.Flags().GetString("isolation") | ||
if err != nil { | ||
return nil, err | ||
} | ||
switch isolation { | ||
case "hyperv": | ||
if memStr != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there defaults (are those in the runtime config?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related, if no default was supplied I'm forgetting if the HCS v1 behavior was to expose the hosts cpu and memory size to the VM (if anyone wants to try this, otherwise I can report back tonight). With this change if you didn't supply any values the VM would have 2 cores and a similar low amount of ram. Could change this in a follow up though
lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Signed-off-by: Dan Ardelean dardelean@cloudbasesolutions.com
Implements 519