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

Add opt-in P&F configs for Visibility API and reference from documentation #3043

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Add opt-in P&F configs for Visibility API and reference from documentation #3043

merged 3 commits into from
Sep 20, 2024

Conversation

mbobrovskyi
Copy link
Contributor

@mbobrovskyi mbobrovskyi commented Sep 13, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add FlowSchema and PriorityLevelConfiguration for Visibility API.

Which issue(s) this PR fixes:

Part of #2973.

Special notes for your reviewer:

kubectl get --raw /debug/api_priority_and_fairness/dump_priority_levels 
PriorityLevelName, ActiveQueues, IsIdle, IsQuiescing, WaitingRequests, ExecutingRequests, DispatchedRequests, RejectedRequests, TimedoutRequests, CancelledRequests
catch-all,         0,            true,   false,       0,               0,                 41,                 0,                0,                0
exempt,            0,            true,   false,       0,               0,                 573,                0,                0,                0
global-default,    0,            false,  false,       0,               1,                 287,                0,                0,                0
kueue-visibility,  0,            true,   false,       0,               0,                 3547,               0,                0,                0
leader-election,   0,            true,   false,       0,               0,                 90,                 0,                0,                0
node-high,         0,            true,   false,       0,               0,                 68,                 0,                0,                0
system,            0,            true,   false,       0,               0,                 300,                0,                0,                0
workload-high,     0,            true,   false,       0,               0,                 623,                0,                0,                0
workload-low,      0,            true,   false,       0,               0,                 380,                0,                0,                0

DispatchedRequests for kueue-visibility increased. That's mean that we are using kueue-visibility PriorityLevel instead global-default.

Does this PR introduce a user-facing change?

Added FlowSchema and PriorityLevelConfiguration for Visibility API.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 13, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 13, 2024
@netlify Netlify
Copy link

netlify bot commented Sep 13, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 390a9b0
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/66ed6747a6425200093f2779

@mbobrovskyi
Copy link
Contributor Author

/cc @mimowo @tenzen-y

mimowo
mimowo reviewed Sep 13, 2024
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment

Choose a reason for hiding this comment

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

/lgtm
Please add "Part of #2973" in the description for discoverability.
Leaving approve to Yuki

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2a849f66d8cd016b5f752a44c6b887bb4f03525a

@mimowo
Copy link
Contributor

mimowo commented Sep 13, 2024

Please also confirm by a manual test that it works fine e2e before we merge
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 13, 2024
@mbobrovskyi
Copy link
Contributor Author

Ah, this feature available from v1.29 https://kubernetes.io/docs/concepts/cluster-administration/flow-control/.

@mimowo
Copy link
Contributor

mimowo commented Sep 13, 2024

It is in Beta (enabled by default) since 1.20 apparently: https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/1040-priority-and-fairness/kep.yaml. But maybe the API version is different?

@mbobrovskyi
Copy link
Contributor Author

mbobrovskyi commented Sep 13, 2024 •
edited
Loading

Not sure how we can fix it... I propose to add a section on the docs. Just one kubectl apply -f https://.... And say that if you use <1.29, then use older version. It will be easy to use by users. And after release k8s 1.32 we can add it.

@mimowo @tenzen-y WDYT?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2024
@k8s-ci-robot k8s-ci-robot requested a review from mimowo September 13, 2024 17:52
@mbobrovskyi
Copy link
Contributor Author

mbobrovskyi commented Sep 13, 2024 •
edited
Loading

Please also confirm by a manual test that it works fine e2e before we merge

Yes, sure.

