Fix #316 by labeling the cluster uid to each PVC

Orange-OpenSource / casskop

This Kubernetes operator automates the Cassandra operations such as deploying a new rack aware cluster, adding/removing nodes, configuring the C* and JVM parameters, upgrading JVM and C* versions, and many more...

https://orange-opensource.github.io/casskop/

Apache License 2.0

183 stars 54 forks source link

Fix #316 by labeling the cluster uid to each PVC #322

Closed srteam2020 closed 3 years ago

srteam2020 commented 3 years ago

Q	A
Bug fix?	[X]
Related tickets	fixes #316
License	Apache 2.0

What's in this PR?

Label the cassandracluster uid to the PVC and checks the uid before deleting PVC.

Checklist

[ X] Implementation tested

cscetbon commented 3 years ago

@srteam2020 i wonder if it doesn't make more sense to update the helper to create Cassandra cluster to add a uid on the object if it's not there by default. Wdyt ? That would need we don't need the uid at all in our deployment files like we will do on a real k8s cluster. In the meantime integration tests are running

srteam2020 commented 3 years ago

@srteam2020 i wonder if it doesn't make more sense to update the helper to create Cassandra cluster to add a uid on the object if it's not there by default. Wdyt ? That would need we don't need the uid at all in our deployment files like we will do on a real k8s cluster. In the meantime integration tests are running

Yes, that definitely makes more sense to me, I'll revise that accordingly.

cscetbon commented 3 years ago

@srteam2020 I'll soon make a new release. It would be great if you can finish that one in order to include it. Thanks ! Lmk if you need some help

srteam2020 commented 3 years ago

@cscetbon Hi, I've removed the empty UID checking accordingly, I think now it is ready to go.

cscetbon commented 3 years ago

Thank you @srteam2020 for your first contribution ! 👏

cscetbon commented 3 years ago

@srteam2020 we discovered that this PR causes issues for users upgrading from 1.1.3-release to 1.1.4-release because of that label that is missing. We need to come up with the right way to do the upgrade. A big issue is that statefulsets can't be updated because of

...
# * spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 
'updateStrategy' are forbidden

It's needed to add the new label you've added and it's a problem...

srteam2020 commented 3 years ago

From the error msg, it seems that the label (cassandracluster-uid) we added was updated into some field of Spec, but normally, we only suppose it to be updated to ObjectMeta.Labels. Looking into the code which constructs statefulset, I found the label was also set into spec.selector which I believe cause the problem.

I think to add some filter here to the labels to only set cassandracluster-uid to statefulset’s ObjectMeta.Labels can fix the issue. What do you think @cscetbon .

cscetbon commented 3 years ago

That might work. I was going to rollback the change but that could be the answer. It still means a rolling restart during an upgrade but that should be ok. Can you test such a change and check an upgrade from 1.1.3 ?