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

runtime: fix creation of SEV confidential container on SNP enabled host. #9037

Merged
merged 1 commit into from Feb 8, 2024

Conversation

niteeshkd
Copy link

Fixes the issue #9036 .

@katacontainersbot katacontainersbot added the size/tiny Smallest and simplest task label Feb 6, 2024
@niteeshkd
Copy link
Author

Copy link
Member

@ryansavino ryansavino left a comment

Choose a reason for hiding this comment

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

Ah, thanks Niteesh for fixing this.

Copy link
Contributor

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

Great catch. We're changing a lot of stuff to ConfidentialGuest, but the snpGuest flag should be SevSnpGuest, because ConfidentialGuest is true for both SEV and SNP.

@niteeshkd niteeshkd force-pushed the nd_SevSnpGuest branch 2 times, most recently from 79526f8 to 32bc08f Compare February 6, 2024 17:30
Copy link
Member

@fidencio fidencio left a comment

Choose a reason for hiding this comment

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

@niteeshkd, would you mind to add a commiit message explaining why this is needed?
If this is a regression, mind to mention when we regressed?

This kind of information is crucial for maintainers to navigate through the history of the projects (and bugs, and fixes).

@fidencio
Copy link
Member

fidencio commented Feb 6, 2024

Answering some of my own questions here, it seems that the regression was introduced by de39fb7 (which, if had been merged with a reasonable commit message would have made our lives easier at this point --)). Let's also loop in @zvonkok to the loop to make sure it doesn't break his GPU related work, and if it does break, figure out the best way to solve.

@niteeshkd
Copy link
Author

@niteeshkd, would you mind to add a commiit message explaining why this is needed?

@fidencio updated the commit message.

This is needed to fix the bug which is not allowing to create SEV container
on SNP enabled host anymore. This is a regression that was introduced as
part of the following commit:
kata-containers@de39fb7

Fixes: kata-containers#9036

Signed-off-by: Niteesh Dubey <niteesh@us.ibm.com>
Copy link
Member

@fidencio fidencio left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @niteeshkd!

@niteeshkd
Copy link
Author

Created #9040 to add a test in future.

@ryansavino
Copy link
Member

All required checks are passing on this one. @stevenhorsman @danmihai1 @fidencio We good to merge this?

@danmihai1
Copy link
Contributor

/test

Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

LGTM

@stevenhorsman stevenhorsman merged commit b99f574 into kata-containers:main Feb 8, 2024
286 of 299 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test size/tiny Smallest and simplest task
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

8 participants