search

red-hat-storage / ocs-operator

Operator for RHOCS
Apache License 2.0
86 stars 184 forks source link

[WIP] Sends `V*ReplicationClass` to client #2727

Open raaizik opened 1 month ago

raaizik commented 1 month ago

Changes

Sends two VRCs (one for image flattening) and one VGRC. Depends on #2620

RHSTOR-5753, RHSTOR-5794

raaizik commented 1 month ago

/cc @Madhu-1

raaizik commented 1 month ago

/cc @rewantsoni

openshift-ci[bot] commented 1 month ago

@raaizik: GitHub didn't allow me to request PR reviews from the following users: Rakshith-R.

Note that only red-hat-storage members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to [this](https://github.com/red-hat-storage/ocs-operator/pull/2727#issuecomment-2271445934): >/cc @Rakshith-R Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
raaizik commented 1 month ago

/cc @Rakshith-R

openshift-ci[bot] commented 1 month ago

@raaizik: GitHub didn't allow me to request PR reviews from the following users: Rakshith-R.

Note that only red-hat-storage members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to [this](https://github.com/red-hat-storage/ocs-operator/pull/2727#issuecomment-2271449581): >/cc @Rakshith-R Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
raaizik commented 1 month ago

"replication.storage.openshift.io/is-default-class": "true", This should be added as a annotation

and "replication.storage.openshift.io/flatten-mode": "force",

This as a label

hope both of the above will be handled accordingly in consumer side.

Other than the above, everything else looks good to me.

@Rakshith-R I'm aware and it will.

nb-ohad commented 1 month ago

For VolumeReplicationClass we are sending only the parameters field like we did for VolumeSnapshotClass and StorageClass, Should we send the entire spec for it or maybe the entire object that will allow us to add labels/annotations required (ramen labels, reclaimspace annotation) from the provider itself and the client would be responsible for adding missing fields like ProvisionerName and namespace for secrets and then creating/upating it.

@nb-ohad WDYT?

I will start with the later proposal first:

maybe the entire object that will allow us to add labels/annotations required

We do not want to do that, an object contains not just desired state but also status and some metadata that is local to the provider cluster. In internal discussions, we already acknowledged the need to send labels and annotations but not as part of the serialized spec we send (the data field of the ExternalResource struct). The plan is to add it to the metadata of the ExternalResource object, similar to the kind and name. Until that is implemented we will keep on having logic in the client to add the missing metadata.

Should we send the entire spec This depends on the structure of the type, we originally used data because that was the desired state for configmaps and secrets. If the Volume*ReplicationClass has a formal spec field it will make sense to serialize it instead of a map[string]string. As a matter of fact, we are already doing something similar for the ClusterResourceQuota CR (https://github.com/red-hat-storage/ocs-operator/blob/main/services/provider/server/server.go#L376-L399)

raaizik commented 2 weeks ago

/test ocs-operator-bundle-e2e-aws

raaizik commented 2 weeks ago

/test ocs-operator-bundle-e2e-aws

raaizik commented 6 days ago

/test ocs-operator-bundle-e2e-aws

nb-ohad commented 3 days ago

/lgtm

openshift-ci[bot] commented 3 days ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nb-ohad, raaizik

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](https://github.com/red-hat-storage/ocs-operator/blob/main/OWNERS)~~ [nb-ohad] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
raaizik commented 3 days ago

/retest

rewantsoni commented 3 days ago

/hold Holding as per https://github.com/red-hat-storage/ocs-operator/pull/2620#discussion_r1749185716 As the comment requests to add metadata (labels and annotation) and we are sending the labels and annotations both in the Data(Spec) field.

openshift-ci[bot] commented 12 hours ago

New changes are detected. LGTM label has been removed.

raaizik commented 10 hours ago

/hold Holding as per #2620 (comment) As the comment requests to add metadata (labels and annotation) and we are sending the labels and annotations both in the Data(Spec) field.

Can unhold now, @rewantsoni 👍

rewantsoni commented 10 hours ago

/unhold

raaizik commented 10 hours ago

/test ocs-operator-bundle-e2e-aws

raaizik commented 8 hours ago

Should we consider sending the spec of Volume*ReplicationClass instead of spec.parameter from the provider server?

I think we've had a previous discussion regarding this or similar. IMO the server should send only the info that's required and not the entire spec.