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

testutil: Add HyperVSupported check #2062

Merged
merged 1 commit into from
May 3, 2023

Conversation

dcantah
Copy link
Member

@dcantah dcantah commented Mar 1, 2023

Will probably be a growing amount of hyper-v tests, and they'll all need this preamble to check if virt/nested virt is turned on.

@AkihiroSuda
Copy link
Member

Is this still a draft?

@dcantah dcantah force-pushed the hyperv-util branch 2 times, most recently from 824ee1a to eecbfc5 Compare March 15, 2023 09:05
@dcantah dcantah marked this pull request as ready for review March 15, 2023 09:05
@dcantah
Copy link
Member Author

dcantah commented Mar 15, 2023

Sorry for delay @AkihiroSuda, I think this is okay to un-draft. Will keep an eye on CI result, I'll need to look into the CI failure

@dcantah dcantah force-pushed the hyperv-util branch 2 times, most recently from 335727a to 0d9c243 Compare March 21, 2023 19:45
@dcantah
Copy link
Member Author

dcantah commented Mar 21, 2023

Figured out why this is failing -_-, there's a process isolated test that checks that the output of container inspect --mode native doesn't contain the string hyperv. However, the output of container inspect --mode native includes the environment variables for the oci hooks (although these don't do anything on windows) which seems to inherit the env vars of the host. Cirrus sets a bunch of environment variables with the commit and pr messages, where I've used, you guessed it, the string hyperv in my commit message.

"CIRRUS_COMMIT_MESSAGE=testutil: Add HyperVSupported check\n\nWill probably be a growing amount of hyperv tests, and they'll all need this preamble to check if virt/nested virt is turned on.\r\n\r\nDraft for now until I can run the suite on my windows box also. Been meaning to do this anyways",

@dcantah dcantah marked this pull request as draft March 21, 2023 20:52
@dardelean
Copy link
Contributor

The sooner this lands, the better for me. Thanks.

@dcantah
Copy link
Member Author

dcantah commented Mar 23, 2023

Been trying to find time, do you want to try and find a good testcase for the test(s) described above that check for the "hyperv" string? This should work after that, frankly this should work right now as is if I removed hyperv from my commit message 😭

@dcantah
Copy link
Member Author

dcantah commented Mar 23, 2023

Perhaps we can check this in and I'll make an issue for us to fix the hyperv string check. CI should pass now that I changed hyperv to hyper-v

@dcantah
Copy link
Member Author

dcantah commented Mar 23, 2023

Failures in the CI for 22.04 should be irrelevant

@dcantah
Copy link
Member Author

dcantah commented Mar 23, 2023

I've made #2126 to track fixing the process isolated test

@dardelean
Copy link
Contributor

Can this be merged? Would help me in #2145. Thanks.

@AkihiroSuda
Copy link
Member

CI can't be restarted, could you rebase and push again?

@AkihiroSuda AkihiroSuda added this to the v1.4.0 (tentative) milestone May 2, 2023
@AkihiroSuda AkihiroSuda added the platform/Windows/Non-WSL2 Microsoft Windows (non-WSL2) label May 2, 2023
Will probably be a growing amount of hyper-v tests, and they'll all need
this preamble to check if virt/nested virt is turned on.

Signed-off-by: Danny Canter <danny@dcantah.dev>
@dcantah
Copy link
Member Author

dcantah commented May 2, 2023

@AkihiroSuda Done

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 070e24a into containerd:main May 3, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform/Windows/Non-WSL2 Microsoft Windows (non-WSL2)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants