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 2087684: Support for rejecting transition from/to extreme latency profiles #1372

Merged
merged 3 commits into from Jun 22, 2022
Merged

Bug 2087684: Support for rejecting transition from/to extreme latency profiles #1372

merged 3 commits into from Jun 22, 2022

Conversation

swghosh
Copy link
Member

@swghosh swghosh commented May 25, 2022 •
edited

Worker Latency Profile(s) support for rejecting updates between extreme profiles (Default <-> Low), in favour of cluster stability as described in the EP [1][2].

  • use slice of suppressor fn(s) in NewLatencyProfileObserver
  • add new suppressor fn, NewSuppressConfigUpdateForExtremeProfilesFunc which suppresses updates when switching from Default -> Low or Low -> Default
  • add checkRejectFn in controller to update status when rejection occurs

Helps to fix, xref: https://bugzilla.redhat.com/show_bug.cgi?id=2087684

Signed-off-by: Swarup Ghosh swghosh@redhat.com

@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 May 25, 2022
@openshift-ci openshift-ci bot requested review from harche and sairameshv May 25, 2022 09:15
@swghosh swghosh force-pushed the reject-extreme-latency-profiles branch from f8ba295 to 314a905 Compare May 25, 2022 12:44
@swghosh swghosh changed the title WIP: Reject transition from/to extreme latency profiles Reject transition from/to extreme latency profiles May 25, 2022
@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 May 25, 2022
@swghosh swghosh changed the title Reject transition from/to extreme latency profiles Support for rejecting transition from/to extreme latency profiles May 25, 2022
@swghosh
Copy link
Member Author

swghosh commented May 25, 2022

This is required for https://bugzilla.redhat.com/show_bug.cgi?id=2087684 and corresponding PR in KCMO.

@swghosh swghosh mentioned this pull request May 25, 2022
Merged
@swghosh swghosh force-pushed the reject-extreme-latency-profiles branch from 314a905 to b802bfa Compare May 25, 2022 18:24
@swghosh swghosh changed the title Support for rejecting transition from/to extreme latency profiles WIP: Support for rejecting transition from/to extreme latency profiles May 26, 2022
@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 May 26, 2022
@swghosh swghosh force-pushed the reject-extreme-latency-profiles branch 2 times, most recently from 18d590b to c0a81bb Compare May 26, 2022 09:13
@swghosh swghosh changed the title WIP: Support for rejecting transition from/to extreme latency profiles Support for rejecting transition from/to extreme latency profiles May 26, 2022
@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 May 26, 2022
@swghosh swghosh force-pushed the reject-extreme-latency-profiles branch 2 times, most recently from 17df1db to 594ea24 Compare May 26, 2022 11:49
@swghosh
Copy link
Member Author

swghosh commented May 26, 2022

/assign @p0lyn0mial @atiratree

@swghosh
Copy link
Member Author

swghosh commented May 26, 2022

/cc @soltysh @rphillips

@openshift-ci openshift-ci bot requested review from rphillips and soltysh May 26, 2022 11:57
@swghosh
Nit: rm unused cmLister, latencyConfigs in NewLatencyProfileController
2a5a246 
Signed-off-by: Swarup Ghosh <swghosh@redhat.com>
@swghosh
Refactor: move latencyprofileobserver supressors to separate file
c6dce2a 
Signed-off-by: Swarup Ghosh <swghosh@redhat.com>
@swghosh swghosh force-pushed the reject-extreme-latency-profiles branch 2 times, most recently from 6fdb07d to 675a3a8 Compare May 27, 2022 09:22
atiratree
atiratree reviewed May 27, 2022
View reviewed changes
pkg/operator/configobserver/node/suppress_config_updates.go Show resolved Hide resolved
pkg/operator/configobserver/node/suppress_config_updates.go Outdated Show resolved Hide resolved
pkg/operator/configobserver/node/suppress_config_updates.go Outdated Show resolved Hide resolved
pkg/operator/latencyprofilecontroller/profilerejectionchecker.go Outdated Show resolved Hide resolved
pkg/operator/configobserver/node/observe_latency_profile.go Outdated Show resolved Hide resolved
pkg/operator/latencyprofilecontroller/profilerejectionchecker.go Outdated Show resolved Hide resolved
pkg/operator/latencyprofilecontroller/controller.go Outdated Show resolved Hide resolved
pkg/operator/latencyprofilecontroller/profilerejectionchecker_test.go Outdated Show resolved Hide resolved
@swghosh swghosh force-pushed the reject-extreme-latency-profiles branch 3 times, most recently from df77f71 to df941d4 Compare June 17, 2022 09:59
@swghosh swghosh force-pushed the reject-extreme-latency-profiles branch 4 times, most recently from feed1c1 to 20c0270 Compare June 21, 2022 08:33
atiratree
atiratree reviewed Jun 21, 2022
View reviewed changes
Copy link
Member

