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

Fill in HyperV runtime spec field if shim options asks for it #1388

Merged
merged 1 commit into from
May 6, 2022

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented May 6, 2022

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.

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 <dcanter@microsoft.com>
Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

@dcantah dcantah merged commit 18f4761 into microsoft:master May 6, 2022
// options if we can set this ourselves.
if shimOpts.SandboxIsolation == runhcsopts.Options_HYPERVISOR {
if spec.Windows == nil {
spec.Windows = &specs.Windows{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Even for LCOW?

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

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.

4 participants