-
Notifications
You must be signed in to change notification settings - Fork 48
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
Removed owner reference from etcd statefulsets #48
Removed owner reference from etcd statefulsets #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Can we add the test cases to cover at least the following edge cases?
- Reconciliation of new etcd resource deployment without any existing statefulsets.
- Reconciliation of new etcd resource deployment with adoption of existing statefulset.
- Reconciliation of existing etcd resource with statefulset with ownerReference.
- Reconciliation of deletion of etcd resource with statefulset with ownerReference.
- Reconciliation of deletion of etcd resource with statefulset without ownerReference but with owned-by annotations.
- Reconciliation of deletion of etcd resource with statefulset without ownerReference and without owned-by annotations.
Also, can you please add the reasoning to the release notes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks you for PR. Please address the comments.
a4c7ba8
to
e2f6419
Compare
e2f6419
to
5ef66e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, i thought of refactoring controller-ref-manager.go as separate task. But since we are touching part of it, let's at-least refactor and reusae the code that is being touched here.
ce80ff5
to
f64df55
Compare
f64df55
to
9a76a09
Compare
f0ed0aa
to
8cede83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanx for the test. I have tested the shoot creation/update with Gardener as well. Looks fine.
@ggaurav10 Will you please test against VPA/HVPA? Post that we can merge the PR and release.
@georgekuruvillak and I have already tested working from VPA/HVPA side, and it worked as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the test cases! Suggested some changes.
Apart from the suggestions below, I see quite a bit of code copied with variations in the test cases. Would it make sense to clean them up? Table tests could help.
controllers/etcd_controller_test.go
Outdated
time.Sleep(2 * time.Second) | ||
err = c.Delete(context.TODO(), instance) | ||
Expect(err).NotTo(HaveOccurred()) | ||
err = retryTillStatefulSetRemoved(c, &sts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Eventually
instead.
controllers/etcd_controller_test.go
Outdated
Expect(err).NotTo(HaveOccurred()) | ||
err = retryTillStatefulSetRemoved(c, &sts) | ||
Expect(err).NotTo(HaveOccurred()) | ||
err = retryTillEtcdRemoved(c, instance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Eventually
instead.
controllers/etcd_controller_test.go
Outdated
}) | ||
|
||
func getStatefulset(c client.Client, instance *druidv1alpha1.Etcd, ss *appsv1.StatefulSet) { | ||
func retryTillPodsDeleted(c client.Client, etcd *druidv1alpha1.Etcd) (*corev1.Pod, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Eventually
instead.
controllers/etcd_controller_test.go
Outdated
return pod, err | ||
} | ||
|
||
func retryTillEtcdRemoved(c client.Client, etcd *druidv1alpha1.Etcd) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Eventually
instead.
d07660e
to
b40cf63
Compare
@amshuman-kr I have changed the tests to table tests. PTAL |
b40cf63
to
14d3f4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for refactoring the tests. Could you please add a couple of test cases (one each for creation and deletion scenario) which are small variations on the existing cases. I think they would strengthen druid against some common use-cases.
case WithOwnerReference: | ||
ss = createStatefulsetWithOwnerReference(instance) | ||
Expect(len(ss.OwnerReferences)).ShouldNot(BeZero()) | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a case for pre-existing StatefulSet
with annotations? I know this won't happen when we make the next release. But it would be good to cover the case which would happen for every reconciliation after that. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pondering upon how the test would be like, what would be the expected behavior of the test. That the annotations still persist after the reconcile?
Eventually(func() error { return statefulSetRemoved(c, &sts) }, timeout, pollingInterval).Should(BeNil()) | ||
Eventually(func() error { return etcdRemoved(c, instance) }, timeout, pollingInterval).Should(BeNil()) | ||
}, | ||
Entry("when statefulset with ownerReference and without owner annotations, druid should adopt and delete statefulset", "foo61", WithOwnerReference, false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a case of pre-existing StatefulSet
with no ownerReference and no annotations in which case the it should not be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, if there is no owner reference/ owner annotations in the sts, the druid will adopt it if the selector match. This is because, in the pre druid scenario, we didnt have both and druid went ahead adopting it. Should we not continue the behavior? I have written the test for the same in the following line below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it won't adopt if the etcd being deleted, no? Then the StatefulSet
would be left untouched. A negative case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please make the change for the negative case, I mentioned above? I did not mean the adoption+deletion case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on offline conversation. We can take it forward to another PR since this requires reconcile call to be brought in into the test func.
14d3f4d
to
28b3c0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the test cases! The one open case can be addressed in another PR.
What this PR does / why we need it:
This PR removes the dependance on ownerReference in etcd statefulset. It has been observerd that ownerReference in statefulset prevents VPA from recommending values for vertical scaling.
Which issue(s) this PR fixes:
Fixes #47
Special notes for your reviewer:
Release note: