Introduce GetKeyValuesStretchCluster for Accurate Label Matching in StretchCluster

[OdedViner]

This PR addresses FBUGS-160a and introduces the GetKeyValuesStretchCluster function to resolve an issue with the contains string method. The new function ensures a separation of logic without impacting the existing GetKeyValues function. However, this PR did not resolve the issue: #2841

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: OdedViner
Once this PR has been reviewed and has the lgtm label, please assign umangachapagain for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Nikhil-Ladha]

Please update the comment as per the working of this function.

[OdedViner]

done

[Nikhil-Ladha]

This means for stretch cluster we can't have a different failure domain? Like, rack, host?

[Nikhil-Ladha]

We should update the GetKeysValues function to do an equality check with valid labels that we define in the validTopologyLabelKeys and return the key, values accordingly.

[Nikhil-Ladha]

@malayparida2000 @iamniting what do you guys think?

[malayparida2000]

Agree with @Nikhil-Ladha

[travisn]

Stretch always uses zones, just to confirm in this thread in addition to comments from others that confirmed the same.

[OdedViner]

done

[malayparida2000]

@OdedViner Can you add some details on what's the problem with the existing code? And what's the exact issue. It's difficult to understand the motive for the change without knowing the root cause you are trying to fix.

[malayparida2000]

Wasn't this already defined somewhere in the codebase?

[OdedViner]

The variable is defined in another package within the project, but it is a private variable (starts with a lowercase letter). Since it cannot be accessed outside its package.

[malayparida2000]

I saw our docs & went as far back as OCS 4.7. And I can confirm 2 things.

1. Stretch cluster always uses failure domain zone
2. For zone failure domain always topology.kubernetes.io/zone label is used. I don't see any need to search for any other labels such as failure-domain.kubernetes.io/zone.

[travisn]

Agreed with Malay on all these points. failure-domain.kubernetes.io/zone is an old deprecated label for topology that was removed from K8s many releases ago, we should not use it anymore.

[OdedViner]

@OdedViner Can you add some details on what's the problem with the existing code? And what's the exact issue. It's difficult to understand the motive for the change without knowing the root cause you are trying to fix.

Root Cause:
When incorrect labels are matched, it updates the NodeTopologies in the StorageCluster spec with incorrect zone information. This directly impacts the stretch cluster settings in the CephCluster spec by adding invalid zone information, causing the Rook operator to fail.

Impact:
1.The storage cluster misidentifies node topology information, leading to incorrect zone configuration.
2.In the stretch cluster setup, this results in the wrong zone labels being applied, disrupting the intended fault domain configuration.
3.Ultimately, the Rook operator fails to reconcile the CephCluster, breaking the deployment process.

Proposed Solution: To address this issue, I introduced a new function, GetKeyValuesStrechCluster, which specifically checks for relevant and valid topology keys (corev1.LabelZoneFailureDomainStable and labelZoneFailureDomainWithoutBeta). This ensures only the intended labels are considered, preventing incorrect matches and maintaining the integrity of the stretch cluster configuration.
The function isolates the logic required for stretch cluster scenarios, leaving the existing GetKeyValues behavior unchanged for other use cases. This makes the fix targeted and minimizes the risk of regressions elsewhere.

Why This Fix Is Necessary:
The root cause of the issue lies in the overly broad match logic (strings.Contains), which needs to be refined for stretch cluster use cases.
Without this fix, incorrect labels (e.g., topology.kubernetes.io/zone-principal) will continue to disrupt the stretch cluster settings.
The new function ensures correctness by limiting the keys checked to those explicitly relevant for stretch cluster configurations.
Let me know if more clarification or details are needed!

@malayparida2000 @Nikhil-Ladha @iamniting @sp98
Is the stretch cluster failure domain always set to "zone"?

[malayparida2000]

@malayparida2000 @Nikhil-Ladha @iamniting @sp98 Is the stretch cluster failure domain always set to "zone"?
I belive so https://docs.redhat.com/en/documentation/red_hat_openshift_data_foundation/4.13/html/configuring_openshift_data_foundation_disaster_recovery_for_openshift_workloads/introduction-to-stretch-cluster-disaster-recovery_stretch-cluster#applying-topology-zone-labels-to-ocp-nodes_stretch-cluster
@travisn can you please correct me?

[sp98]

Yes. It's always zone.

[malayparida2000]

In the whole of ODF we have only 3 supported kind of failure domains rack, hostname, and zone.
rack- topology.rook.io/rack
hostname/host- kubernetes.io/hostname
zone- topology.kubernetes.io/zone
So I think we can just refactor the code base & stop caring about any other labels in here. This would help us eliminate the contains logic. And we can just directly match the label key with these 3 values of labels.

[malayparida2000]

@OdedViner This fix selectively addresses the case of stretch cluster only, while the mis match of labels can still happen to a normal cluster if the customer uses an improperly formatted label. I would suggest fixing the issue properly for all cases.
Ref-#2965 (comment)
We should refactor the parts like how topologymap is built, failure domain is determined, keyvalues are fetched so that we finally fix this issue as a whole.