Change default labels to use Kubernetes resource name #698

burmanm · 2024-09-04T12:30:34Z

What this PR does:
Changes datacenter label value to be Kubernetes resource name for Datacenter again. Provides backwards compatibility with older versions using the wrong one.

All the label names are updated after a successful reconcile (this is an existing feature). Tested with Helm installation of 1.22.1 and then updating the cass-operator image only.

Which issue(s) this PR fixes:
Fixes #691
Fixes #699

Checklist

Changes manually tested
Automated Tests added/updated
Documentation added/updated
CHANGELOG.md updated (not required for documentation PRs)
CLA Signed: DataStax CLA

…not datacenter override name. Create new Status field for MetadataVersion and if we keep running old ones (or old CRD), we simply fetch also with the older possible name

Miles-Garnsey

I need to do more testing on this in several respects;

Does an upgrade from the previous version to this one work correctly when an override name is in place?
Does this work when incorporated into downstream projects (e.g. k8ssandra-operator)? If not, should we make changes to the downstream projects, ensure everything works, and only then merge the changes?

Also @burmanm can you give me an overview of how the backwards compatibility is supposed to work? I don't think this was on the original ticket and I'm curious as to whether that status field you're using is pre-existing, and where it comes from generally.

Miles-Garnsey · 2024-09-11T09:22:08Z

apis/cassandra/v1beta1/cassandradatacenter_types.go