kubectl get --raw /debug/api_priority_and_fairness/dump_priority_levels 
PriorityLevelName, ActiveQueues, IsIdle, IsQuiescing, WaitingRequests, ExecutingRequests, DispatchedRequests, RejectedRequests, TimedoutRequests, CancelledRequests
catch-all,         0,            true,   false,       0,               0,                 41,                 0,                0,                0
exempt,            0,            true,   false,       0,               0,                 573,                0,                0,                0
global-default,    0,            false,  false,       0,               1,                 287,                0,                0,                0
kueue-visibility,  0,            true,   false,       0,               0,                 3547,               0,                0,                0
leader-election,   0,            true,   false,       0,               0,                 90,                 0,                0,                0
node-high,         0,            true,   false,       0,               0,                 68,                 0,                0,                0
system,            0,            true,   false,       0,               0,                 300,                0,                0,                0
workload-high,     0,            true,   false,       0,               0,                 623,                0,                0,                0
workload-low,      0,            true,   false,       0,               0,                 380,                0,                0,                0

DispatchedRequests for kueue-visibility increased. That's mean that we are using kueue-visibility PriorityLevel instead global-default.

@mbobrovskyi
Copy link
Contributor Author

mbobrovskyi commented Sep 13, 2024 •
edited
Loading

Please also confirm by a manual test that it works fine e2e before we merge

Also I've tested with helm. It's working fine too.

kubectl get --raw /debug/api_priority_and_fairness/dump_priority_levels 
PriorityLevelName, ActiveQueues, IsIdle, IsQuiescing, WaitingRequests, ExecutingRequests, DispatchedRequests, RejectedRequests, TimedoutRequests, CancelledRequests
catch-all,         0,            true,   false,       0,               0,                 36,                 0,                0,                0
exempt,            0,            true,   false,       0,               0,                 534,                0,                0,                0
global-default,    0,            false,  false,       0,               1,                 228,                0,                0,                0
kueue-visibility,  0,            false,  false,       0,               5,                 981,                0,                0,                0
leader-election,   0,            true,   false,       0,               0,                 73,                 0,                0,                0
node-high,         0,            true,   false,       0,               0,                 19,                 0,                0,                0
system,            0,            true,   false,       0,               0,                 131,                0,                0,                0
workload-high,     0,            true,   false,       0,               0,                 464,                0,                0,                0
workload-low,      0,            true,   false,       0,               0,                 159,                0,                0,                0

@mbobrovskyi mbobrovskyi mentioned this pull request Sep 13, 2024
Open
3 tasks
@mimowo
Copy link
Contributor

mimowo commented Sep 16, 2024

/hold
For #3043 (comment)

@mbobrovskyi mbobrovskyi mentioned this pull request Sep 18, 2024
Closed
3 tasks
@mbobrovskyi mbobrovskyi force-pushed the feature/add-flow-schema-and-priority-level-configuration-for-visibility-api branch from fe6682a to 1355dcc Compare September 18, 2024 14:35
@mimowo
Copy link
Contributor

mimowo commented Sep 19, 2024

Still
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 94a21547030574aebf37ab0e65c152d499608082

@mbobrovskyi
Copy link
Contributor Author

/assign @tenzen-y

@tenzen-y
Copy link
Member

(2.) is a new feature. However, I don't think it is necessary, and I would prefer not to install the configuration via helm, if we don't install it in released manifets. cc @tenzen-y

@mimowo I'm ok with the removal of APF configurations from Helm Chart.
But, my root question is what is supporting scope of Helm Chart? We used to provide and support the same functionality between kustomize manifests and Helm Chart. Here, are there any justifications the reason what we can drop the APF configurations from Helm Chart?

@mbobrovskyi
Copy link
Contributor Author

mbobrovskyi commented Sep 20, 2024 •
edited
Loading

(2.) is a new feature. However, I don't think it is necessary, and I would prefer not to install the configuration via helm, if we don't install it in released manifets. cc @tenzen-y

@mimowo I'm ok with the removal of APF configurations from Helm Chart. But, my root question is what is supporting scope of Helm Chart? We used to provide and support the same functionality between kustomize manifests and Helm Chart. Here, are there any justifications the reason what we can drop the APF configurations from Helm Chart?

