OCPBUGS-23120: [IBM ROKS] cluster-storage-operator does not set upgradeable=True #417

dobsonj · 2023-11-09T22:53:49Z

https://issues.redhat.com/browse/OCPBUGS-23120

Problem

The upgradeable condition used to be set by SnapshotCRDController, but that was removed in fa9af3a

DefaultStorageClassController only sets upgradeable=True if unsupportedPlatformError.
IBM VPC returns supportedbyCSIError instead.
The current code assumes if supportedbyCSIError, then csidriveroperator controller will eventually set it.

The problem is if shouldRunController returns false for a driver with supportedbyCSIError. Then csidriveroperator does not start and nothing will set the condition. shouldRunController always returns false for IBM ROKS because of this StatusFilter.

Solution

DefaultStorageClassController has no way to tell if shouldRunController will eventually return false. So in this PR, CSIDriverStarter sets the condition if no controller is started, because we know at that point no other controller will set the condition.

/cc @openshift/storage @jeffnowicki

openshift-ci-robot · 2023-11-09T22:53:57Z

@dobsonj: This pull request references Jira Issue OCPBUGS-23120, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (wduan@redhat.com), skipping review request.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

https://issues.redhat.com/browse/OCPBUGS-23120

Problem

The upgradeable condition used to be set by SnapshotCRDController, but that was removed in fa9af3a

DefaultStorageClassController only sets upgradeable=True if unsupportedPlatformError.
IBM VPC returns supportedbyCSIError instead.
The current code assumes if supportedbyCSIError, then csidriveroperator controller will eventually set it.

The problem is if shouldRunController returns false for a driver with supportedbyCSIError. Then csidriveroperator does not start and nothing will set the condition. shouldRunController always returns false for IBM ROKS because of this StatusFilter.

Solution

DefaultStorageClassController has no way to tell if shouldRunController will eventually return false. So in this PR, CSIDriverStarter sets the condition if shouldRunController returns false, because we know at that point no other controller will set it.

/cc @openshift/storage @jeffnowicki

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/test-infra repository.

openshift-ci · 2023-11-09T22:54:01Z

@dobsonj: GitHub didn't allow me to request PR reviews from the following users: openshift/storage, jeffnowicki.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

https://issues.redhat.com/browse/OCPBUGS-23120

Problem

The upgradeable condition used to be set by SnapshotCRDController, but that was removed in fa9af3a

DefaultStorageClassController only sets upgradeable=True if unsupportedPlatformError.
IBM VPC returns supportedbyCSIError instead.
The current code assumes if supportedbyCSIError, then csidriveroperator controller will eventually set it.

The problem is if shouldRunController returns false for a driver with supportedbyCSIError. Then csidriveroperator does not start and nothing will set the condition. shouldRunController always returns false for IBM ROKS because of this StatusFilter.

Solution

DefaultStorageClassController has no way to tell if shouldRunController will eventually return false. So in this PR, CSIDriverStarter sets the condition if shouldRunController returns false, because we know at that point no other controller will set it.

/cc @openshift/storage @jeffnowicki

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/test-infra repository.

openshift-ci · 2023-11-09T22:54:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dobsonj

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~~ [dobsonj]

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

dobsonj · 2023-11-10T01:54:39Z

/retest

jsafrane · 2023-11-10T09:32:44Z

pkg/operator/csidriveroperator/driver_starter.go

