Skip to content

Blkio weight/leafWeight pointer value #69

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
Merged
dqminh merged 1 commit into from
Dec 8, 2018
Merged

Blkio weight/leafWeight pointer value #69

dqminh merged 1 commit into from
Dec 8, 2018

Conversation

@cclerget
Copy link
Contributor

@cclerget cclerget commented Dec 3, 2018

Prevent weight/leafweight from returning address value instead :

type LinuxWeightDevice struct {
	linuxBlockIODevice
	Weight *uint16 `json:"weight,omitempty"`
	LeafWeight *uint16 `json:"leafWeight,omitempty"`
}

Not sure if this is the right thing to do by returning "1000" when pointer is null ...

@cclerget cclerget force-pushed the master-weight-pointer branch from 1925066 to be7c666 Compare December 3, 2018 12:49
@codecov-io
Copy link

codecov-io commented Dec 3, 2018 •
edited
Loading

Codecov Report

Merging #69 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage    14.3%   14.27%   -0.03%     
==========================================
  Files          23       23              
  Lines        4579     4588       +9     
==========================================
  Hits          655      655              
- Misses       3803     3812       +9     
  Partials      121      121
Impacted Files Coverage Δ
blkio.go 20.77% <0%> (-0.95%) ⬇️

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 3bc6dde...64bade4. Read the comment docs.

crosbymichael
crosbymichael reviewed Dec 4, 2018
View reviewed changes
@crosbymichael
Copy link
Member

crosbymichael commented Dec 4, 2018

@cclerget i'd make https://github.com/containerd/cgroups/pull/69/files#diff-a0138ebe17a246a844ed97a795eecd9eR194 and https://github.com/containerd/cgroups/pull/69/files#diff-a0138ebe17a246a844ed97a795eecd9eR206 conditional. Don't place them in the slice if they have nil weights

@cclerget
Copy link
Contributor Author

cclerget commented Dec 4, 2018

@crosbymichael Sorry about confusion, updated with your suggestions

@crosbymichael
Copy link
Member

crosbymichael commented Dec 4, 2018

Looks great @cclerget ! maybe a simple test to make sure this does not panic anymore would be amazing.

@crosbymichael
Copy link
Member

crosbymichael commented Dec 4, 2018

And can you squash your commits into one before we merge?

@cclerget cclerget force-pushed the master-weight-pointer branch from a48fed3 to 64bade4 Compare December 4, 2018 20:43
@cclerget
Copy link
Contributor Author

cclerget commented Dec 4, 2018

Yep, I successfully run same tests (from opencontainers/runtime-tools validation tests) without panic

@crosbymichael
Copy link
Member

crosbymichael commented Dec 4, 2018

LGTM

1 similar comment
@dqminh
Copy link
Member

dqminh commented Dec 8, 2018

LGTM

@dqminh dqminh merged commit 65ce98b into containerd:master Dec 8, 2018
@thaJeztah thaJeztah mentioned this pull request Mar 13, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@crosbymichael crosbymichael crosbymichael left review comments

Reviewers whose approvals may not affect merge requirements

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

@cclerget @codecov-io @crosbymichael @dqminh