Skip to content
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

Update provider-api-server to be deployed in internal and provider mode #2718

Merged

Conversation

rewantsoni
Copy link
Member

@rewantsoni rewantsoni commented Jul 29, 2024

Allow provider-server to be deployed in intenal and provider mode

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 both modes to enable both client to provider communication and cross odf communication.

The PR does the following:

  1. Create service, deployment, onboarding job for both modes
  2. Update the variable from watchnamespace to podnamespace
  3. Remove hardcoded name for storagecluster
  4. Move client configmap in storageclient
  5. Update the unit tests

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2024
Copy link
Contributor

openshift-ci bot commented Jul 29, 2024

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
Copy link
Contributor

nb-ohad commented Jul 29, 2024

@rewantsoni Please provide a proper PR title and description

@rewantsoni rewantsoni changed the title Api server Update provider-api-server to be deployed in all modes Jul 29, 2024
@iamniting
Copy link
Member

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.

Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you pls add deprication warning in the storage cluster API in first commit?

Also pls separate the generated changes in the 3rd commit and rename the deployment to the ocs-server.

@rewantsoni
Copy link
Member Author

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 rewantsoni force-pushed the api-server-ocs-init branch 4 times, most recently from 1262792 to e40e894 Compare August 1, 2024 03:13
@rewantsoni rewantsoni marked this pull request as ready for review August 1, 2024 04:13
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2024
Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you pls resolve merge conflict

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2024
@rewantsoni
Copy link
Member Author

/retest-required

@rewantsoni rewantsoni force-pushed the api-server-ocs-init branch 2 times, most recently from 31745ee to a2d1009 Compare August 6, 2024 06:24
controllers/storagecluster/provider_server.go Outdated Show resolved Hide resolved
tools/csv-merger/csv-merger.go Outdated Show resolved Hide resolved
Copy link
Contributor

@leelavg leelavg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rewantsoni
Copy link
Member Author

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.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2024
controllers/storagecluster/provider_server.go Show resolved Hide resolved
controllers/storagecluster/reconcile.go Outdated Show resolved Hide resolved
controllers/util/k8sutil.go Outdated Show resolved Hide resolved
onboarding-validation-keys-generator/main.go Outdated Show resolved Hide resolved
onboarding-validation-keys-generator/main.go Outdated Show resolved Hide resolved
onboarding-validation-keys-generator/main.go Outdated Show resolved Hide resolved
@rewantsoni rewantsoni changed the title Update provider-api-server to be deployed in all modes Update provider-api-server to be deployed in internal and provider mode Sep 26, 2024
@nb-ohad
Copy link
Contributor

nb-ohad commented Sep 26, 2024

LGTM

@rewantsoni
Copy link
Member Author

rewantsoni commented Sep 26, 2024

rebased the PR

@nb-ohad
Copy link
Contributor

nb-ohad commented Sep 26, 2024

@iamniting Can you please take a look as well?

@openshift-ci openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2024
The commit does the following:
1. Create service, deployment, onboarding job for both modes
2. Update the variable from watchnamespace to podnamespace
3. Remove hardcoded name for storagecluster
4. Move client configmap in storageclient

Signed-off-by: Rewant Soni <resoni@redhat.com>
Signed-off-by: Rewant Soni <resoni@redhat.com>
Signed-off-by: Rewant Soni <resoni@redhat.com>
@rewantsoni
Copy link
Member Author

moved unit-tests into a separate commit

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2024
Copy link
Contributor

openshift-ci bot commented Sep 27, 2024

[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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2024
@iamniting
Copy link
Member

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 225a479 into red-hat-storage:main Sep 27, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants