Allow version before 0.52 to upgrade #1126
Conversation
…/statefulset/daemonset will be deleted and a new one with updated selector will be created.
|
CI failed |
sure, realized it conflicted with other tests, I used a new obj and name to avoid it. I did not found it because I only tested on my IDE which ran one test at a time.
|
|
Hi @pavolloffay, may I get it reviewed? I am not sure about whether we want the e2e test for upgrading. I am ok to remove that part of change. |
| @@ -0,0 +1,15 @@ | |||
| apiVersion: kuttl.dev/v1beta1 | |||
There was a problem hiding this comment.
Could you please add here some docs that explain why this test is there?
| @@ -158,6 +158,11 @@ generate: controller-gen api-docs | |||
| e2e: | |||
| $(KUTTL) test | |||
|
|
|||
| # end-to-end-test for testing upgrading | |||
| .PHONY: e2e-upgrade | |||
| e2e-upgrade: | |||
There was a problem hiding this comment.
is the upgrade test executed on the CI?
| @@ -70,6 +71,16 @@ func expectedDaemonSets(ctx context.Context, params Params, expected []appsv1.Da | |||
| return fmt.Errorf("failed to get: %w", err) | |||
| } | |||
|
|
|||
| // Selector is an immutable field, if set, we cannot modify it otherwise we will face reconciliation error. | |||
| if !apiequality.Semantic.DeepEqual(desired.Spec.Selector, existing.Spec.Selector) { | |||
| params.Log.V(2).Info("Spec.Selector change detected, trying to delete, the new collector daemonset will be created in the next reconcile cycle", "daemonset.name", existing.Name, "daemonset.namespace", existing.Namespace) | |||
There was a problem hiding this comment.
When is the next reconcile loop?
Maybe we should be more explicit and force the creation and continue.
There was a problem hiding this comment.
since the operator is watching the delete event, the next loop will happen when the collector got deleted (you can try manually delete the deployment and operator created almost immediately). Normally we shall not do 2 updates at one time, which will generate extra events.
kuttl-test-upgrade.yaml
Outdated
| @@ -0,0 +1,17 @@ | |||
| # Make sure that the OT operator after upgrading itself, can upgrade the OT collectors without error. | |||
| # See issues 840 and 1117 for the bugs on upgrading OT operator. | |||
There was a problem hiding this comment.
Please use full links when mentioning issues/PRs.
I would prefer to be explicit there and document which upgrade version is tested and exactly why and for instance add link that caused the breaking change.
There was a problem hiding this comment.
sure, I added detailed comments here.
kuttl cannot verify a label was deleted, and unfortunately, in this test, the "app.kubernetes.io/version" was removed. I can verify the "app.kubernetes.io/version" was there in the old version but I cannot verify it does not exists in the new version. |
|
🎉 |
* when Spec.selector changed, since it is immutable, the old deployment/statefulset/daemonset will be deleted and a new one with updated selector will be created. * add e2e test. * address lint. * add newline to end. * add newline to end. * add delete options to deployment and statefulset. * remove unecessary propagation policy, the default background is working fine. * Add unit test. * revert accident change. * remove timeout wait, don't 2 changes in one reconcile cycle. * fix lint * fix test, change name to isolate it from other tests. * revert accident change. * remove unsed constant. * Add comment for explaining the upgrading e2e test. * add e2e upgrade to workflow. * Address comment, adding detail comments for e2e upgrade test. * rever accident change. Co-authored-by: Pavol Loffay <[email protected]>
resolves 1117.
I wrote a PoC e2e test, need suggestion on whether it should be included into the pipeline.