Reverts deprecated ClusterClaims for odf-info

[raaizik]

Changes

• Re-adds a single ClusterClaim that stores the NamespacedName of the odf-info CM so that MCO will be able to fetch it.
• Skips uninstall/delete so this cluster claim will live as long as the operator does.
• RHSTOR-5117
• RDR: ODF info CM logic #2440
• Depends on Available CRDs check feature #2712

TODO

• Simplify by moving ClusterClaim to be reconciled in OCSInitialization
• Delete the cluster claim once the odf-info CM is removed

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[raaizik]

/cc @rewantsoni @umangachapagain

[nb-ohad]

Is there really a need to create a new non-cached client? Why can't we use the client which is already installed on the reconciler object?

As a general note, we should only use non-cached clients in cases where there is underlying reason to do so.

[vbnrh]

@raaizik Why does the claim need to be removed manually ? Can we not set it to be removed alongside StorageCluster?

[vbnrh]

Can we add another claim which will indicate which clusters are client and provider for the UI ?

[raaizik]

Can we add another claim which will indicate which clusters are client and provider for the UI ?

@vbnrh This would break the concept of the odf-info CM which is supposed to be a replacement for cluster claims (other than the one that's added here, which I'd classify as the CM's metadata). What is an option is to add this to the CM itself.

[raaizik]

@raaizik Why does the claim need to be removed manually ? Can we not set it to be removed alongside StorageCluster?

@vbnrh The cluster claims used to be reconciled via storage cluster because they contained information about it. This is no longer the case, plus the cluster claim needs to be coupled with the operator's lifecycle where it is created once and removed manually (since there's no uninstall for OCSInit).

[vbnrh]

@raaizik Why does the claim need to be removed manually ? Can we not set it to be removed alongside StorageCluster?

@vbnrh The cluster claims used to be reconciled via storage cluster because they contained information about it. This is no longer the case, plus the cluster claim needs to be coupled with the operator's lifecycle where it is created once and removed manually (since there's no uninstall for OCSInit).

If the StorageCluster is uninstalled at a later point of time, the CM will still be present on the hub. How are we going to indicate that cluster is not available for RDR on hub ? Are we planning or have we added the StorageCluster's status on the CM ?

[raaizik]

@raaizik Why does the claim need to be removed manually ? Can we not set it to be removed alongside StorageCluster?

@vbnrh The cluster claims used to be reconciled via storage cluster because they contained information about it. This is no longer the case, plus the cluster claim needs to be coupled with the operator's lifecycle where it is created once and removed manually (since there's no uninstall for OCSInit).

If the StorageCluster is uninstalled at a later point of time, the CM will still be present on the hub. How are we going to indicate that cluster is not available for RDR on hub ? Are we planning or have we added the StorageCluster's status on the CM ?

@vbnrh Whenever a StorageCluster is uninstalled, its respective key in the CM gets removed. If that key was the last one to get uninstalled, then the whole CM gets removed.

[nb-ohad]

@vbnrh The responsibility of the claim is to broadcast (to ACM) the existence of the configmap not the existence of a specific storage cluster. The existence, state, and info of the various storage clusters can be found as part of the config map data (which will be available on the ACM hub)

As @raaizik already mentioned, when a storage cluster is deleted the config map is updated to remove all information related to that storage cluster from the config map, and that change will be synced back to the hub

So as you know, clients are not standalone entities, and the information about them is specified under the storage cluster that supports them. We don't need to have a separate claim for them.

[raaizik]

/retest

[umangachapagain]

No need of this. Simply fetch the operator namespace and proceed. This pattern was used earlier when there were a lot of things to fetch and create.

[umangachapagain]

I'd like to avoid setting panic to restart the process as much as possible. There are ways to set watches dynamically, we should try those first.

[nb-ohad]

@umangachapagain They don't work because they do not re-popolate the cache. The only reliable way we found was to panic out.

@raaizik I would not use panic, I will use os.exit with a specific code. That way we can warp the operator invocation with a small bash command that invokes the operator process if it sees that particular exit code.
Doing it that way will prevent unnecessary pos restarts

[umangachapagain]

Since we are not deleting clusterclaims, can we remove the rbac for it?

[umangachapagain]

We don't need to use this pattern. We can just create a "ensureClusteClaimExists" function and call it directly.

[nb-ohad]

If the intent is to make sure it exists, we can have an IsClusterClaimExist function that returns a bool.
Using an error to indicate a valid result (does not exists) is a semantic mistake

[umangachapagain]

IsClusterClaimExist is a check that returns True or False. What we want is to take action to EnsureClusterClaimExists and return an error if failed. That is semantically correct.

func ensureClusterClaimExists() error {
  if IsClusterClaimExist(){
     return nil
  }
  err := create.clusterclaim()
  return err
}

[raaizik]

/retest

[raaizik]

@nb-ohad @umangachapagain QE tests for MDR and RDR is getting blocked as this PR is not in 4.17

Please take a look ASAP

It depends on #2712 @vbnrh

[raaizik]

/test ocs-operator-bundle-e2e-aws

[nb-ohad]

LGTM
@iamniting @umangachapagain Do you want to take a look before merging?

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: raaizik, umangachapagain

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [umangachapagain]

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

[rewantsoni]

/cherry-pick release-4.17

[openshift-cherrypick-robot]

@rewantsoni: new pull request created: #2742

In response to this:

/cherry-pick release-4.17

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.