Add new labels to sts pods, for backward compatibility with v0.23.0, which includes #777 #804
Conversation
|
/retest |
There was a problem hiding this comment.
Thanks for the PR @shreyas-s-rao. I have few comments and/or questions. PTAL. Thanks
…abel selector for sts recreation
|
@anveshreddy18 I have addressed your comments. PTAL |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
There was a problem hiding this comment.
Thanks for the PR @shreyas-s-rao!
Just one question: when downgrading etcd-druid from the changes proposed for v0.23.0 in 777 to this branch, I observe that the pods are rolled twice.
- Once when the statefulset is patched with the older labels for the pod template before the statefulset deletion.
- Once after the statefulset is deleted, and is recreated with the correct selectors.
Is this double rolling expected? It was my understanding that once the statefulset is recreated, it simply adopts the pods with the necessary labels.
Have a couple nits with comments for the sake of documenting in the code as well, but you can take a call and skip them, since the kind tests passing right now is a game of Russian roulette.
Thanks!
|
@shreyas-s-rao I've tested this PR with the latest changes pulled in from the refactor PR which were added after my review of this PR. My observations while performing the test on the latest changes in 777:
I've tried this multiple times and I observe the same behavior. Can @anveshreddy18 take a look and replicate the tests I've performed with the latest changes pulled in from the refactor PR? |
|
For some reason, I am unable to replicate the supposed issue I saw in my previous comment with |
…y are updated or ready, similar to gardener#823
|
@renormalize thanks for the review.
As we tested in-person, we see that the second rolling is due to the change in sts label selector, which reverts to the old labels. This changes the sts controller revision hash label on the pods, which causes the second rolling. Whereas, during upgrade from druid:v0.22.0 to this PR version, there is only one rolling of the pods, because the label selector on the sts remains the same. This PR simply adds new labels during the upgrade, and essentially adds old labels and updates label selector to old labels during downgrade. I have now tested this behavior, and pushed the changes. PTAL. |
|
@renormalize I have also added the comments as requested in your review. |
|
/retest |
|
/assign |
There was a problem hiding this comment.
Thanks @shreyas-s-rao for supporting backward compatibility with v0.23.0
|
@shreyas-s-rao & @renormalize: I've tested all the scenarios from mentioned in #804 (comment).
I'm not sure but going back to v0.22 of druid will lead to rollin and then recreation of ETCD. |
|
@ashwani2k Thanks for your testing. What you saw is as expected from this PR. Just to correct your wording slightly - the Downgrading from this branch to hotfix-v0.22 would cause the pods to roll once. Neither the sts nor the |
How to categorize this PR?
/area quality
/kind task
What this PR does / why we need it:
This PR adds new k8s-recommended labels to the etcd pods -
app.kubernetes.io/*, as defined here. This is required for backward compatibility when upgrading to etcd-druid:v0.23.0, which includes #777 , which switches all component labels and selectors to the new k8s-recommended labels, such that a downgrade from v0.23.0+ to v0.22.1 will be possible.Additionally, this PR also ensures that any new label selector set on the statefulset is replaced with old (expected) label selector, ie
{instance: <etcd-name>, name: etcd}, to be consistent with v0.22.0.All of this is achieved by adding support for a
preReconcileEtcd()step for the etcd reconciler, which calls statefulset component'sPreDeploymethod, which in turn does the following:orphancascade option, so that the subsequent stsDeployflow re-creates the sts with the expected (old) label selectorWhich issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: