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

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.

…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

burmanm · 2024-10-15T06:21:32Z

@Miles-Garnsey ping

Miles-Garnsey · 2024-10-24T08:33:02Z

Going again... datacenterName: DC&n_!!

Checks:
Master Branch

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

In the status I see datacenterName: DC&n_!!

After upgrade

Pods get cassandra.datastax.com/datacenter: test-dc
Services get cassandra.datastax.com/datacenter: test-dc
PVCs get cassandra.datastax.com/datacenter: test-dc
statefulset gets cassandra.datastax.com/datacenter: DCn (@burmanm is this expected?)

Otherwise this looks about right to me.

Miles-Garnsey

@burmanm has raised another ticket to investigate the STS labels not being changed here. I'll approve this since he wants to investigate that behaviour elsewhere.

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

Miles-Garnsey reviewed Sep 11, 2024

View reviewed changes

Miles-Garnsey reviewed Sep 12, 2024

View reviewed changes

burmanm force-pushed the change_labels branch from 3f1fd20 to 2b8787a Compare October 2, 2024 06:47

burmanm added 8 commits October 7, 2024 15:06

Change default labels to use Kubernetes resource datacenter name and …

0700d29

…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

344c573

Some test changes

b0e4cdd

Update some tests to get Github to run correct tests

d0648ef

Fix unit test

3edb0ea

Test for datacenterPods() functionality

2b2b4b4

Move autoupdate-spec annotation removal

3353fc2

Fix DatacenterName description

b106e9b

burmanm force-pushed the change_labels branch from 2b8787a to b106e9b Compare October 7, 2024 12:08

Regenerate CRD

a08e8eb

burmanm requested a review from Miles-Garnsey October 23, 2024 08:27

burmanm mentioned this pull request Oct 24, 2024

CheckRackLabels patching is ignored? #719

Closed

Miles-Garnsey approved these changes Oct 24, 2024

View reviewed changes

burmanm merged commit c82c668 into k8ssandra:master Oct 24, 2024
47 checks passed

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

burmanm commented Oct 15, 2024

Miles-Garnsey commented Oct 24, 2024

Miles-Garnsey left a comment

Change default labels to use Kubernetes resource name #698

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 Oct 15, 2024

Miles-Garnsey commented Oct 24, 2024

Miles-Garnsey left a comment

Choose a reason for hiding this comment

burmanm commented Sep 4, 2024 •

edited

Loading