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

MAISTRA-985 track operator version on custom resources #252

Merged
merged 3 commits into from Oct 5, 2019

Conversation

rcernich
Copy link
Contributor

@rcernich rcernich commented Oct 2, 2019

Signed-off-by: rcernich rcernich@redhat.com

@rcernich rcernich requested review from luksa and knrc October 2, 2019 18:55
@rcernich rcernich force-pushed the MAISTRA-985 branch 3 times, most recently from 50c8727 to 0318b2c Compare October 3, 2019 00:12
@rcernich
Copy link
Contributor Author

rcernich commented Oct 3, 2019

I've repurposed the ObservedGeneration field so that it's formatted as version-generation, e.g. 1.0.0-1 (operator version 1.0.0, cr generation 1).

That said, the upgrade fails because the labels are changed on the deployment template specs (we set maistra-version label there). We'll need to figure out how to handle this (probably best to omit this from the deployment.spec.template.spec.labels).

@luksa
Copy link
Contributor

luksa commented Oct 3, 2019

I'm not sure changing the ObservedGeneration is a good idea, since it is always just a simple integer in all other Kubernetes resources. I'm afraid deviating from the convention might end up biting us in the future.

Also, do people need the operator version or the service mesh version? I don't see why the operator version is even important here.

@knrc
Copy link

knrc commented Oct 3, 2019

Also, do people need the operator version or the service mesh version? I don't see why the operator version is even important here.

The operator version may be important in future when we have multiple streams however the service mesh version is more likely to be the useful in the longer term especially when we have support for multiple versions.

@rcernich
Copy link
Contributor Author

rcernich commented Oct 3, 2019

I can certainly change the name of the variable, deprecating ObservedGeneration.

The reason we're using the operator version is because the purpose here is to force a reconcile when the operator is updated.

@rcernich
Copy link
Contributor Author

rcernich commented Oct 3, 2019

Regarding service mesh version, I would expect this to be tracked separately, where users specify the version they want to use (i.e. mesh version would be in the spec, not necessarily the status). Beyond that, I think the operator version is more important, for reasons stated above.

Signed-off-by: rcernich <rcernich@redhat.com>
Signed-off-by: rcernich <rcernich@redhat.com>
…pdates

Signed-off-by: rcernich <rcernich@redhat.com>
@rcernich
Copy link
Contributor Author

rcernich commented Oct 4, 2019

I've left ObservedGeneration alone and have added a ReconciledVersion field which encompasses the CR generation and the operator's version. This is used to trigger a reconcile when the operator is updated (e.g. version would go from 1.0.0-1 to 1.0.1-1, same generation, 1, but different operator version).

The other commits deal with issues in the Deployment.spec.selector, where the maistra-version label was being included in the selector (the selector field is immutable). The MAISTRA-991 removes the label from places where it shouldn't have been (e.g. in the pod template labels, PodDisruptionBudget selector, etc.), while MAISTRA-992 handles updates where patching won't work (e.g. modifying Deployment.spec.selector).

All of these commits together should enable an automatic update when the operator is updated and should help alleviate issues in future z-stream updates (e.g. 1.0.1 -> 1.0.2).

@knrc
Copy link

knrc commented Oct 5, 2019

It looks as if this PR covers three issues, MAISTRA-985, MAISTRA-991 and MAISTRA-992

Copy link

@knrc knrc left a comment

Choose a reason for hiding this comment

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

lgtm, I'll merge it manually

@knrc knrc merged commit 8617354 into maistra:maistra-1.0 Oct 5, 2019
@rcernich rcernich deleted the MAISTRA-985 branch March 25, 2020 17:49
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.

None yet

3 participants