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

ccruntime: Improvements on the runtime classes we deploy #420

Merged

Conversation

fidencio
Copy link
Member

Please, see each commit message for more description.

Right now, for some reason, we're not setting TEEs to use guest-pull,
although we do that on the Kata Containers side.

This is done in order to ensure those would also work when testing using
CRI-O, as done for peer-pods.

Signed-off-by: Fabiano Fidêncio <[email protected]>
As folks may want to give it a try on non-TEE environments.

Signed-off-by: Fabiano Fidêncio <[email protected]>
As those are non-TEE and work better with whatever is the default on the
Kata Containers side (virtio-fs, in this case).

Signed-off-by: Fabiano Fidêncio <[email protected]>
Copy link
Member

@beraldoleal beraldoleal 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

@@ -27,13 +27,13 @@ patches:
pulltype: ""
- name: "kata-qemu-tdx"
snapshotter: "nydus"
pulltype: ""
pulltype: "guest-pull"
Copy link
Member

Choose a reason for hiding this comment

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

Hi @fidencio !

We don't test TDX/SEV/SNP with CRI-O so I left this empty on purpose. But it was a mistake, we better have it set to guest-pull anyway, hence, thanks for the fix!

BTW, do you plan to test TDX + CRI-O? There is this #409 and I asked on last CI meeting if platform folks would like to test with CRI-O (I didn't get a 'yes' nor 'no'...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now we're struggling to have machines available to test.
This, however, helps me when doing tests for Red Hat related stuff. ;-)

Copy link
Member

Choose a reason for hiding this comment

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

Got it. yeah, it would be trick to use same machine to containerd and cri-o and keep switching the configurations....

@@ -25,6 +25,9 @@ patches:
- name: "kata-qemu"
snapshotter: "nydus"
pulltype: ""
- name: "kata-qemu-coco-dev"
Copy link
Member

Choose a reason for hiding this comment

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

Hey, this was on my TODO list, thanks!

I plan to create a job to test that runtimeclass too: #409 . But let me know if you are already working on that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not working on that, just had to add this for a personal test earlier Today, and shared as I had that done already.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fidencio might I ask why on TEE you want nydus and on non-TEE you don't? Also you mentioned in kata there are the defaults, is it possible to rely on those defaults and not setting it here? (would empty string result in using the default or is this not technically possible?)

Copy link
Member

@wainersm wainersm 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 @fidencio !

Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

Changes look good, works well and who else should know what config to use better than you. So let's get this in.

@ldoktor ldoktor merged commit 97f1db5 into confidential-containers:main Aug 20, 2024
18 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants