Support multiple runtimeclasses in kataconfig.status.runtimeClass #344
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @pmores, patch looks straightforward. Thanks! Does it impact upgrading the operator? I believe you said it requires removing the kataconfig and then re-applying? Is that because of the change in the .status section of the crd? |
gkurz
reviewed
Sep 22, 2023
Yes, exactly. Due to the earlier changes in kataconfig.status on this branch, this has likely already been the case. |
This controller can now create multiple RuntimeClasses, for plain kata
and for peer pods. However only the original kata RuntimeClass has ever
been accounted for in KataConfig.status.runtimeClass so far.
This is not merely a cosmetic problem. The controller uses
KataConfig.status.runtimeClass to find out whether there are any pods
using the kata runtime before it removes the runtime from a cluster on
uninstallation. Since the peer pods RuntimeClass was never represented
in the field so far, running peer pods were ignored on uninstallation.
As for implementation, KataConfig.status.runtimeClass is now optional,
not required. This doesn't really have to do with the core idea of this
commit, it's more to work around the fact that kubebuilder doesn't seem
to support array fields with an empty array as the default value.
The removal of KataConfig resource immediate Update() after modifying the
runtimeClass field is also not strictly necessary but still included since
such Update()s in the middle of reconciliation can cause problems on a
subsequent Update() ("object has been modified").
Signed-off-by: Pavel Mores <pmores@redhat.com>
pmores
force-pushed
the
support-multiple-runtimeclasses-in-kataconfig-status
branch
from
0b7aa34
to
3bdd0e7
Compare
gkurz
approved these changes
Sep 22, 2023
| @@ -751,12 +751,8 @@ func (r *KataConfigOpenShiftReconciler) createRuntimeClass(runtimeClassName stri | |||
| } | |||
| } | |||
|
|
|||
| if r.kataConfig.Status.RuntimeClass == "" { | |||
| r.kataConfig.Status.RuntimeClass = runtimeClassName | |||
| err = r.Client.Status().Update(context.TODO(), r.kataConfig) | |||
There was a problem hiding this comment.
I'm fine with this change as it partly addresses https://issues.redhat.com/browse/KATA-2441.
This will need to be documented in the OSC upgrade guidelines. |
pmores
added a commit
to pmores/sandboxed-containers-operator
that referenced
this pull request
Sep 25, 2023
A chunk was unfortunately left out from PR openshift#344 by mistake. We also take this opportunity to split the condition in the daemonset creation error checking to get rid of a long-standing logging annoyance. No error should be logged if getting the DS returns "not found". Signed-off-by: Pavel Mores <pmores@redhat.com>
Merged
pmores
added a commit
that referenced
this pull request
Sep 25, 2023
A chunk was unfortunately left out from PR #344 by mistake. We also take this opportunity to split the condition in the daemonset creation error checking to get rid of a long-standing logging annoyance. No error should be logged if getting the DS returns "not found". Signed-off-by: Pavel Mores <pmores@redhat.com>
snir911
pushed a commit
to snir911/sandboxed-containers-operator
that referenced
this pull request
Oct 11, 2023
A chunk was unfortunately left out from PR openshift#344 by mistake. We also take this opportunity to split the condition in the daemonset creation error checking to get rid of a long-standing logging annoyance. No error should be logged if getting the DS returns "not found". Signed-off-by: Pavel Mores <pmores@redhat.com>
snir911
pushed a commit
to snir911/sandboxed-containers-operator
that referenced
this pull request
Oct 12, 2023
A chunk was unfortunately left out from PR openshift#344 by mistake. We also take this opportunity to split the condition in the daemonset creation error checking to get rid of a long-standing logging annoyance. No error should be logged if getting the DS returns "not found". Signed-off-by: Pavel Mores <pmores@redhat.com>
snir911
pushed a commit
to snir911/sandboxed-containers-operator
that referenced
this pull request
Oct 17, 2023
A chunk was unfortunately left out from PR openshift#344 by mistake. We also take this opportunity to split the condition in the daemonset creation error checking to get rid of a long-standing logging annoyance. No error should be logged if getting the DS returns "not found". Signed-off-by: Pavel Mores <pmores@redhat.com>
gkurz
added a commit
to gkurz/sandboxed-containers-operator
that referenced
this pull request
Dec 7, 2023
PR openshift#344 changed the type of KataConfigStatus::RuntimeClass from string to array of string. This change in the CRD isn't supported and prevents upgrades from an older operator if a KataConfig is present. The new CSV will stay in the `Pending` state forever and the following error is reported by the install plan : message: 'error validating existing CRs against new CRD''s schema for "kataconfigs.kataconfiguration.openshift.io": error validating custom resource against new schema for KataConfig /example-kataconfig: [].status.runtimeClass: Invalid value: "string": status.runtimeClass in body must be of type array: "string"' Simply rename the field to avoid the issue. Signed-off-by: Greg Kurz <groug@kaod.org>
beraldoleal
pushed a commit
to beraldoleal/sandboxed-containers-operator
that referenced
this pull request
Jan 4, 2024
PR openshift#344 changed the type of KataConfigStatus::RuntimeClass from string to array of string. This change in the CRD isn't supported and prevents upgrades from an older operator if a KataConfig is present. The new CSV will stay in the `Pending` state forever and the following error is reported by the install plan : message: 'error validating existing CRs against new CRD''s schema for "kataconfigs.kataconfiguration.openshift.io": error validating custom resource against new schema for KataConfig /example-kataconfig: [].status.runtimeClass: Invalid value: "string": status.runtimeClass in body must be of type array: "string"' Simply rename the field to avoid the issue. Signed-off-by: Greg Kurz <groug@kaod.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This controller can now create multiple RuntimeClasses, for plain kata
and for peer pods. However only the original kata RuntimeClass has ever
been accounted for in KataConfig.status.runtimeClass so far.
This is not merely a cosmetic problem. The controller uses
KataConfig.status.runtimeClass to find out whether there are any pods
using the kata runtime before it removes the runtime from a cluster on
uninstallation. Since the peer pods RuntimeClass was never represented
in the field so far, running peer pods were ignored on uninstallation.
As for implementation, KataConfig.status.runtimeClass is now optional,
not required. This doesn't really have to do with the core idea of this
commit, it's more to work around the fact that kubebuilder doesn't seem
to support array fields with an empty array as the default value.
The removal of KataConfig resource immediate Update() after modifying the
runtimeClass field is also not strictly necessary but still included since
such Update()s in the middle of reconciliation can cause problems on a
subsequent Update() ("object has been modified").
Fixes KATA-2294
[EDIT, greg]
Fixes KATA-2441 # Partially
Signed-off-by: Pavel Mores pmores@redhat.com