send vgrc from provider to client #3275
Conversation
|
/retest-required |
| package util | ||
|
|
||
| import ( | ||
| "context" |
There was a problem hiding this comment.
pls configure your editor to group the imports properly,
import (
stdlib
own-module
third-party
)
services/provider/server/server.go
Outdated
|
|
||
| resources := getKubeResourcesForClass( | ||
| consumer.Spec.VolumeGroupReplicationClasses, | ||
| "VolumeGroupSnapshotClass", |
There was a problem hiding this comment.
| "VolumeGroupSnapshotClass", | |
| "VolumeGroupReplicationClass", |
services/provider/server/server.go
Outdated
| remoteRbdStorageId string, | ||
| ) ([]client.Object, error) { | ||
| if mirrorEnabled, err := s.isConsumerMirrorEnabled(ctx, consumer); err != nil { | ||
| return kubeResources, err |
There was a problem hiding this comment.
| return kubeResources, err | |
| return nil, err |
no need for caller to second guess
openshift-merge-robot
added
the
needs-rebase
services/provider/server/server.go
Outdated
| } else if !mirrorEnabled { | ||
| klog.Infof("skipping distribution of VolumeGroupReplicationClass as mirroring is not enabled for the consumer") | ||
| return kubeResources, nil | ||
| } |
There was a problem hiding this comment.
Why does the server condition the sending of the information to the client if mirroring is not enabled? I would say that this is more a confusing behavior then anything else. If the admin configured something to be sent it should be sent, if he didn't first enabled mirroring then it is his mistake but the system should continue to be consistent.
There was a problem hiding this comment.
I was following the same behaviour that we had for the Repliaction class, but I think we should have the behaviour that you specified. I will open a new PR to have the same behaviour for the Repliaction class as well
| // pool name is added to the VGRC's template | ||
| poolName := vgrc.Spec.Parameters["pool"] | ||
| storageIDs := []string{rbdStorageId, remoteRbdStorageId, poolName} | ||
| slices.Sort(storageIDs) |
There was a problem hiding this comment.
This is wrong! it will create an assortment of potential orders between the storageId and the poolName.
You should sort only the storage ID and the name should be added last. Because we are only talking about 2 strings you don't need even apply a slice sort functionality, just a string compare functionality
| storageIDs := []string{rbdStorageId, remoteRbdStorageId, poolName} | ||
| slices.Sort(storageIDs) | ||
| replicationID := CalculateMD5Hash(storageIDs) | ||
| vgrc.Spec.Parameters["replication.storage.openshift.io/group-replication-secret-name"] = consumerConfig.GetCsiRbdProvisionerCephUserName() |
There was a problem hiding this comment.
Is there a reason the replication class uses the same user as the storage class?
There was a problem hiding this comment.
Replication class requires the provisioner secret, which is similar to what we have for the snapshot classes
| vgrc.Spec.Parameters["replication.storage.openshift.io/group-replication-secret-namespace"] = consumer.Status.Client.OperatorNamespace | ||
| vgrc.Spec.Parameters["clusterID"] = consumerConfig.GetRbdClientProfileName() | ||
| AddLabel(vgrc, ramenDRStorageIDLabelKey, rbdStorageId) | ||
| AddLabel(vgrc, ramenMaintenanceModeLabelKey, "Failover") |
There was a problem hiding this comment.
IF this is a constant in need to be part of the template
There was a problem hiding this comment.
Yes, this is a constant. We could add it to the template for VGRC and VRC as well.
@Nikhil-Ladha This needs to be added to the VGRC and VRC template created by MCO
Signed-off-by: Rewant Soni <[email protected]>
Signed-off-by: Rewant Soni <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rewantsoni The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-robot
removed
the
needs-rebase
Send VGRC from the provider to the client.
Fixes: https://issues.redhat.com/browse/DFBUGS-2672