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

Fix Hyper-V check in late clone spec comparisons #1400

Merged
merged 1 commit into from
May 17, 2022

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented May 17, 2022

In the function that handles checking if the cloned containers spec retrieved from the registry matches the current spec, it was comparing the hyperv field on the runtime spec using the != operator. This won't work as the hyperv field is a pointer so it would just be comparing the address and not the contents.

This became an 'issue' as we set the hyperv field now if in our shim options the SandboxIsolation field is set to the HYPERVISOR option. This doesn't have much of an effect being set for containers that are going to be launched IN the UVM, so this is mostly just a bug that was surfaced from a field that didn't use to be set. This was causing the late clone spec comparisons to fail at the hyperv field, and thus the late clone tests.

This additionally changes to errors.New instead of fmt.Errorf where there was no formatting in the error.

In the function that handles checking if the cloned containers spec
retrieved from the registry matches the current spec, it was comparing
the hyperv field on the runtime spec using the != operator which won't
work as the hyperv field is a pointer so it would just be comparing the
addr. Swap to reflect.DeepEqual.

This became an 'issue' as we set the hyperv field now if in our shim
options SandboxIsolation is set to the HYPERVISOR option. This doesn't
have much of an effect being set for containers that are going to be launched
IN the UVM, so this is mostly just a bug that was surfaced from a field
that didn't use to be set.

This additionally changes to errors.New instead of fmt.Errorf where
there was no formatting in the error.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah dcantah requested a review from a team as a code owner May 17, 2022 12:55
Copy link
Contributor

@helsaawy helsaawy left a comment

Choose a reason for hiding this comment

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

I feel like a having an auto-generated WindowsHyperV.Equal would make everything easier ...

@helsaawy helsaawy self-assigned this May 17, 2022
@dcantah dcantah merged commit c6aa049 into microsoft:master May 17, 2022
anmaxvl added a commit that referenced this pull request Feb 7, 2023
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
In the function that handles checking if the cloned containers spec
retrieved from the registry matches the current spec, it was comparing
the hyperv field on the runtime spec using the != operator which won't
work as the hyperv field is a pointer so it would just be comparing the
addr. Swap to reflect.DeepEqual.

This became an 'issue' as we set the hyperv field now if in our shim
options SandboxIsolation is set to the HYPERVISOR option. This doesn't
have much of an effect being set for containers that are going to be launched
IN the UVM, so this is mostly just a bug that was surfaced from a field
that didn't use to be set.

This additionally changes to errors.New instead of fmt.Errorf where
there was no formatting in the error.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
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.

3 participants