Skip to content

controller: handle internal client in provider mode upgrade #3157

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rewantsoni
Copy link
Member

@rewantsoni rewantsoni commented Apr 14, 2025

The internal Client/consumer in provider mode needs to be handled differently.
We need to onboard the client to the internal consumer. Hence, from the upgrade controller, we are creating a consumer/configMap, updating the client status to point to this new consumer, and in the end, deleting the old storage consumer.

We also need to restart ocs-provider-server as stores a cache of UUIDs of consumers

Fixes: https://issues.redhat.com/browse/DFBUGS-2290

Copy link
Contributor

openshift-ci bot commented Apr 14, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rewantsoni
Once this PR has been reviewed and has the lgtm label, please assign blaineexe for approval. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment


clusterID := util.GetClusterID(ctx, r.Client, &log)

internalConsumer := clusterID != "" && clusterID == storageConsumer.Status.Client.ClusterID
Copy link
Contributor

Choose a reason for hiding this comment

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

let's requeue if the clusterID is empty, better be correct rather than skipping it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree

Copy link
Contributor

Choose a reason for hiding this comment

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

don't we need to adjust this file some more as not to create a storageconsumer for upgraded provider mode internal consumer and also not to reconcile configmap?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, will do

Copy link
Contributor

Choose a reason for hiding this comment

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

Not reconciling CM needs a discussion probably w/ Ohad as he opposed to that earlier.

@rewantsoni rewantsoni force-pushed the upgrade-controller-internal branch 2 times, most recently from bf13971 to 1854534 Compare April 15, 2025 08:01
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2025
@rewantsoni rewantsoni force-pushed the upgrade-controller-internal branch from 1854534 to 6535cbc Compare April 21, 2025 06:19
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2025
@rewantsoni rewantsoni force-pushed the upgrade-controller-internal branch from 6535cbc to 763aa63 Compare April 21, 2025 06:21
@nb-ohad
Copy link
Contributor

nb-ohad commented Apr 24, 2025

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 24, 2025
spec.StorageClasses = []ocsv1alpha1.StorageClassSpec{
{Name: util.GenerateNameForCephBlockPoolStorageClass(storageCluster)},
{Name: util.GenerateNameForCephFilesystemStorageClass(storageCluster)},
func (r *StorageConsumerUpgradeReconciler) reconcileStorageConsumer(
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any reason duplicating it here, the minute provider internal consumer is marked as internal storagecluster controller will reconcile it and fills the necessary spec, moreover this is overwritten as well.

let me know if I'm misunderstanding anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be called for both internal consumers and external consumers that were in provider mode

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, that's the missing piece, thanks.

@rewantsoni rewantsoni force-pushed the upgrade-controller-internal branch from 763aa63 to c1dce98 Compare May 21, 2025 14:18
@rewantsoni rewantsoni force-pushed the upgrade-controller-internal branch from c1dce98 to 89c9973 Compare May 21, 2025 15:02
Copy link
Contributor

openshift-ci bot commented May 21, 2025

@rewantsoni: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/ocs-operator-bundle-e2e-aws 89c9973 link true /test ocs-operator-bundle-e2e-aws

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants