-
Notifications
You must be signed in to change notification settings - Fork 259
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
Fill in HyperV runtime spec field if shim options asks for it #1388
Conversation
As part of the work to get WCOW-Hypervisor working for the upstream Containerd CRI plugin, parse our shims SandboxIsolation field here and set HyperV if it hasn't been set. This avoids us needind one place we need to parse our shim specific options in Containerd which is always nice. Signed-off-by: Daniel Canter <[email protected]>
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.
LGTM
// options if we can set this ourselves. | ||
if shimOpts.SandboxIsolation == runhcsopts.Options_HYPERVISOR { | ||
if spec.Windows == nil { | ||
spec.Windows = &specs.Windows{} |
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.
Even for LCOW?
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.
Looks like old code did do that lol. i dont remember why
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.
We identify LCOW by the Linux section being filled in as well as hyperv. Which kinda makes sense in my head hahah
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.
Oh, I am dumb. yes I know why this works lol. I for some reason thought that spec.Windows.HyperV
was outside of spec.Windows
and you didnt need the Windows
part to add the HyperV
part. Just ignore me.
As part of the work to get WCOW-Hypervisor working for the upstream Containerd CRI plugin, parse our shims SandboxIsolation field here and set the HyperV runtime spec field if it's set to the HYPERVISOR option. This avoids us needing to parse our shim specific options in upstream Containerd which is always a plus.