@atiratree atiratree left a comment

Choose a reason for hiding this comment

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

also this PR should have a bug id in the title no?

pkg/operator/configobserver/node/observe_latency_profile.go Show resolved Hide resolved
pkg/operator/latencyprofilecontroller/profilerejectionchecker.go Show resolved Hide resolved
@swghosh swghosh force-pushed the reject-extreme-latency-profiles branch 2 times, most recently from 6ff925f to f3f459c Compare June 21, 2022 17:01
@swghosh
Changes to latency-profile-observer,controller
5fd20f8 
- use a slice for suppress fns in NewLatencyProfileObserver
- add new suppressor fn: NewSuppressConfigUpdateForExtremeProfilesFunc
- add checkRejectFn in controller to update status when rejection occurs

Signed-off-by: Swarup Ghosh <swghosh@redhat.com>
@swghosh swghosh force-pushed the reject-extreme-latency-profiles branch from f3f459c to 5fd20f8 Compare June 22, 2022 04:44
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 22, 2022

@swghosh: all tests passed!

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.

@swghosh swghosh changed the title Support for rejecting transition from/to extreme latency profiles Bug 2087684: Support for rejecting transition from/to extreme latency profiles Jun 22, 2022
@openshift-ci openshift-ci bot added 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. labels Jun 22, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 22, 2022

@swghosh: This pull request references Bugzilla bug 2087684, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.11.0) matches configured target release for branch (4.11.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

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

In response to this:

Bug 2087684: Support for rejecting transition from/to extreme latency profiles

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

openshift-ci bot commented Jun 22, 2022

@swghosh: This pull request references Bugzilla bug 2087684, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.11.0) matches configured target release for branch (4.11.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

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

In response to this:

Bug 2087684: Support for rejecting transition from/to extreme latency profiles

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.

@atiratree
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, swghosh

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 Jun 22, 2022
@atiratree
Copy link
Member

/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 Jun 22, 2022
@openshift-ci openshift-ci bot merged commit 84d884f into openshift:master Jun 22, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 22, 2022

@swghosh: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with /bugzilla refresh.

Bugzilla bug 2087684 has not been moved to the MODIFIED state.

In response to this:

Bug 2087684: Support for rejecting transition from/to extreme latency profiles

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.

@atiratree
Copy link
Member

/hold cancel

@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 Jun 22, 2022
swghosh added a commit to swghosh/cluster-kube-apiserver-operator that referenced this pull request Jun 23, 2022
@swghosh
Vendor changes from bumped openshift/library-go#1372
f221855 
Signed-off-by: Swarup Ghosh <swghosh@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atiratree atiratree atiratree left review comments

@sairameshv sairameshv sairameshv left review comments

@harche harche Awaiting requested review from harche

@rphillips rphillips Awaiting requested review from rphillips

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@atiratree atiratree

@p0lyn0mial p0lyn0mial

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
4 participants
@swghosh @atiratree @sairameshv @p0lyn0mial