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

balloons: improve dynamic configuration update #830

Merged
merged 3 commits into from Jun 20, 2022

Conversation

askervin
Copy link
Contributor

  • Recreate balloons and reassign workloads only if necessary.
  • CPU class of balloon types can be switched without reassigning
    workloads.

Copy link
Contributor

@klihub klihub left a comment

Choose a reason for hiding this comment

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

@askervin This needs a few minor touches to make gofmt happy.

@askervin
Copy link
Contributor Author

@askervin This needs a few minor touches to make gofmt happy.

I don't get gofmt complaints, and reformatting every file does not change anything... Let's see if our environments differ...

@klihub
Copy link
Contributor

klihub commented Jun 16, 2022

@askervin This needs a few minor touches to make gofmt happy.

I don't get gofmt complaints, and reformatting every file does not change anything... Let's see if our environments differ...

In this case you don't even need to check it locally, there are only a very few related to for i := being preferred over for i, _ := .

You can check the logs for the failed verify workflow. Or you can cherry-pick this fixup commit from my test tree and squash into your original one in this PR.

@klihub
Copy link
Contributor

klihub commented Jun 17, 2022

And here is another patch I had to roll to fix up the unit tests:
0001-balloons-adjust-unit-tests-for-update-option-types.patch.gz
.

askervin and others added 2 commits June 17, 2022 15:10
- Recreate balloons and reassign workloads only if necessary.
- CPU class of balloon types can be switched without reassigning
  workloads.
@askervin askervin force-pushed the 5JD_balloons_config_updates branch from 2bd4f52 to 0c5f8f0 Compare June 17, 2022 12:15
@askervin
Copy link
Contributor Author

Wow, thanks @klihub
gofmt should be now happy. Applied your unit test fix as a separate commit in this PR.

@codecov-commenter
Copy link

Codecov Report

Merging #830 (e910812) into master (2930193) will decrease coverage by 2.64%.
The diff coverage is 26.78%.

@@            Coverage Diff             @@
##           master     #830      +/-   ##
==========================================
- Coverage   37.58%   34.94%   -2.65%     
==========================================
  Files          55       60       +5     
  Lines        8138     8858     +720     
==========================================
+ Hits         3059     3095      +36     
- Misses       4783     5461     +678     
- Partials      296      302       +6     
Impacted Files Coverage Δ
...manager/policy/builtin/balloons/balloons-policy.go 2.04% <21.73%> (ø)
.../resource-manager/policy/builtin/balloons/flags.go 46.42% <50.00%> (ø)
...cy/builtin/topology-aware/topology-aware-policy.go 15.96% <0.00%> (-0.11%) ⬇️
pkg/cri/resource-manager/cache/container.go 17.33% <0.00%> (-0.11%) ⬇️
pkg/apis/resmgr/expression.go 53.79% <0.00%> (ø)
...rce-manager/policy/builtin/topology-aware/flags.go 68.75% <0.00%> (ø)
...urce-manager/policy/builtin/balloons/fillmethod.go 0.00% <0.00%> (ø)
...-manager/policy/builtin/topology-aware/affinity.go 35.13% <0.00%> (ø)
...esource-manager/policy/builtin/balloons/metrics.go 0.00% <0.00%> (ø)
... and 3 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Contributor

@klihub klihub left a comment

Choose a reason for hiding this comment

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

LGTM

@klihub klihub merged commit 889166f into intel:master Jun 20, 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.

None yet

3 participants