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

Extend node tune controller with status conditions #2165

Merged
merged 1 commit into from
Nov 14, 2024
Merged

Extend node tune controller with status conditions #2165

merged 1 commit into from
Nov 14, 2024

Conversation

rzetelskik
Copy link
Member

@rzetelskik rzetelskik commented Oct 28, 2024 •
edited
Loading

Description of your changes: Node tune controller does not currently set status conditions on NodeConfig's reconciliation. Instead it only set status' NodeStatuses. While this is enough for checking the status programmatically, setting and aggregating status conditions allows for a better UX, by giving the users an option to query NodeConfig conditions.
This PR extends the node tune controller with per-node status conditions and further aggregates them with the existing ones in NodeConfig controller.

It also adds handlers for NodeConfig events to node tune controller and aligns the existing e2e tests with the changes.

Which issue is resolved by this Pull Request:
Resolves #1920

/kind feature
/priority important-longterm
/cc

@scylla-operator-bot scylla-operator-bot
Copy link
Contributor

@rzetelskik: GitHub didn't allow me to request PR reviews from the following users: rzetelskik.

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

In response to this:

Description of your changes: wip

Which issue is resolved by this Pull Request:
Resolves #1920

/kind feature
/priority important-longterm
/cc

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.

@scylla-operator-bot scylla-operator-bot bot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 28, 2024
@rzetelskik rzetelskik force-pushed the nodetune-conditions branch 3 times, most recently from de15257 to a95a864 Compare October 29, 2024 11:03
@rzetelskik rzetelskik changed the title [WIP] Extend node tune controller with status conditions Extend node tune controller with status conditions Oct 29, 2024
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2024
@rzetelskik
Copy link
Member Author

/cc zimnx tnozicka

zimnx
zimnx reviewed Oct 29, 2024
View reviewed changes
Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

first batch

@rzetelskik rzetelskik force-pushed the nodetune-conditions branch 2 times, most recently from d2d0a40 to b188883 Compare October 29, 2024 19:04
@rzetelskik rzetelskik requested a review from zimnx October 29, 2024 19:05
@rzetelskik rzetelskik changed the title Extend node tune controller with status conditions [WIP] Extend node tune controller with status conditions Oct 30, 2024
@scylla-operator-bot scylla-operator-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 30, 2024
@rzetelskik rzetelskik force-pushed the nodetune-conditions branch 6 times, most recently from 2a42a2e to f708e20 Compare October 31, 2024 14:59
@rzetelskik rzetelskik changed the title [WIP] Extend node tune controller with status conditions Extend node tune controller with status conditions Oct 31, 2024
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2024
@rzetelskik rzetelskik changed the title Extend node tune controller with status conditions [WIP] Extend node tune controller with status conditions Nov 5, 2024
@scylla-operator-bot scylla-operator-bot 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 5, 2024
@rzetelskik rzetelskik force-pushed the nodetune-conditions branch 6 times, most recently from f86391a to a0714b1 Compare November 6, 2024 14:55
@rzetelskik rzetelskik changed the title [WIP] Extend node tune controller with status conditions Extend node tune controller with status conditions Nov 6, 2024
@rzetelskik rzetelskik requested a review from tnozicka November 6, 2024 14:56
@scylla-operator-bot scylla-operator-bot 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 6, 2024
@rzetelskik
Copy link
Member Author

@rzetelskik: 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-gke-parallel a0714b1 link true /test e2e-gke-parallel
Full PR test history. Your PR dashboard.

New flake #2185, unrelated to these changes though.

@rzetelskik
Copy link
Member Author

/retest

zimnx
zimnx reviewed Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

copule of comments, rest lgtm

@rzetelskik
Extend node tune controller with status conditions
39cda4b 
Node tune controller does not currently set status conditions on NodeConfig's reconciliation. Instead it only set status' NodeStatuses.
While this is enough for checking the status programmatically, setting and aggregating status conditions allows for a better UX, by giving the users an option to query NodeConfig conditions.
This PR extends the node tune controller with per-node status conditions and further aggregates them with the existing ones in NodeConfig controller.

It also adds handlers for NodeConfig events to node tune controller and aligns the existing e2e tests with the changes.
@rzetelskik rzetelskik requested a review from zimnx November 12, 2024 14:25
zimnx
zimnx approved these changes Nov 12, 2024
View reviewed changes
Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

lgtm
/assign tnozicka

@scylla-operator-bot scylla-operator-bot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2024
@tnozicka
Copy link
Contributor

/approve
/lgtm

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2024
@scylla-operator-bot scylla-operator-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rzetelskik, tnozicka, zimnx

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

@scylla-operator-bot scylla-operator-bot bot merged commit 58104f2 into scylladb:master Nov 14, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimnx zimnx zimnx approved these changes

@tnozicka tnozicka Awaiting requested review from tnozicka

Assignees

@tnozicka tnozicka

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extend node tune controller with status conditions
3 participants
@rzetelskik @tnozicka @zimnx