-
Notifications
You must be signed in to change notification settings - Fork 177
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
Bug 1909793: Backport: [Release 4.6] adds tunefastdeviceclass property #1019
Bug 1909793: Backport: [Release 4.6] adds tunefastdeviceclass property #1019
Conversation
@crombus: No Bugzilla bug is referenced in the title of this pull request. In response to this:
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. |
/hold until the decision is made. |
@crombus: No Bugzilla bug is referenced in the title of this pull request. In response to this:
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. |
@crombus: This pull request references Bugzilla bug 1909793, which is invalid:
Comment In response to this:
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. |
Dependent on #1021 |
/bugzilla refresh |
@agarwal-mudit: This pull request references Bugzilla bug 1909793, which is invalid:
Comment In response to this:
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. |
/retest |
/hold cancel |
/bugzilla refresh |
@obnoxxx: This pull request references Bugzilla bug 1909793, which is invalid:
Comment In response to this:
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. |
/test ocs-operator-ci |
/override ci/prow/red-hat-storage-ocs-ci-e2e-aws |
@obnoxxx: Overrode contexts on behalf of obnoxxx: ci/prow/red-hat-storage-ocs-ci-e2e-aws In response to this:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few hunks that don't belong to the original commit. Not sure we should merge like this - mainly in the test code so maybe?...
Prelimiary request changes...
@@ -163,6 +163,55 @@ func TestNonWatchedResourceNameNotFound(t *testing.T) { | |||
assert.Equal(t, reconcile.Result{}, result) | |||
} | |||
|
|||
func TestThrottleStorageDevices(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is actually added in previous commits. Shouldn't we cherry-pick those commits separately? The original commit had only small modifications to the test function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and in release-4.6 the test function is not there https://github.com/openshift/ocs-operator/blob/release-4.6/pkg/controller/storagecluster/storagecluster_controller_test.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the commit which adds the test is lost after the sdk update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2994cee
found the commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit is not lost, but it is just on a different file than the test is in the current master: 2994cee
But this is actually too much different context.
I guess for this backport, it is ok to just add the function like this.
It could have been added as a separate commit to keep the actual commit to add TuneFastDeviceClass closer to the original, but I won't insist.
move throttleStorageDevices func in cephcluster.go and add managed-premium as fastdeviceclass. modify test file according to the changes. Signed-off-by: crombus <pkundra@redhat.com> (cherry picked from commit 2d9deab)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks clean now, thanks for the update, @crombus .
As discussed, we can add the test function like this, because the history is too convoluted.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: obnoxxx 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 |
/override ci/prow/red-hat-storage-ocs-ci-e2e-aws |
@obnoxxx: Overrode contexts on behalf of obnoxxx: ci/prow/red-hat-storage-ocs-ci-e2e-aws In response to this:
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. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@crombus: All pull requests linked via external trackers have merged: Bugzilla bug 1909793 has been moved to the MODIFIED state. In response to this:
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. |
No description provided.