Fix replication dcname overrides #1030
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1030 +/- ##
==========================================
+ Coverage 57.43% 57.48% +0.04%
==========================================
Files 99 100 +1
Lines 10011 10045 +34
==========================================
+ Hits 5750 5774 +24
- Misses 3768 3779 +11
+ Partials 493 492 -1
|
| @@ -59,7 +59,7 @@ jobs: | |||
| e2e_test: | |||
| - CreateMultiDatacenterCluster | |||
| - CreateMixedMultiDataCenterCluster | |||
| - AddDcToCluster | |||
| - AddDcToClusterDiffDataplane | |||
There was a problem hiding this comment.
This test was renamed because we have another test with the AddDcToCluster prefix (AddDcToClusterSameDataplane), which was executed each time AddDcToCluster was invoked.
This is because subtests in Go seem to execute every test that starts with the configured value.
| cassDcName := dcName | ||
| if dcNameOverride != nil && *dcNameOverride != "" { | ||
| cassDcName = *dcNameOverride | ||
| } |
There was a problem hiding this comment.
This is covered in the e2e tests, which codecov cannot detect.
| @@ -24,7 +24,7 @@ const ( | |||
| // DefaultResourceName generates a name for a new Reaper resource that is derived from the Cassandra cluster and DC | |||
| // names. | |||
| func DefaultResourceName(dc *cassdcapi.CassandraDatacenter) string { | |||
| return cassdcapi.CleanupForKubernetes(dc.Spec.ClusterName + "-" + dc.Name + "-reaper") | |||
| return cassdcapi.CleanupForKubernetes(dc.Spec.ClusterName + "-" + dc.DatacenterName() + "-reaper") | |||
There was a problem hiding this comment.
Tested in the e2e tests.
test/e2e/auth_test.go
Outdated
| reaper1Key := framework.ClusterKey{K8sContext: f.DataPlaneContexts[0], NamespacedName: types.NamespacedName{Namespace: namespace, Name: "cluster1-real-dc1-reaper"}} | ||
| reaper2Key := framework.ClusterKey{K8sContext: f.DataPlaneContexts[1], NamespacedName: types.NamespacedName{Namespace: namespace, Name: "cluster1-real-dc2-reaper"}} |
There was a problem hiding this comment.
I couldn't use the DcPrefix() function here because the DC object doesn't exist yet.
pkg/k8ssandra/util.go
Outdated
| @@ -5,7 +5,7 @@ import ( | |||
| "k8s.io/utils/strings/slices" | |||
| ) | |||
|
|
|||
| func GetDatacenterForDecommission(kc *api.K8ssandraCluster) string { | |||
| func GetDatacenterForDecommission(kc *api.K8ssandraCluster) (string, *string) { | |||
There was a problem hiding this comment.
suggestion: Could we have (string, string) as return type? I'm sure many would stumble into this issue, looking at the signature, but not realizing one is a pointer and one isn't. Especially since there's no real need for it be a pointer (it's not like we have to replace this instance of data and keep pointing to it catching changes).
|
issue: The last commit is not right and shouldn't be done. We need to have tests without dc name override as default. Otherwise the entire purpose of "metadata/name" goes away and should be removed and we create uuids for all datacenters. This PR however does not do that, but now we've removed all the normal behavior testing, thus user could be using the default settings and end up with non-working setup. Special cases should only have their special tests, not be part of every test. Otherwise we risk the possibility that we start to depend on every possible extra setting being available (which should then be added to all e2e tests) and cases where there's less settings set things will no longer work. |
adejanovski
force-pushed
the
fix-replication-dcname-overrides
branch
from
8b6368e
to
4453da0
Compare
|
SonarCloud Quality Gate failed.
|
What this PR does:
There are several places in the code where we don't account for the DC name overrides, notably when placing annotations that include dc names, and when creating restore mappings.
This PR addresses these problems and adds testing to avoid regressions.
Most E2E tests are now using a DC name override (at the notable exception of the upgrade one because older versions of the CRD didn't have the DatacenterName field).
This led to a lot of changes in the tests to replace many hardcoded references to the dc names with some future proof name discovery logic.
Which issue(s) this PR fixes:
Fixes #1026
Checklist