Set Mirroring field to false than setting it to nil

[vbnrh]

https://github.com/red-hat-storage/ocs-operator/blob/d62b2a2b1f5663b5a11ff88c9b7a830a298ca071/controllers/storagecluster/cephblockpools.go#L97

OCS operator does not check if cephblockpool's mirroring is enabled when mirroring field in the StorageCluster spec is set to nil.

Hence setting it nil from here, causes the cephblockpool to remain in true state.

This PR fixes that and explicitly sets the Mirroring spec to be false in StorageCluster

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vbnrh

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 [vbnrh]

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

[vbnrh]

/cherry-pick release-4.18

[openshift-cherrypick-robot]

@vbnrh: once the present PR merges, I will cherry-pick it on top of release-4.18 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.18

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.

[vbnrh]

/assign @umangachapagain

[umangachapagain]

We do not want to initialize it everytime.
We only want to initialize it when MCO is the one controlling the enable/disable of mirroring. That was the reason we moved the initialization under the if enabled { block.

@rewantsoni Since the mirroring spec is managed by 2 different controllers, I think we need to document how to work with this spec and the expected behavior.

[umangachapagain]

We should set it to nil as we will no longer be managing this spec once mirroring is disabled.

[umangachapagain]

IMO we need to understand what the problem is, before making any changes. This could fix one problem but cause another.

[vbnrh]

IMO we need to understand what the problem is, before making any changes. This could fix one problem but cause another.

@umangachapagain

type MirroringSpec struct {
	// If true, data mirroring is enabled for the StorageCluster.
	// This configuration will only be applied to resources (such as CephBlockPool)
	// managed by the operator.
	// It is optional and defaults to false.
	// +optional
	Enabled bool `json:"enabled,omitempty"`

	// PeerSecretNames represents the Kubernetes Secret names of rbd-mirror peers tokens
	// +optional
	PeerSecretNames []string `json:"peerSecretNames,omitempty"`
}

Currently there are two fields in the MirroringSpec, which are both used solely for RDR purposes. Intializing an empty struct would only intialize with the default values which would be false and nil for the PeerSecretNames.

The issue is caused in OCS end which does not check the CephBlockPool's mirroring is enabled or not if the struct is set to nil. Hence if it is set to nil at a later point, the CBP's mirroring still remains enabled (which is the issue the script is facing).

The script tries to
-> Delete MirrorPeer
-> Uninstall starts which sets the Mirroring field to nil
-> During OCS reconciliation, the controller skips through the Mirroring field if it is nil (As pasted in the description)
-> CephBlockPool remains in Mirroring enabled state.

So there are few ways to do this -

1. Setting it false on MCO end explicitly as in this PR. This will ensure the StorageCluster mirroring is disabled when MirrorPeer has been deleted (which is what we were doing since many releases till the Mirroring spec was turned to a pointer variable)
2. Fix it on OCS end to disable CephBlockPool's mirroring if it finds the Mirroring spec to be nil which is what it should ideally be doing.

I opted to explicitly initialize and disable it as i did not see any side effects through initialized default values. This is only for internal mode, the uninstall end for provider RDR does not manage these fields as it is handled through the StorageClusterPeer API