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

Fix: RestartPolicy serde results in different revision #497

Merged
merged 2 commits into from
Oct 21, 2022

Conversation

regadas
Copy link
Contributor

@regadas regadas commented Oct 21, 2022

The change to ommit the RestartPolicy when not set (introduce in #449) results in null not be included in the json serialization resulting in different revision number causing Stopped clusters to Update.

We should ommitempty for RestartPolicy but that should be deffered to v1beta2

@regadas regadas requested a review from live-wire October 21, 2022 08:13
var revision, _ = newRevision(&flinkCluster, 1, &collisionCount)
var expectedRevision = appsv1.ControllerRevision{
ObjectMeta: metav1.ObjectMeta{
Name: "mycluster-fb7687bf5",
Name: "mycluster-7bc87c954f",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New revisions trigger cluster updates! so this value change should be treated carefully. In this case this is changed to previous value.

The change to ommit the `RestartPolicy` when not set results in null not
be included in the json serialization resulting in different revision
number causing Stopped clusters to Update.

We should `ommitempty` for `RestartPolicy` but that should be deffered to `v1beta1`
@live-wire
Copy link
Contributor

Hey @regadas ,
Do I understand this correctly?

  • Whenever there is a change in the API spec (serde) we get a new controller-revision.
  • Whenever there is a new controller-revision, all old CRDs are also reconciled.
  • If old CRDs didn't have a RestartPolicy set, they would be restarted. <- If this is correct, shouldn't we have a default value for RestartPolicy while computing "desired state"? 🤔

Copy link
Contributor

@live-wire live-wire left a comment

Choose a reason for hiding this comment

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

I think it is worthwhile to have a "default" RestartPolicy. Wdyt?

@regadas
Copy link
Contributor Author

regadas commented Oct 21, 2022

@live-wire RestartPolicy has the default value of "Never".

Whenever there is a change in the API spec (serde) we get a new controller-revision.

It depends but in general yes. There are some fields that will make it a backwards incompatible change.

Whenever there is a new controller-revision, all old CRDs are also reconciled.

Yes, if there's a new revision it's because there's a "seen" change and it triggers the Update on all clusters.

If old CRDs didn't have a RestartPolicy set, they would be restarted. <- If this is correct, shouldn't we have a default value for RestartPolicy while computing "desired state"?

There's a default value for it.

// +kubebuilder:default:=Never
RestartPolicy *JobRestartPolicy `json:"restartPolicy,omitempty"`

@regadas regadas requested a review from live-wire October 21, 2022 13:41
@live-wire
Copy link
Contributor

  • Whenever there is a new controller-revision, all old CRDs are restarted. (expected behaviour)
  • RestartPolicy being set or not is not respected if there is a controller-revision update.

@regadas regadas merged commit 3775d00 into spotify:master Oct 21, 2022
@regadas regadas deleted the fix_revision branch October 21, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants