red-hat-storage / ocs-operator

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

Update provider-api-server to be deployed in all modes #2718

Open rewantsoni opened 1 month ago

rewantsoni commented 1 month ago

Allow provider-server to be deployed in all modes

Why? This is a part of RDR for Provider Mode RHSTOR-4886. With this epic, we are introducing rpc calls for odf to odf communication, the RPC's we have for this epic are limited to Onboarding a Storage Cluster on different Openshift clusters and Getting the Mirroring Info required for setting up RDR (refer #2671).

This can also be used to replace the mechanism we have in the internal mode for setting up Mirroring for blockpool using MCO's Agents.

Hence moving the ocs-provider-server deployment to be deployed in all modes to enable both client to provider communication and cross odf communication.

The PR does the following:

  1. Move the serviceType to ocsinit spec and update the endpoint details on ocsinit status
  2. Update the ocs-provider-server name to ocs-server and reconcile as a part of ocsinit reconciler
  3. Create the osc-server deployment via the csv
  4. Update onboarding job to be deployed from ocsinit reconciler
  5. Update the unit tests
  6. Handle deletion of the old secret, service, and deployment
  7. update odfInfo configMap to use the endpoint from ocsinit status
openshift-ci[bot] commented 1 month ago

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

nb-ohad commented 1 month ago

@rewantsoni Please provide a proper PR title and description

iamniting commented 1 month ago

Can the provider handle requests for multiple storage clusters?

Do we want to run only one ocs-provider-server deployment in case of the multiple storagecluster?

If yes then we can have it as part of the ocs initialization otherwise it should be part of the storagecluster reconciliation.

rewantsoni commented 1 month ago

Can the provider handle requests for multiple storage clusters?

Yes, the server should be able to handle multiple storage clusters, but the client will onboard to only the storageCluster in the same namespace as the provider-server

Do we want to run only one ocs-provider-server deployment in case of the multiple storagecluster?

Yes, one provider-server for multiple storageClusters

If yes then we can have it as part of the ocs initialization otherwise it should be part of the storagecluster reconciliation.

I have added the deployment as a part of csv and secret, service as part of ocsinit

rewantsoni commented 1 month ago

/retest-required

rewantsoni commented 1 month ago

client-op needs an update after this gets merged, isn't it?

@leelavg I don't think we need any update in client o/p after merging this, let me know if I missed anything.

leelavg commented 1 month ago

Looked again, yes, no update required.

obnoxxx commented 3 weeks ago

@nb-ohad wrote:

@rewantsoni Please provide a proper PR title and description

I think the description is leaking too much internal downstream product information to this public project.

Shouldn't we try to avoid this?

openshift-ci[bot] commented 3 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, 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: - ~~[OWNERS](https://github.com/red-hat-storage/ocs-operator/blob/main/OWNERS)~~ [iamniting] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
nb-ohad commented 2 weeks ago

/hold

@rewantsoni This PR introduces architecture changes to the way we handle the API service. Looking at the code I think these changes are unnecessary, complicated, and are pushing us in the wrong direction. I would like to have more discussion on this

nb-ohad commented 2 weeks ago

Adding to my last comment: The API server is not part of the orchestrator, it is part of the application being deployed. Assuming multiple StorageClusters deployed over multiple namespaces, I would expect to see an API server in each of these namespaces, the same way I would expect to see any other orchestrated payload.

We also need to consider that each of these might be configured differently

openshift-ci[bot] commented 2 weeks ago

New changes are detected. LGTM label has been removed.