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.

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

Add priority classes to builds based on change reason #908

Merged
merged 5 commits into from
Apr 4, 2022

Conversation

sambhav
Copy link
Contributor

@sambhav sambhav commented Dec 24, 2021

Signed-off-by: Sambhav Kothari skothari44@bloomberg.net

Fixes #774

This PR adds priority class names to build pods based on the change that triggered it. For user triggered changes (changes in config, source resolver, manual trigger) - the pod is created with a high priority. For operator based changes (stack/buildpack change) it is created with low priority.

The whole feature is currently gated behind a flag which is set to false by default.

The way this works is that based on the build change, an annotation is added to the build with the priority class, which the build adds directly to the pod's priorityclass name. I also added some default priority classes which match the classes that kpack expects, but users are free to update these priority classes to match their cluster configurations if they need to.

The high priority class is currently hard coded to kpack-build-high-priority and similarly kpack-build-low-priority for the low priority class.

@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2021

Codecov Report

Merging #908 (bff003a) into main (b66668e) will increase coverage by 0.03%.
The diff coverage is 68.18%.

@@            Coverage Diff             @@
##             main     #908      +/-   ##
==========================================
+ Coverage   69.75%   69.79%   +0.03%     
==========================================
  Files         119      120       +1     
  Lines        5423     5452      +29     
==========================================
+ Hits         3783     3805      +22     
- Misses       1271     1276       +5     
- Partials      369      371       +2     
Impacted Files Coverage Δ
pkg/apis/build/v1alpha2/build_priority.go 0.00% <0.00%> (ø)
pkg/apis/build/v1alpha2/build_types.go 100.00% <ø> (ø)
pkg/reconciler/image/image.go 50.65% <0.00%> (-0.34%) ⬇️
pkg/apis/build/v1alpha2/build.go 32.35% <50.00%> (+0.72%) ⬆️
pkg/reconciler/image/reconcile_build.go 86.95% <50.00%> (-1.81%) ⬇️
pkg/apis/build/v1alpha2/build_pod.go 98.20% <100.00%> (+<0.01%) ⬆️
pkg/apis/build/v1alpha2/image_builds.go 69.32% <100.00%> (+0.18%) ⬆️
pkg/buildchange/buildpack_change.go 81.25% <100.00%> (+1.25%) ⬆️
pkg/buildchange/change.go 100.00% <100.00%> (ø)
pkg/buildchange/change_processor.go 84.00% <100.00%> (+3.04%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b66668e...bff003a. Read the comment docs.

@sambhav sambhav force-pushed the preemption branch 2 times, most recently from f0f623c to d6dd824 Compare December 24, 2021 22:02
Copy link

@AidanDelaney AidanDelaney left a comment

Choose a reason for hiding this comment

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

I have one question, otherwise LGTM.

config/controller.yaml Outdated Show resolved Hide resolved
Copy link

@AidanDelaney AidanDelaney left a comment

Choose a reason for hiding this comment

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

@sambhav sambhav force-pushed the preemption branch 2 times, most recently from cae17ca to aecaab7 Compare January 20, 2022 18:33
@sambhav sambhav requested a review from tylerphelan January 20, 2022 18:33
@sambhav
Copy link
Contributor Author

sambhav commented Jan 20, 2022

@matthewmcnew @tylerphelan @tomkennedy513 could you please review?

@sambhav
Copy link
Contributor Author

sambhav commented Feb 1, 2022

@matthewmcnew updated to a spec field in commit 58bf3fc

It should not be a blocker to this PR but I can't get openapi gen to work correctly and it is producing a large diff. Is there a specific version I should use?

@sambhav sambhav force-pushed the preemption branch 2 times, most recently from 58bf3fc to c2d0377 Compare February 1, 2022 11:53
@dumez-k dumez-k self-requested a review February 1, 2022 15:19
@matthewmcnew matthewmcnew self-requested a review February 1, 2022 15:20
@tomkennedy513 tomkennedy513 self-requested a review February 1, 2022 15:20
@tylerphelan
Copy link
Contributor

@samj1912

It should not be a blocker to this PR but I can't get openapi gen to work correctly and it is producing a large diff. Is there a specific version I should use?

Get correct version with:

go get k8s.io/kube-openapi/cmd/openapi-gen@release-1.19

sambhav added 2 commits March 8, 2022 13:27
Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@sambhav sambhav force-pushed the preemption branch 2 times, most recently from 832bfbb to 1dc2f63 Compare March 8, 2022 13:31
@sambhav
Copy link
Contributor Author

sambhav commented Mar 8, 2022

@tylerphelan thanks for that. codegen and openapigen should both be in sync now. I noticed some of the codegen changes were not applied from previous PRs so updated those as well.

@sambhav sambhav force-pushed the preemption branch 2 times, most recently from 832bfbb to 3b4df7a Compare March 8, 2022 18:19
Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@tylerphelan
Copy link
Contributor

code gen seems to be correct, I think we can merge

@sambhav
Copy link
Contributor Author

sambhav commented Mar 31, 2022

@tylerphelan tylerphelan merged commit cecb337 into buildpacks-community:main Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kpack operator triggered rebuilds lead to cluster starvation
6 participants