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.

Sign up for GitHub

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

Jump to bottom

Bug 1909793: Backport: [Release 4.6] adds tunefastdeviceclass property #1019

Merged
merged 3 commits into from Feb 8, 2021
Merged

Bug 1909793: Backport: [Release 4.6] adds tunefastdeviceclass property #1019

merged 3 commits into from Feb 8, 2021

Conversation

crombus
Copy link
Contributor

@crombus crombus commented Jan 29, 2021

No description provided.

@openshift-ci-robot
Copy link

@crombus: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

Release 4.6

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

crombus commented Jan 29, 2021

/hold until the decision is made.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2021
@crombus crombus changed the title Release 4.6 Backport: [Release 4.6] adds tunefastdeviceclass property Jan 29, 2021
@openshift-ci-robot
Copy link

@crombus: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

Backport: [Release 4.6] adds tunefastdeviceclass property

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 crombus changed the title Backport: [Release 4.6] adds tunefastdeviceclass property Bug 1909793: Backport: [Release 4.6] adds tunefastdeviceclass property Feb 1, 2021
@openshift-ci-robot openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Feb 1, 2021
@openshift-ci-robot
Copy link

@crombus: This pull request references Bugzilla bug 1909793, which is invalid:

  • expected the bug to target the "OCS 4.6.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1909793: Backport: [Release 4.6] adds tunefastdeviceclass property

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

crombus commented Feb 1, 2021

Dependent on #1021

@agarwal-mudit
Copy link
Member

/bugzilla refresh

@openshift-ci-robot
Copy link

@agarwal-mudit: This pull request references Bugzilla bug 1909793, which is invalid:

  • expected the bug to target the "OCS 4.6.0" release, but it targets "OCS 4.6.3" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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 openshift-ci-robot mentioned this pull request Feb 5, 2021
Merged
@agarwal-mudit
Copy link
Member

/retest

@obnoxxx
Copy link
Contributor

obnoxxx commented Feb 5, 2021

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 5, 2021
@obnoxxx
Copy link
Contributor

obnoxxx commented Feb 5, 2021

/bugzilla refresh

@openshift-ci-robot
Copy link

@obnoxxx: This pull request references Bugzilla bug 1909793, which is invalid:

  • expected the bug to target the "OCS 4.6.0" release, but it targets "OCS 4.6.3" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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.

@obnoxxx obnoxxx added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Feb 5, 2021
@crombus
Copy link
Contributor Author

crombus commented Feb 5, 2021

/test ocs-operator-ci

@obnoxxx
Copy link
Contributor

obnoxxx commented Feb 5, 2021

/override ci/prow/red-hat-storage-ocs-ci-e2e-aws

@openshift-ci-robot
Copy link

@obnoxxx: Overrode contexts on behalf of obnoxxx: ci/prow/red-hat-storage-ocs-ci-e2e-aws

In response to this:

/override ci/prow/red-hat-storage-ocs-ci-e2e-aws

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.

obnoxxx
obnoxxx requested changes Feb 5, 2021
View reviewed changes
Copy link
Contributor

@obnoxxx obnoxxx left a 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...

@jarrpa

pkg/controller/storagecluster/reconcile.go Outdated Show resolved Hide resolved
pkg/controller/storagecluster/reconcile.go Outdated Show resolved Hide resolved
pkg/controller/storagecluster/storagecluster_controller_test.go
@@ -163,6 +163,55 @@ func TestNonWatchedResourceNameNotFound(t *testing.T) {
assert.Equal(t, reconcile.Result{}, result)
}

func TestThrottleStorageDevices(t *testing.T) {
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2994cee
found the commit

Copy link
Contributor

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.

@obnoxxx
Copy link
Contributor

obnoxxx commented Feb 6, 2021

JFTR, @jarrpa agreed to require making this backport cleaner. So I'll leave the change request. @crombus

@crombus
add new property TuneFastDeviceClass
7ed716f 
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)
@crombus
test: update TestThrottleStorageDevices
06aa621 
add case for fasttune device

Signed-off-by: crombus <pkundra@redhat.com>
(cherry picked from commit 5386188)
@crombus
update crd changes
db4359a 
Signed-off-by: crombus <pkundra@redhat.com>
(cherry picked from commit 2e8b7aa)
obnoxxx
obnoxxx approved these changes Feb 8, 2021
View reviewed changes
Copy link
Contributor

@obnoxxx obnoxxx left a 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

@openshift-ci-robot
Copy link

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

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 8, 2021
@obnoxxx
Copy link
Contributor

obnoxxx commented Feb 8, 2021

/override ci/prow/red-hat-storage-ocs-ci-e2e-aws

@openshift-ci-robot
Copy link

@obnoxxx: Overrode contexts on behalf of obnoxxx: ci/prow/red-hat-storage-ocs-ci-e2e-aws

In response to this:

/override ci/prow/red-hat-storage-ocs-ci-e2e-aws

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

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit d7d1377 into red-hat-storage:release-4.6 Feb 8, 2021
@openshift-ci-robot
Copy link

@crombus: All pull requests linked via external trackers have merged:

Bugzilla bug 1909793 has been moved to the MODIFIED state.

In response to this:

Bug 1909793: Backport: [Release 4.6] adds tunefastdeviceclass property

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 openshift-ci-robot mentioned this pull request Feb 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obnoxxx obnoxxx obnoxxx approved these changes

@jarrpa jarrpa Awaiting requested review from jarrpa

@umangachapagain umangachapagain Awaiting requested review from umangachapagain

Assignees

@obnoxxx obnoxxx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@crombus @openshift-ci-robot @agarwal-mudit @obnoxxx @openshift-bot @openshift-merge-robot