@@ -596,7 +599,7 @@ func (dc *CassandraDatacenter) SetCondition(condition DatacenterCondition) {
 // GetDatacenterLabels ...
 func (dc *CassandraDatacenter) GetDatacenterLabels() map[string]string {
 	labels := dc.GetClusterLabels()
-	labels[DatacenterLabel] = CleanLabelValue(dc.DatacenterName())
+	labels[DatacenterLabel] = CleanLabelValue(dc.Name)


Question: Is CleanLabelValue still necessary now that we're using the Kubernetes meta.name which will be DNS compliant anyway?

Well, not really, but they do have a bit different rules:

dns1123SubdomainFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*" dns1123LabelFmt string = "(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?"

So better safe than sorry I guess, especially if Kubernetes would one day update these..

Suggestion: I think the dns1123LabelFmt is more liberal than the dns1123SubdomainFmt, I also don't think k8s will ever change these (since they're linked to the DNS spec).

I'd remove the extra function call just so it is clear to readers that the label is always going to be the exact meta.Name, and not something else that we've interfered with.

This is just a suggestion though, so it isn't blocking.

burmanm · 2024-09-11T11:18:42Z

Yes, the operator upgrade test will deploy with override name in place (with an operator version that uses the override naming) and update from there.

The status field is not pre-existing, yet it wouldn't matter if it did or did not. A CRD upgrade will add it and the default is treated as 0 (such as in the cases that the field was not previously populated or does not exists). If the CRD is old one, it would simply fetch too much information on every reconcile but not break anything. I did test this manually against existing cluster.

If you wish to test that, the simplest way is to deploy cass-operator with Helm and do an upgrade. For example:

helm install --namespace cass-operator --create-namespace k8ssandra k8ssandra/cass-operator --set global.clusterScoped=true

Deploy something, like the one used in operator upgrade:

kubectl apply -f tests/testdata/default-three-rack-three-node-dc-4x.yaml

And then update cass-operator to a version built from this repo:

helm upgrade --namespace cass-operator k8ssandra k8ssandra/cass-operator --reuse-values --set image.registry="" --set image.tag=v1.23.0-dev.3f1fd20-20240906

And the cluster should stay healthy and get the updated values. This process does not update the CRD, only the operator image.

As for downstream projects, it's not really a place here to think about that (if the downstream is unable to use upstream, then the fix must happen in the downstream). That said, k8ssandra-operator uses these methods directly so that would work as soon as the first reconcile has finished here.

Miles-Garnsey · 2024-09-11T11:58:11Z

As for downstream projects, it's not really a place here to think about that (if the downstream is unable to use upstream, then the fix must happen in the downstream). That said, k8ssandra-operator uses these methods directly so that would work as soon as the first reconcile has finished here.

In theory that's true, but if we merge this to master and then cut a cass-operator release, then we are blocked on incorporating that release into downstream projects until they are also updated and tested to ensure that the new cass-operator version doesn't break them. As a result, I tend to think we should prep and test the downstream changes before we merge this.

On the other hand, changes to cass-operator can always break downstream repos and that's why we do integration tests in those downstreams before release. So I'm not necessarily going to block this PR on that basis, if you really want to merge now. Just saying that we may need to do quite a lot of work in the downstreams before we can then upgrade cass-operator.

If you wish to test that, the simplest way is to deploy cass-operator with Helm and do an upgrade. For example:

This is very helpful, thank you! That'll help me get this tested much faster.

Miles-Garnsey

There's a doc string in the types you need to change that currently reads:

Miles-Garnsey

Issue: There's a doc string in the types you need to change that currently reads:

Suggestion: It would be good to have some webhook protection against excessively long dc and cluster names to avoid this kind of problem:

2024-09-12T07:45:13.712Z	ERROR	Could not create headless service	{"controller": "cassandradatacenter_controller", "controllerGroup": "cassandra.datastax.com", "controllerKind": "CassandraDatacenter", "CassandraDatacenter": {"name":"correct-dc-name","namespace":"default"}, "namespace": "default", "name": "correct-dc-name", "reconcileID": "3e8bfbde-c188-408b-8244-0a74c55edb56", "namespace": "default", "datacenterName": "dcnamespecialcharacters", "clusterName": "cluster1&&OveriddenName", "error": "Service \"cluster1overiddenname-dcnamespecialcharacters-additional-seed-service\" is invalid: metadata.name: Invalid value: \"cluster1overiddenname-dcnamespecialcharacters-additional-seed-service\": must be no more than 63 characters"}

Checks:

Master Branch

Pods getcassandra.datastax.com/datacenter: dcNmCharacters
Services get cassandra.datastax.com/datacenter: dcNmCharacters
PVCs get cassandra.datastax.com/datacenter: dcNmCharacters
sts gets cassandra.datastax.com/datacenter: dcNmCharacters
pvs aren't labelled at all.
ConfigMaps - it doesn't look like there are any.

In the status I see datacenterName: dcNm!&^^ Characters

One odd thing with secrets, it looked like I ended up with two: cl1oridename-superuser cluster1-superuser. One had cassandra.datastax.com/datacenter: My_Super_Dc and the other had cassandra.datastax.com/datacenter: dcNmCharacters. I think the former might not have been cleaned up. Suggestion: investigate if this still happens after this PR, since it is possible that the cleanup failures are related to the buggy labelling behaviour that we are fixing here.

After upgrade

Pods get cassandra.datastax.com/datacenter: dcNmCharacters
Services get cassandra.datastax.com/datacenter: dcNmCharacters
I'm gonna stop here, it doesn't look like things have been upgraded...

In the status I see datacenterName: dcNm!&^^ Characters

Checking the logs, I get

kubectl logs -n cass-operator k8ssandra-cass-operator-84ff466f58-cxlxq
Error from server (BadRequest): container "cass-operator" in pod "k8ssandra-cass-operator-84ff466f58-cxlxq" is waiting to start: trying and failing to pull image

I assume this is because the image isn't published anywhere? Do I need to build it myself and then push it somewhere to get this tested @burmanm ?

burmanm · 2024-09-17T13:40:56Z

I assume this is because the image isn't published anywhere? Do I need to build it myself and then push it somewhere to get this tested @burmanm ?

Right, it should be available in michaelburman290/cass-operator with the tag I mentioned. Your comments around doc string don't show up at all, no idea what happened here.

burmanm added 6 commits September 4, 2024 15:30

Change default labels to use Kubernetes resource datacenter name and …

0573b8b

…not datacenter override name. Create new Status field for MetadataVersion and if we keep running old ones (or old CRD), we simply fetch also with the older possible name

Try to fix the flaky envtest

c16cf23

Some test changes

264d94d

Update some tests to get Github to run correct tests

a9fae75

Fix unit test

f04541b

Test for datacenterPods() functionality

5c78d45

burmanm marked this pull request as ready for review September 6, 2024 07:43

burmanm requested a review from a team as a code owner September 6, 2024 07:43

Move autoupdate-spec annotation removal

3f1fd20

Miles-Garnsey reviewed Sep 11, 2024

View reviewed changes

Miles-Garnsey reviewed Sep 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default labels to use Kubernetes resource name #698

Change default labels to use Kubernetes resource name #698

burmanm commented Sep 4, 2024 •

edited

Loading

Miles-Garnsey left a comment

Miles-Garnsey Sep 11, 2024

burmanm Sep 11, 2024

Miles-Garnsey Sep 11, 2024

burmanm commented Sep 11, 2024

Miles-Garnsey commented Sep 11, 2024

Miles-Garnsey left a comment

Miles-Garnsey left a comment

burmanm commented Sep 17, 2024

Change default labels to use Kubernetes resource name #698

Are you sure you want to change the base?

Change default labels to use Kubernetes resource name #698

Conversation

burmanm commented Sep 4, 2024 • edited Loading

Miles-Garnsey left a comment

Choose a reason for hiding this comment

Miles-Garnsey Sep 11, 2024

Choose a reason for hiding this comment

burmanm Sep 11, 2024

Choose a reason for hiding this comment

Miles-Garnsey Sep 11, 2024

Choose a reason for hiding this comment

burmanm commented Sep 11, 2024

Miles-Garnsey commented Sep 11, 2024

Miles-Garnsey left a comment

Choose a reason for hiding this comment

Miles-Garnsey left a comment

Choose a reason for hiding this comment

Checks:

burmanm commented Sep 17, 2024

burmanm commented Sep 4, 2024 •

edited

Loading