@@ -218,6 +230,14 @@ func (dsrc *driverStarterCommon) sync(ctx context.Context, syncCtx factory.SyncC
 				return err
 			}
 			if !shouldRun {
+				// If shouldRun is false, then the


This branch will be executed for all CSI drivers that do not belong to the current platform, i.e. for most of these:

cluster-storage-operator/pkg/operator/operator_starter.go

Lines 203 to 214 in 813fcbe

csioperatorclient.GetAWSEBSCSIOperatorConfig(false),

csioperatorclient.GetGCPPDCSIOperatorConfig(),

csioperatorclient.GetOpenStackCinderCSIOperatorConfig(clients, ssr.eventRecorder),

csioperatorclient.GetOVirtCSIOperatorConfig(clients, ssr.eventRecorder),

csioperatorclient.GetManilaOperatorConfig(clients, ssr.eventRecorder),

csioperatorclient.GetVMwareVSphereCSIOperatorConfig(),

csioperatorclient.GetAzureDiskCSIOperatorConfig(),

csioperatorclient.GetAzureFileCSIOperatorConfig(),

csioperatorclient.GetSharedResourceCSIOperatorConfig(false),

csioperatorclient.GetAlibabaDiskCSIOperatorConfig(),

csioperatorclient.GetIBMVPCBlockCSIOperatorConfig(),

csioperatorclient.GetPowerVSBlockCSIOperatorConfig(false),

You should check if this loop actually matched any CSI driver operator and set Upgradeable=true after the loop.

Or, you can always unconditionally create one StorageOperatorrUpgradeable=true. If any ClusterCSIDriver then reports *Upgradeable=False, ClusterOperatorStatusController will do an union where one *Upgradeable=false trumps any number of *Upgradeable=true.

Thanks! I moved the setUpgradeableTrue call to after the loop, if none of these controllers have (ever) started.

I'm hesitant to set Upgradeable=true unconditionally because then in the most common case, where at least one controller is started, we may introduce a brief window of time where it's set to true but will soon change to false. Maybe this is benign, but I would like to avoid that flip-flop and set it only if we know those controllers will not run.

…deable=True

dobsonj · 2023-11-10T22:13:24Z

I built e67877c and pushed the image to quay.io/jdobson/cluster-storage-operator:bug23120-fix2 for testing.

dobsonj · 2023-11-10T22:31:35Z

/label acknowledge-critical-fixes-only
/cherry-pick release-4.14

openshift-cherrypick-robot · 2023-11-10T22:31:37Z

@dobsonj: once the present PR merges, I will cherry-pick it on top of release-4.14 in a new PR and assign it to you.

In response to this:

/label acknowledge-critical-fixes-only
/cherry-pick release-4.14

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/test-infra repository.

openshift-ci-robot · 2023-11-10T22:36:54Z

@dobsonj: This pull request references Jira Issue OCPBUGS-23120, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (wduan@redhat.com), skipping review request.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

https://issues.redhat.com/browse/OCPBUGS-23120

Problem

The upgradeable condition used to be set by SnapshotCRDController, but that was removed in fa9af3a

DefaultStorageClassController only sets upgradeable=True if unsupportedPlatformError.
IBM VPC returns supportedbyCSIError instead.
The current code assumes if supportedbyCSIError, then csidriveroperator controller will eventually set it.

The problem is if shouldRunController returns false for a driver with supportedbyCSIError. Then csidriveroperator does not start and nothing will set the condition. shouldRunController always returns false for IBM ROKS because of this StatusFilter.

Solution

DefaultStorageClassController has no way to tell if shouldRunController will eventually return false. So in this PR, CSIDriverStarter sets the condition if no controller is started, because we know at that point no other controller will set the condition.

/cc @openshift/storage @jeffnowicki

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/test-infra repository.

dobsonj · 2023-11-11T00:22:35Z

Just one failure in e2e-ibmcloud-csi:

: [Jira:"NetworkEdge"] monitor test service-type-load-balancer-availability setup expand_less | 53s
-- | --
{  failed during setup error waiting for replicaset: failed to create RC "service-test": Internal error occurred: resource quota evaluation timed out}

/retest

openshift-ci · 2023-11-11T01:48:21Z

@dobsonj: 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/e2e-ibmcloud-csi	`e67877c`	link	false	`/test e2e-ibmcloud-csi`

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/test-infra repository. I understand the commands that are listed here.

jsafrane · 2023-11-13T10:53:27Z

/lgtm

openshift-ci-robot · 2023-11-13T10:57:32Z

@dobsonj: Jira Issue OCPBUGS-23120: All pull requests linked via external trackers have merged:

openshift/cluster-storage-operator#417

Jira Issue OCPBUGS-23120 has been moved to the MODIFIED state.

In response to this:

https://issues.redhat.com/browse/OCPBUGS-23120

Problem

The upgradeable condition used to be set by SnapshotCRDController, but that was removed in fa9af3a

DefaultStorageClassController only sets upgradeable=True if unsupportedPlatformError.
IBM VPC returns supportedbyCSIError instead.
The current code assumes if supportedbyCSIError, then csidriveroperator controller will eventually set it.

The problem is if shouldRunController returns false for a driver with supportedbyCSIError. Then csidriveroperator does not start and nothing will set the condition. shouldRunController always returns false for IBM ROKS because of this StatusFilter.

Solution

DefaultStorageClassController has no way to tell if shouldRunController will eventually return false. So in this PR, CSIDriverStarter sets the condition if no controller is started, because we know at that point no other controller will set the condition.

/cc @openshift/storage @jeffnowicki

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/test-infra repository.

openshift-cherrypick-robot · 2023-11-13T10:58:19Z

@dobsonj: #417 failed to apply on top of branch "release-4.14":

Applying: OCPBUGS-23120: [IBM ROKS] cluster-storage-operator does not set upgradeable=True
Using index info to reconstruct a base tree...
M	pkg/operator/csidriveroperator/driver_starter.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/operator/csidriveroperator/driver_starter.go
CONFLICT (content): Merge conflict in pkg/operator/csidriveroperator/driver_starter.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 OCPBUGS-23120: [IBM ROKS] cluster-storage-operator does not set upgradeable=True
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/label acknowledge-critical-fixes-only
/cherry-pick release-4.14

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/test-infra repository.

dobsonj · 2023-11-13T16:25:17Z

/jira cherry-pick

openshift-bot · 2023-11-13T17:57:43Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-storage-operator-container-v4.15.0-202311131732.p0.g5d86084.assembly.stream for distgit cluster-storage-operator.
All builds following this will include this PR.

openshift-merge-robot · 2023-11-14T08:00:30Z

Fix included in accepted release 4.15.0-0.nightly-2023-11-14-041944

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 Nov 9, 2023

openshift-ci-robot added jira/severity-critical Referenced Jira bug's severity is critical for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels Nov 9, 2023

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Nov 9, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2023

jsafrane reviewed Nov 10, 2023

View reviewed changes

OCPBUGS-23120: [IBM ROKS] cluster-storage-operator does not set upgra…

e67877c

…deable=True

dobsonj force-pushed the OCPBUGS-23120 branch from c530671 to e67877c Compare November 10, 2023 22:03

openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Nov 10, 2023

dobsonj changed the title ~~WIP: OCPBUGS-23120: [IBM ROKS] cluster-storage-operator does not set upgradeable=True~~ OCPBUGS-23120: [IBM ROKS] cluster-storage-operator does not set upgradeable=True Nov 10, 2023

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 Nov 10, 2023

openshift-ci bot assigned jsafrane Nov 13, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2023

openshift-merge-bot bot merged commit 5d86084 into openshift:master Nov 13, 2023
11 of 12 checks passed

dobsonj mentioned this pull request Nov 13, 2023

[release-4.14] OCPBUGS-23210: [IBM ROKS] cluster-storage-operator does not set upgradeable=True #419

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-23120: [IBM ROKS] cluster-storage-operator does not set upgradeable=True #417

OCPBUGS-23120: [IBM ROKS] cluster-storage-operator does not set upgradeable=True #417

dobsonj commented Nov 9, 2023 •

edited

openshift-ci-robot commented Nov 9, 2023

Problem

Solution

openshift-ci bot commented Nov 9, 2023

Problem

Solution

openshift-ci bot commented Nov 9, 2023

dobsonj commented Nov 10, 2023

jsafrane Nov 10, 2023 •

edited

dobsonj Nov 10, 2023

dobsonj commented Nov 10, 2023 •

edited

dobsonj commented Nov 10, 2023

openshift-cherrypick-robot commented Nov 10, 2023

openshift-ci-robot commented Nov 10, 2023

Problem

Solution

dobsonj commented Nov 11, 2023

openshift-ci bot commented Nov 11, 2023

jsafrane commented Nov 13, 2023

openshift-ci-robot commented Nov 13, 2023

Problem

Solution

openshift-cherrypick-robot commented Nov 13, 2023

dobsonj commented Nov 13, 2023

openshift-bot commented Nov 13, 2023

openshift-merge-robot commented Nov 14, 2023

	csioperatorclient.GetAWSEBSCSIOperatorConfig(false),
	csioperatorclient.GetGCPPDCSIOperatorConfig(),
	csioperatorclient.GetOpenStackCinderCSIOperatorConfig(clients, ssr.eventRecorder),
	csioperatorclient.GetOVirtCSIOperatorConfig(clients, ssr.eventRecorder),
	csioperatorclient.GetManilaOperatorConfig(clients, ssr.eventRecorder),
	csioperatorclient.GetVMwareVSphereCSIOperatorConfig(),
	csioperatorclient.GetAzureDiskCSIOperatorConfig(),
	csioperatorclient.GetAzureFileCSIOperatorConfig(),
	csioperatorclient.GetSharedResourceCSIOperatorConfig(false),
	csioperatorclient.GetAlibabaDiskCSIOperatorConfig(),
	csioperatorclient.GetIBMVPCBlockCSIOperatorConfig(),
	csioperatorclient.GetPowerVSBlockCSIOperatorConfig(false),

OCPBUGS-23120: [IBM ROKS] cluster-storage-operator does not set upgradeable=True #417

OCPBUGS-23120: [IBM ROKS] cluster-storage-operator does not set upgradeable=True #417

Conversation

dobsonj commented Nov 9, 2023 • edited

Problem

Solution

openshift-ci-robot commented Nov 9, 2023

Problem

Solution

openshift-ci bot commented Nov 9, 2023

Problem

Solution

openshift-ci bot commented Nov 9, 2023

dobsonj commented Nov 10, 2023

jsafrane Nov 10, 2023 • edited

Choose a reason for hiding this comment

dobsonj Nov 10, 2023

Choose a reason for hiding this comment

dobsonj commented Nov 10, 2023 • edited

dobsonj commented Nov 10, 2023

openshift-cherrypick-robot commented Nov 10, 2023

openshift-ci-robot commented Nov 10, 2023

Problem

Solution

dobsonj commented Nov 11, 2023

openshift-ci bot commented Nov 11, 2023

jsafrane commented Nov 13, 2023

openshift-ci-robot commented Nov 13, 2023

Problem

Solution

openshift-cherrypick-robot commented Nov 13, 2023

dobsonj commented Nov 13, 2023

openshift-bot commented Nov 13, 2023

openshift-merge-robot commented Nov 14, 2023

dobsonj commented Nov 9, 2023 •

edited

jsafrane Nov 10, 2023 •

edited

dobsonj commented Nov 10, 2023 •

edited