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

CRD: Expose more options to the users and let the payload take care of the runtime class creation #240

Conversation

fidencio
Copy link
Member

In this PR we're exposing a few new attributes to the users:

  • Debug: Whether to enable debug for the runtime configuretion, which will also properly configure the CRI Engine accordingly
  • DefaultRuntimeClassName: The runtime class name that will be used as the default one, and will become the kata alias.

Together with these 2 options being exposed, we're leaving the runtime class creation to the runtime payloads, as they know better than the Operator what are the possible podOverhead and, in the future, NFD rules to be used.

Right now this is a draft as it depends on content that's not yet part of the Kata Containers payload, and also work that's been ongoing on the Enclave CC side (thanks @mythi for that!). Regardless, I'd love to hear feedback from the other operator maintainers, thus I'm looping them here.

@fidencio fidencio force-pushed the topic/crd-expose-more-options-to-users branch from ff4475d to 5e93c08 Compare August 5, 2023 09:25
@fidencio
Copy link
Member Author

fidencio commented Aug 5, 2023

/test

@fidencio fidencio force-pushed the topic/crd-expose-more-options-to-users branch from 5e93c08 to e1db62c Compare August 5, 2023 09:32
@fidencio
Copy link
Member Author

fidencio commented Aug 5, 2023

/test

@fidencio fidencio force-pushed the topic/crd-expose-more-options-to-users branch from e1db62c to 7feaac9 Compare August 5, 2023 10:03
@fidencio
Copy link
Member Author

fidencio commented Aug 5, 2023

/test

1 similar comment
@fidencio
Copy link
Member Author

fidencio commented Aug 5, 2023

/test

@fidencio fidencio force-pushed the topic/crd-expose-more-options-to-users branch from 7feaac9 to b9ce4fa Compare August 5, 2023 10:24
@fidencio
Copy link
Member Author

fidencio commented Aug 5, 2023

/test

@fidencio fidencio force-pushed the topic/crd-expose-more-options-to-users branch from b9ce4fa to f5748bf Compare August 5, 2023 12:00
This will allow us passing down to the payloads whether it should be
installed and configured to run on Debug mode, easing the life of folks
who want to debug issues with the Operator's paylods.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
@fidencio fidencio force-pushed the topic/crd-expose-more-options-to-users branch from f5748bf to 3c69fc6 Compare August 5, 2023 12:23
@fidencio
Copy link
Member Author

fidencio commented Aug 5, 2023

/test

@fidencio fidencio force-pushed the topic/crd-expose-more-options-to-users branch from 2422aff to 5bdb07f Compare August 5, 2023 13:52
@fidencio
Copy link
Member Author

fidencio commented Aug 5, 2023

/test

@fidencio fidencio marked this pull request as ready for review August 5, 2023 14:24
@fidencio
Copy link
Member Author

fidencio commented Aug 5, 2023

/cc @jensfr @bpradipt @stevenhorsman

This is ready to be reviewed and it'll be ready to be merged after kata-containers/kata-containers#7549 gets merged on Kata Contauners.

The s390x test is supposed to fail right now the payload I'm using for testing (which includes the not yet merged kata-containers#7549).

The SEV failure is a well known flaky test, the 3rd one, and the other test of that job are passing.

The other jobs should be passing. :-)

This will allow users to define which runtime class they want to be used
as the default "kata" one.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
The payload images will take care of the runtime class creation as they
know better what's needed for their own container runtimes, including
values related to the podOverhead, and also NFD related rules in the
future.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
@@ -18,5 +18,11 @@ patches:
- op: replace
path: /spec/config/runtimeClassNames
value: ["kata-remote"]
- op: add
path: /spec/config/defaultRuntimeClassName
value: "kata-remote"
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this means that if you install the peer-pods CC runtime sample then users would be able to then create peer pods with the kata runtime class now? I guess this might get a bit confusing, so we should stick to documenting using runtimeclass of kata-remote still?

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 can simply drop it, and avoid the confusion entirely.

Would that work better for you?

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 just dropped, so we don't have any change in the current behaviour

Copy link
Member

Choose a reason for hiding this comment

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

I'm just wondering if others have opinions on us using the default runtimeclass name as I can imagine that someone like me might associate kata with kata-qemu and then in this set-up think they are creating a local hypervisor, rather than peer pod instance? It's probably a wider kata doc/UX think to think about.

Copy link
Member Author

Choose a reason for hiding this comment

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

With what I kept here, at least for this PR, is following exactly what was done as part of the operator till Today.
For kata-containers x86_64 payload, the default is QEMU (kata-qemu).
For kata-containers s390x payload, the default is QEMU (kata-qemu)
The case for the remote hypervisor was the only one I was not sure on how to proceed.

In any case, I think that leaving the default as it's always been is the way to go, as long as we give the opportunity to the users to adapt it whatever is their preference.

Let's already adapt the samples, as those can be easily used as part
of the CI.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
@fidencio fidencio force-pushed the topic/crd-expose-more-options-to-users branch from 5bdb07f to 85e9b7c Compare August 7, 2023 10:25
@fidencio
Copy link
Member Author

fidencio commented Aug 7, 2023

/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. Thanks

Copy link
Member

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

@fidencio
Copy link
Member Author

fidencio commented Aug 7, 2023

As in the previous run of the CI we had CLH and QEMU TDX tests passing, and there were zero changes related to them, I'm going ahead and merging this one.

@fidencio fidencio merged commit b282054 into confidential-containers:main Aug 7, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants