Skip to content

Add reading of information from cpuacct.usage_all #2271

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
merged 1 commit into from
May 13, 2020
Merged

Add reading of information from cpuacct.usage_all #2271

merged 1 commit into from
May 13, 2020

Conversation

@katarzyna-z
Copy link

@katarzyna-z katarzyna-z commented Mar 26, 2020

This pull request introduces reading of cpuacct.usage_all with unit tests for reading stats from cpuacct cgroup.

kolyshkin
kolyshkin reviewed Mar 27, 2020
View reviewed changes
kolyshkin
kolyshkin reviewed Mar 27, 2020
View reviewed changes
kolyshkin
kolyshkin reviewed Mar 27, 2020
View reviewed changes
kolyshkin
kolyshkin reviewed Mar 27, 2020
View reviewed changes
kolyshkin
kolyshkin reviewed Mar 27, 2020
View reviewed changes
@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 27, 2020 •
edited
Loading

LGTM, but please squash the commits

Approved with PullApprove

@katarzyna-z katarzyna-z force-pushed the kk-cpuacct-usage-all branch from 74a169c to d74f62e Compare March 30, 2020 07:08
@katarzyna-z
Copy link
Author

katarzyna-z commented Mar 30, 2020

@AkihiroSuda I squashed commits

@katarzyna-z katarzyna-z force-pushed the kk-cpuacct-usage-all branch from d74f62e to 935716a Compare March 30, 2020 07:55
@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 30, 2020

CI failing

@iwankgb iwankgb mentioned this pull request Mar 30, 2020
Merged
@katarzyna-z katarzyna-z force-pushed the kk-cpuacct-usage-all branch from 935716a to baba000 Compare March 30, 2020 09:33
@katarzyna-z
Copy link
Author

katarzyna-z commented Mar 30, 2020

CI passed thanks to small fix in .travis.yaml (related to #2268)

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 30, 2020

needs rebase

@katarzyna-z katarzyna-z force-pushed the kk-cpuacct-usage-all branch 2 times, most recently from 5c4986e to f4919ff Compare March 31, 2020 07:16
@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 31, 2020 •
edited
Loading

LGTM

Approved with PullApprove

@katarzyna-z
Copy link
Author

katarzyna-z commented Apr 7, 2020

@caniszczyk, @crosbymichael, @cyphar, @dqminh, @hqhq, @mrunalp, @rjnagal, @vmarmol
Could you take a look on this pull request?

cyphar
cyphar requested changes Apr 7, 2020
View reviewed changes
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

Just a few nits. Aside from that it looks good.

libcontainer/cgroups/fs/cpuacct.go Outdated
return err
}

PercpuUsageInKernelmode, PercpuUsageInUsermode, err := getPercpuUsageInModes(path)
Copy link
Member

@cyphar cyphar Apr 7, 2020

Choose a reason for hiding this comment

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

First character should be lowercase in variable names.

libcontainer/cgroups/fs/cpuacct.go Outdated

func getPercpuUsageInModes(path string) ([]uint64, []uint64, error) {
percpuUsageKernelMode := []uint64{}
percpuUsageUserMode := []uint64{}
Copy link
Member

@cyphar cyphar Apr 7, 2020

Choose a reason for hiding this comment

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

NIT: These names are a bit long given that the function already tells you that it's getting the per-cpu usage information. But I don't mind too much.

types/events.go Outdated
Total uint64 `json:"total,omitempty"`
Percpu []uint64 `json:"percpu,omitempty"`
PercpuKernel []uint64 `json:"percpuKernel,omitempty"`
PercpuUser []uint64 `json:"percpuUser,omitempty"`
Copy link
Member

@cyphar cyphar Apr 7, 2020

Choose a reason for hiding this comment

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

These JSON fields should use underscores (percpu_kernel and percpu_user). I know that some others like kernelTCP don't but in theory that's the style for event JSON.

@katarzyna-z katarzyna-z force-pushed the kk-cpuacct-usage-all branch from f4919ff to 685ba13 Compare April 7, 2020 14:00
@katarzyna-z
Copy link
Author

katarzyna-z commented Apr 7, 2020

@cyphar I applied your suggestions.

@katarzyna-z katarzyna-z force-pushed the kk-cpuacct-usage-all branch from 685ba13 to 0037985 Compare April 15, 2020 13:09
@katarzyna-z
Copy link
Author

katarzyna-z commented Apr 15, 2020

@AkihiroSuda @cyphar Could you make the 2nd review? I applied all suggestions.

@katarzyna-z
Copy link
Author

katarzyna-z commented Apr 20, 2020 •
edited
Loading

@caniszczyk, @crosbymichael, @cyphar, @dqminh, @hqhq, @mrunalp, @rjnagal, @vmarmol @AkihiroSuda
Could you make the 2nd review of this pull request?
I want to extend metrics in cAdvisor by information from cpuacct.usage_all and this blocks me because cAdvisor uses packages from runc to read cgroups.

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Apr 20, 2020 •
edited
Loading

LGTM

Approved with PullApprove

kolyshkin
kolyshkin reviewed Apr 20, 2020
View reviewed changes
libcontainer/cgroups/fs/cpuacct.go Outdated
scanner.Scan() //skipping header line

for scanner.Scan() {
lineFields := strings.SplitN(scanner.Text(), " ", cuacctUsageAllColumnsNumber)
Copy link
Contributor

@kolyshkin kolyshkin Apr 20, 2020

Choose a reason for hiding this comment

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

You probably should have used cuacctUsageAllColumnsNumber + 1 as the last parameter. Otherwise you won't be able to detect a situation when there are extra fields.

Copy link
Author

@katarzyna-z katarzyna-z Apr 21, 2020

Choose a reason for hiding this comment

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

I agree, adding +1 will help if a new column appear.

kolyshkin
kolyshkin reviewed Apr 20, 2020
View reviewed changes
libcontainer/cgroups/fs/stats_util_test.go Outdated
}

func expectCPUUsageEqual(t *testing.T, expected, actual cgroups.CpuUsage) {
if expected.TotalUsage != actual.TotalUsage {
Copy link
Contributor

@kolyshkin kolyshkin Apr 20, 2020

Choose a reason for hiding this comment

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

Maybe you could just use reflect.DeepEqual here (or directly in the TestCpuacctStats). If there are other places like this, they can be converted to use DeepEqual, too (as a separate preceding patch).

kolyshkin
kolyshkin requested changes Apr 20, 2020
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Left a few comments.

@katarzyna-z katarzyna-z force-pushed the kk-cpuacct-usage-all branch from 0037985 to 34982ec Compare April 21, 2020 12:14
@katarzyna-z
Copy link
Author

katarzyna-z commented Apr 21, 2020

@kolyshkin I addressed your suggestions, please check if I understood you correctly.

@katarzyna-z
Copy link
Author

katarzyna-z commented Apr 23, 2020

@kolyshkin Could you take a look at this once again?

@katarzyna-z
Copy link
Author

katarzyna-z commented Apr 29, 2020

@AkihiroSuda, @caniszczyk, @crosbymichael, @cyphar, @dqminh, @hqhq, @kolyshkin, @mrunalp
please take another look

@kolyshkin
Copy link
Contributor

kolyshkin commented Apr 29, 2020 •
edited by AkihiroSuda
Loading

LGTM

Approved with PullApprove

@h-vetinari h-vetinari mentioned this pull request Apr 29, 2020
Closed
@katarzyna-z
Add reading of information from cpuacct.usage_all
407e9f9 
Remove logrus logs from tests

Signed-off-by: Katarzyna Kujawa <[email protected]>
@katarzyna-z katarzyna-z force-pushed the kk-cpuacct-usage-all branch from 34982ec to 407e9f9 Compare May 5, 2020 07:39
@katarzyna-z
Copy link
Author

katarzyna-z commented May 5, 2020

@AkihiroSuda @kolyshkin please take another look, I resolved a conflict which appeared after merge of #2377

@katarzyna-z
Copy link
Author

katarzyna-z commented May 7, 2020

@AkihiroSuda @kolyshkin PTAL

@AkihiroSuda
Copy link
Member

AkihiroSuda commented May 7, 2020 •
edited
Loading

LGTM

Approved with PullApprove

@AkihiroSuda
Copy link
Member

AkihiroSuda commented May 11, 2020

@kolyshkin Still LGTY?

@kolyshkin
Copy link
Contributor

kolyshkin commented May 13, 2020 •
edited by AkihiroSuda
Loading

LGTM

Approved with PullApprove

@kolyshkin kolyshkin merged commit 4185531 into opencontainers:master May 13, 2020
@katarzyna-z katarzyna-z mentioned this pull request May 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@cyphar cyphar cyphar requested changes

@kolyshkin kolyshkin kolyshkin requested changes

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

@katarzyna-z @AkihiroSuda @kolyshkin @cyphar