Check orphan PVC before updating statefulSet

[hoyhbx]

Change log description

• Check orphan PVCs are deleted before updating the statefulSet

Purpose of the change

Fixes #513

What the code does

(Detailed description of the code changes)
If the ZooKeeper cluster is using Persistence storage and the reclaimPolicy is set to Delete, this change makes sure that the number of PVCs is not larger than the statefulSet replicas before updating the statefulSet.

How to verify it

The bug #513 cannot be reproduced after this commit. E2E test is added to prevent regression

[codecov]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (2c8bfec) 85.91% compared to head (257813a) 85.41%.

Files	Patch %	Lines
controllers/zookeepercluster_controller.go	90.62%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #526      +/-   ##
==========================================
- Coverage   85.91%   85.41%   -0.51%     
==========================================
  Files          12       12              
  Lines        1633     1659      +26     
==========================================
+ Hits         1403     1417      +14     
- Misses        145      156      +11     
- Partials       85       86       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[anishakj]

@hoyhbx Could you please increase the code coverage. Also the DCO check is failing

[hoyhbx]

I will try to write a system test to cover these two branches

[hoyhbx]

@anishakj, I wrote an e2e test to reproduce the issue.
During the process, I realize the previous fix does not work. I want to understand the code better before implementing the fix.

I want to confirm my understand for the usage of ZooKeeper.Status.Replicas and ZooKeeper.Status.ReadyReplicas before I implement the fix. The two fields seem to be used to reflect the StatefulSet's Status.Replicas and Status.ReadyReplicas fields, but they are only updated in this function:

zookeeper-operator/controllers/zookeepercluster_controller.go

Line 251 in 28d1f69

func (r *ZookeeperClusterReconciler) updateStatefulSet(instance *zookeeperv1beta1.ZookeeperCluster, foundSts *appsv1.StatefulSet, sts *appsv1.StatefulSet) (err error) {

I want to implement a fix to make the reconcileStatefulSet function to return early if there are still orphan PVCs, by comparing the ZooKeeper.Spec.Replicas to ZooKeeper.Status.Replicas.
The orphanPVC cleanup should be triggered by comparing against ZooKeeper.Status.Replicas instead of ZooKeeper.Spec.Replicas here:

zookeeper-operator/controllers/zookeepercluster_controller.go

Line 717 in 28d1f69

if pvcCount > int(instance.Spec.Replicas) {

But this doesn't work if the ZooKeeper.Status.Replicas is updated at the end of the reconcileStatefulSet function, because it would be skipped along with the StatefulSet update. I propose to update the ZooKeeper.Status.Replicas and ZooKeeper.Status.Replicas every reconcilation cycle, by either moving it earlier in the reconcileStatefulSet function, or move it to the reconcileClusterStatus function.

[hoyhbx]

Please check the latest commit for the tentative fix. I moved the ZooKeeper.Status.Replicas and ZooKeeper.Status.ReadyReplicas update to the reconcileClusterStatus.
For cleaning up orphanPVC, the fix gets the up to date StatefulSet to check for the need of cleaning up. It checks for the STS.Spec.Replicas against PVC count, because I found that in some corner cases STS.Status.Replicas may not up to date, causing it to delete the first PVC when starting the StatefulSet initially.

The e2e test added can reproduce the issue without the patch, and it passes after the patch

[anishakj]

@hoyhbx why are we deleting pvcs based on Sts replicas. Operator is looking for zookeeper cluster resource. Is there any issue are you seeing if we delete based on instance.Spec.Replicas?

[hoyhbx]

This is to handle the race condition, where the operator has not yet deleted the orphan pvcs after scaling down, but the user scale the cluster back up. In that case, if instance.Spec.Replicas is used to delete old pvcs, the old pvcs will never get deleted.

Just as the added e2e test, when scaling down from 3 to 1, two pods are deleted, and statefulSet's replica gets down to 1. Then it takes sometime for the operator to delete the orphan pvcs. But before operator is able to delete the orphan pvcs, user scales up from 1 to 3, changing the instance.Spec.Replicas to 3. Then the old PVCs will never be deleted.

[hoyhbx]

Hi @anishakj , does the above explanation make sense to you?
The problem is basically because there is a race condition when deleting the PVC and user upscaling

[anishakj]

@hoyhbx Could you please increase the code coverage for this?

[hoyhbx]

@anishakj I added unittests to test the PVC deletion logics and fixed the e2e test

[hoyhbx]

Hi @anishakj , have you gotten a chance to look at the changes? We are happy to make improvements if you have some suggestions.

[anishakj]

Hi @anishakj , have you gotten a chance to look at the changes? We are happy to make improvements if you have some suggestions.

will perform some more tests from my side and let you know

[hoyhbx]

Thanks @anishakj ! Just let us know if there is anything we could improve. We are also more than happy to fix the issue in the zookeeperStart.sh mentioned here: #513 (comment)

[hoyhbx]

Hi @anishakj , did you encounter any problem when testing this PR? Is there anything we can help?

[iampranabroy]

@anishakj - Is this part of the release v0.2.15. I don't see that as part of the change log. If not, any plans to include it in the new release? Thanks

[anishakj]

@anishakj - Is this part of the release v0.2.15. I don't see that as part of the change log. If not, any plans to include it in the new release? Thanks

@hoyhbx Could you please increase the coverage