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.

Sign up for GitHub

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

Jump to bottom

Fix upgrade from OSC 1.4.1 #366

Merged
merged 3 commits into from
Dec 8, 2023
Merged

Fix upgrade from OSC 1.4.1 #366

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63dfcc9
Drop DS requirement on runtime class
gkurz Dec 1, 2023
2d4ce96
Always regenerate KataConfigStatus::KataNodes
gkurz Dec 2, 2023
3e6f490
Rename RuntimeClass field to RuntimeClasses
gkurz Dec 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 0 additions & 4 deletions controllers/openshift_controller.go
View file
Open in desktop
Expand Up @@ -1667,10 +1667,6 @@ const (
// will be returned.
func (r *KataConfigOpenShiftReconciler) updateStatus() error {

if r.getInProgressConditionValue() != corev1.ConditionTrue {
return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the commit message, why does just editing KataConfig externally trigger this problem? I'd say predating PR #329 should be the only scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is possible to remove the node list with something like :

oc patch --type=merge --subresource=status --patch='{"status":{"kataNodes":null}}' kataconfig/my-kataconfig

Of course, people shouldn't do that but it doesn't mean we shouldn't be able to recover 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is, to my understanding predating PR #329 is the only actual condition. If that's fulfilled then any store will cause problems, right? A store can happen in a number of ways and the user editing the CR is in no way special among them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. Install 1.5.0 on a pristine cluster, deploy kata and do the oc patch above, you'll see in the controller logs that reconcile is called but the node list isn't rebuilt.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is un-rebuilding nodes a blocker?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is un-rebuilding nodes a blocker?

Not really as it doesn't prevent the operator to be functional.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I suspect there would be quite a lot of things that user could do to sabotage the controller which wouldn't recover. ;-) But that's a fact indepedent of the idea of this PR - in fact, this has always been true and continues to be true even after this PR I believe.

My idea was not to mix independent facts in the message and not put them in the same context as if they were related since that could confuse a future reader. I'm not insisting though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah this is merged already... never mind.

err, nodeList := r.getNodes()
if err != nil {
return err
Expand Down