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

Integrate with goresctrl #711

Merged
merged 3 commits into from
Sep 29, 2021
Merged

Integrate with goresctrl #711

merged 3 commits into from
Sep 29, 2021

Conversation

jukkar
Copy link
Contributor

@jukkar jukkar commented Sep 22, 2021 •
edited
Loading

This depends on the changes in intel/goresctrl#75

@jukkar jukkar marked this pull request as ready for review September 24, 2021 07:45
@jukkar
Copy link
Contributor Author

jukkar commented Sep 24, 2021

  • fixed gofmt errors
  • removed remaining sst files

@jukkar
Copy link
Contributor Author

jukkar commented Sep 27, 2021

  • Fixed various linter errors
  • Use goresctrl v0.2.0

@jukkar jukkar force-pushed the integrate-with-goresctrl branch 2 times, most recently from a889386 to 88db640 Compare September 27, 2021 14:13
@codecov-commenter
Copy link

Codecov Report

Merging #711 (88db640) into master (4cec89f) will decrease coverage by 0.05%.
The diff coverage is 52.99%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
- Coverage   37.70%   37.64%   -0.06%     
==========================================
  Files          53       53              
  Lines        7877     7889      +12     
==========================================
  Hits         2970     2970              
- Misses       4631     4643      +12     
  Partials      276      276
Impacted Files Coverage Δ
pkg/cri/resource-manager/cache/cache.go 22.16% <ø> (ø)
...manager/policy/builtin/podpools/podpools-policy.go 24.21% <0.00%> (ø)
...rce-manager/policy/builtin/topology-aware/cache.go 35.33% <ø> (ø)
...cy/builtin/topology-aware/topology-aware-policy.go 16.44% <0.00%> (ø)
pkg/log/log.go 19.00% <0.00%> (-1.10%) ⬇️
...urce-manager/policy/builtin/topology-aware/node.go 52.70% <44.73%> (ø)
...urce-manager/policy/builtin/topology-aware/hint.go 85.71% <57.14%> (ø)
pkg/cpuallocator/allocator.go 48.01% <69.23%> (ø)
...i/resource-manager/control/page-migrate/demoter.go 17.95% <71.42%> (ø)
...rce-manager/policy/builtin/topology-aware/pools.go 62.61% <85.00%> (ø)
... and 1 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 4cec89f...88db640. Read the comment docs.

klihub
klihub reviewed Sep 28, 2021
View reviewed changes
Copy link
Contributor

@klihub klihub left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have spotted a few differences I don't understand, wrt. fully identical code before vs. after these changes. Apart from those/once I understand them, this LGTM.

pkg/sysfs/utils.go Show resolved Hide resolved
pkg/sysfs/utils.go Show resolved Hide resolved
pkg/sysfs/utils.go Show resolved Hide resolved
pkg/sysfs/utils.go Show resolved Hide resolved
klihub
klihub previously approved these changes Sep 28, 2021
View reviewed changes
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.

jukkar
jukkar commented Sep 28, 2021
View reviewed changes
pkg/sysfs/system.go Outdated Show resolved Hide resolved
marquiz
marquiz requested changes Sep 28, 2021
View reviewed changes
Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Quite a diffstat 😅 One small suggestion, otherwise looks good.

Also, I'd suggest to squash the changes to go.{mod,sum} into one single commit.

pkg/sysfs/system.go Outdated Show resolved Hide resolved
pkg/sysfs/system.go Outdated Show resolved Hide resolved
@jukkar
mod: Updated go.[mod|sum] files
3865ca4 
New dependency versions updated.
Use goresctrl v0.2.0 which contains the needed SST modifications.
@jukkar
cri: Remove common SST code that is already found in goresctrl
995a6f2 
Use goresctrl code for SST. This means that we can then remove
duplicate code from cri-resource-manager repo.

Fixes intel#708
@jukkar
log: Add standardized logger interface API
4f9ce60 
Add more standardized logger interface functions so that cri-rm
can use the goresctrl logger API.
@jukkar
Copy link
Contributor Author

jukkar commented Sep 28, 2021

Updated according to comments.

@jukkar jukkar requested a review from marquiz September 28, 2021 11:47
marquiz
marquiz approved these changes Sep 28, 2021
View reviewed changes
Copy link
Contributor

@marquiz marquiz 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 15f4812 into intel:master Sep 29, 2021
@klihub klihub mentioned this pull request Oct 5, 2021
Closed
25 tasks
@jukkar jukkar deleted the integrate-with-goresctrl branch March 7, 2022 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marquiz marquiz marquiz approved these changes

@klihub klihub klihub left review comments

@askervin askervin Awaiting requested review from askervin askervin is a code owner

@ipuustin ipuustin Awaiting requested review from ipuustin

@kad kad Awaiting requested review from kad kad is a code owner

@mythi mythi Awaiting requested review from mythi mythi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jukkar @codecov-commenter @klihub @marquiz