Skip to content

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 <[email protected]>
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