We shouldn't install it by default. But we can add some variable that allow user to setup this configuration optionally on helm. WDYT?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 20, 2024
@mbobrovskyi mbobrovskyi force-pushed the feature/add-flow-schema-and-priority-level-configuration-for-visibility-api branch from fb40ba8 to 0e05e8b Compare September 20, 2024 04:24
@mimowo
Copy link
Contributor

mimowo commented Sep 20, 2024

We used to provide and support the same functionality between kustomize manifests and Helm Chart. Here, are there any justifications the reason what we can drop the APF configurations from Helm Chart?

Right, since we decided not to provide the P&F configs in the main manifest for 0.9 we shouldn't install the configs by default by Helm charts, for consistency (same functionality).

But we can add some variable that allow user to setup this configuration optionally on helm. WDYT?

I like this idea, and see we have an analog for prometheus like here.

Maybe we have a variable like enableVisibilityAPF, disabled by default in 0.9. WDYT @tenzen-y @mbobrovskyi ?

@tenzen-y
Copy link
Member

We used to provide and support the same functionality between kustomize manifests and Helm Chart. Here, are there any justifications the reason what we can drop the APF configurations from Helm Chart?

Right, since we decided not to provide the P&F configs in the main manifest for 0.9 we shouldn't install the configs by default by Helm charts, for consistency (same functionality).

But we can add some variable that allow user to setup this configuration optionally on helm. WDYT?

I like this idea, and see we have an analog for prometheus like here.

Maybe we have a variable like enableVisibilityAPF, disabled by default in 0.9. WDYT @tenzen-y @mbobrovskyi ?

SGTM

@mbobrovskyi mbobrovskyi force-pushed the feature/add-flow-schema-and-priority-level-configuration-for-visibility-api branch 2 times, most recently from ec7891d to 024379f Compare September 20, 2024 12:13
@mbobrovskyi mbobrovskyi force-pushed the feature/add-flow-schema-and-priority-level-configuration-for-visibility-api branch from 024379f to 390a9b0 Compare September 20, 2024 12:15
@mbobrovskyi
Copy link
Contributor Author

Maybe we have a variable like enableVisibilityAPF, disabled by default in 0.9. WDYT @tenzen-y @mbobrovskyi ?

Done

mimowo
mimowo reviewed Sep 20, 2024
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment

Choose a reason for hiding this comment

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

LGTM, but leaving final tagging to @tenzen-y

tenzen-y
tenzen-y reviewed Sep 20, 2024
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

This looks great!
Thank you for doing this!
/approve
/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 20, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: f7991034cbda4698ca556ad58a28d83abb974ae3

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mbobrovskyi, mimowo, tenzen-y

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

@k8s-ci-robot k8s-ci-robot merged commit f5e245f into kubernetes-sigs:main Sep 20, 2024
16 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.9 milestone Sep 20, 2024
@mbobrovskyi mbobrovskyi deleted the feature/add-flow-schema-and-priority-level-configuration-for-visibility-api branch September 21, 2024 05:45
@mbobrovskyi mbobrovskyi mentioned this pull request Sep 23, 2024
Merged
kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024
@mbobrovskyi @kannon92
Add opt-in P&F configs for Visibility API and reference from document…
4c5299b 
…ation (kubernetes-sigs#3043)

* Add opt-in P&F configs for Visibility API and reference from documentation.

* Add API Priority and Fairness section on installation guide.

* Add helm charts configuration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tenzen-y tenzen-y tenzen-y left review comments

@mimowo mimowo mimowo left review comments

@alculquicondor alculquicondor Awaiting requested review from alculquicondor

@denkensk denkensk Awaiting requested review from denkensk

Assignees

@mimowo mimowo

@tenzen-y tenzen-y

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.9
Development

Successfully merging this pull request may close these issues.
4 participants
@mbobrovskyi @k8s-ci-robot @mimowo @tenzen-y