-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
provider: send vrcs with fixed replication schedule #2905
Conversation
services/provider/server/server.go
Outdated
@@ -814,24 +816,36 @@ func (s *OCSProviderServer) GetStorageClaimConfig(ctx context.Context, req *pb.S | |||
}), | |||
Labels: getExternalResourceLabels("VolumeGroupSnapshotClass", mirroringEnabled, false, replicationID, storageID)}, | |||
&pb.ExternalResource{ | |||
Name: "ceph-rbd", | |||
Name: req.StorageClaimName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like all other classes, our VRCs should have a fixed name and not one that depends on the request
services/provider/server/server.go
Outdated
}), | ||
Labels: getExternalResourceLabels("VolumeReplicationClass", mirroringEnabled, false, replicationID, storageID), | ||
Annotations: map[string]string{ | ||
"replication.storage.openshift.io/is-default-class": "true", | ||
}, | ||
}, | ||
&pb.ExternalResource{ | ||
Name: "ceph-rbd-flatten", | ||
Name: fmt.Sprintf("%s-flatten", req.StorageClaimName), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like all other classes, our VRCs should have a fixed name and not one that depends on the request
@@ -658,12 +661,17 @@ func TestOCSProviderServerGetStorageClaimConfig(t *testing.T) { | |||
"ramendr.openshift.io/storageID": "8d40b6be71600457b5dec219d2ce2d4c", | |||
}, | |||
}, | |||
"ceph-rbd-volumereplicationclass": { | |||
Name: "ceph-rbd", | |||
"block-pool-claim-volumereplicationclass": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the original name which is the name of the VRC
Signed-off-by: Rewant Soni <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either expand the existing comment on why this is temporary or the PR description, ie, a bit of technical details.
@@ -731,6 +731,8 @@ func (s *OCSProviderServer) GetStorageClaimConfig(ctx context.Context, req *pb.S | |||
replicationEnabled := len(scp.Items) > 0 | |||
replicationID := req.StorageClaimName | |||
|
|||
clientProfile := util.CalculateMD5Hash(req.StorageClaimName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could become problematic when we refactor the responsibility of provider & client side code, better be consistent and let client-op set this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was already decided that we are going to set clusterID from the provider
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iamniting, leelavg, rewantsoni 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:
Approvers can indicate their approval by writing |
0d54af2
into
red-hat-storage:main
Send the entire spec of VRC that includes schedule, provisioner, clusterID from provider
Sending a VRC with a fixed schedule is temporary for feature freeze, we will work on moving it to dynamic schedules after feature freeze