Added support for filesystem metrics on Docker #2768

JohnnyG235711 · 2020-12-20T17:03:11Z

First reported in issue #2609 (#2609)

google-cla · 2020-12-20T17:03:15Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

k8s-ci-robot · 2020-12-20T17:03:19Z

Hi @JohnnyG235711. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

JohnnyG235711 · 2020-12-20T17:05:16Z

@googlebot I signed it!

google-cla · 2020-12-20T17:05:19Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

iwankgb

@JohnnyG235711 you will have to sign CLA before this PR can be merged. I would appreciate taking look at my comments too :)

iwankgb · 2020-12-29T12:41:26Z

fs/fs.go

@@ -483,6 +483,17 @@ func getDiskStatsMap(diskStatsFile string) (map[string]DiskStats, error) {
 				return nil, error
 			}
 		}
+
+		major64, error :=  strconv.ParseUint(words[0], 10, 64)


Use err instead of error as error-storing variable, please.

Sure, I have renamed to "err" in my latest commit.

iwankgb · 2020-12-29T12:44:14Z

fs/fs.go

@@ -439,7 +439,7 @@ func (i *RealFsInfo) GetFsInfoForPath(mountSet map[string]struct{}) ([]Fs, error

 var partitionRegex = regexp.MustCompile(`^(?:(?:s|v|xv)d[a-z]+\d*|dm-\d+)$`)

-func getDiskStatsMap(diskStatsFile string) (map[string]DiskStats, error) {
+func GetDiskStatsMap(diskStatsFile string) (map[string]DiskStats, error) {


Do you think it would be possible to use RealFsInfo.GetFsInfoForPath() rather than exporting yet another function? It would be great to limit public interfaces as much as possible.

It was indeed possible and I have made getDiskStatsMap private again in my latest commit.

google-cla · 2021-01-04T16:15:51Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-01-06T10:04:03Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

JohnnyG235711 · 2021-01-06T11:21:56Z

@iwankgb Thanks for taking the time to look at this. I have changed my code to be inline with your code review comments and pushed the changes into my branch. Hopefully you can see them for the re-review. I have also updated my primary email address to match the one in the google CLA corporate agreement for my company, which is Syncsort Incorporated. Thanks.

iwankgb · 2021-01-06T11:45:37Z

@JohnnyG235711 you probably need to talk to someone at your company to be added to an AD (or similar) group. The bot still claims you have not signed the CLA.

google-cla · 2021-01-06T18:18:14Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-01-06T18:36:41Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-01-06T18:50:00Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-01-06T18:59:27Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-01-06T19:05:27Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-01-06T19:14:01Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

JohnnyG235711 · 2021-01-06T19:23:46Z

@googlebot I fixed it.

google-cla · 2021-01-06T19:23:55Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

JohnnyG235711 · 2021-01-06T19:37:02Z

@JohnnyG235711 you probably need to talk to someone at your company to be added to an AD (or similar) group. The bot still claims you have not signed the CLA.

@iwankgb I'm in the group now, but now getting a different error: "We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors."
There's only one author though, and it's me. I'm wondering if it's because I didn't set my email in the previous commits or maybe because I merged upwards. If so maybe I need to try again with the different branch.

iwankgb · 2021-01-06T22:59:21Z

@JohnnyG235711 if I were you I would try to rebase against master and then squash all the commits. It might help.

JohnnyG235711 · 2021-01-07T16:48:57Z

@iwankgb Thank you, that worked.

iwankgb · 2021-01-10T16:29:48Z

/ok-to-test

iwankgb

It segfaults on my RPi 4:

I0110 16:38:12.163674   88322 manager.go:987] Added container: "/user.slice" (aliases: [], namespace: "")
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x9fc39c]

goroutine 56 [running]:
github.com/google/cadvisor/container/docker.(*dockerContainerHandler).addDiskStats(0x40000dd2c0, 0x400086e7a8, 0x0)
	/home/ubuntu/go/src/github.com/google/cadvisor/container/docker/handler.go:430 +0xa4
github.com/google/cadvisor/container/docker.(*dockerContainerHandler).getFsStats(0x40000dd2c0, 0x4000805800, 0x0, 0x0)
	/home/ubuntu/go/src/github.com/google/cadvisor/container/docker/handler.go:418 +0x23c
github.com/google/cadvisor/container/docker.(*dockerContainerHandler).GetStats(0x40000dd2c0, 0x0, 0x0, 0x0)
	/home/ubuntu/go/src/github.com/google/cadvisor/container/docker/handler.go:465 +0x68
github.com/google/cadvisor/manager.(*containerData).updateStats(0x4000788000, 0xbff6e93d09287079, 0xace62627)
	/home/ubuntu/go/src/github.com/google/cadvisor/manager/container.go:637 +0x54
github.com/google/cadvisor/manager.(*containerData).housekeepingTick(0x4000788000, 0x400070a2a0, 0x5f5e100, 0x4000127100)
	/home/ubuntu/go/src/github.com/google/cadvisor/manager/container.go:583 +0xf4
github.com/google/cadvisor/manager.(*containerData).housekeeping(0x4000788000)
	/home/ubuntu/go/src/github.com/google/cadvisor/manager/container.go:531 +0x1bc
created by github.com/google/cadvisor/manager.(*containerData).Start
	/home/ubuntu/go/src/github.com/google/cadvisor/manager/container.go:119 +0x3c
ubuntu@malyna4:~/go/src/github.com/google/cadvisor$ uname -a
Linux malyna4 5.4.0-1025-raspi #28-Ubuntu SMP PREEMPT Wed Dec 9 17:10:53 UTC 2020 aarch64 aarch64 aarch64 GNU/Linux
ubuntu@malyna4:~/go/src/github.com/google/cadvisor$ go version
go version go1.13.8 linux/arm64

iwankgb · 2021-01-10T16:41:02Z

I think that we are missing an important test here: CI should not pass :/

JohnnyG235711 · 2021-01-13T16:45:56Z

I have added a failing test, which I then fixed by fixing the bug.

JohnnyG235711 · 2021-01-25T08:47:20Z

@iwankgb Is my revised code OK?

iwankgb · 2021-01-26T05:37:46Z

@JohnnyG235711 sorry for the delay, it's been hectic time at work. I will take another look at your code this week.

JohnnyG235711 · 2021-01-26T09:07:31Z

@iwankgb No worries, and thanks for looking at it.

Creatone

Few comments.

Creatone · 2021-02-01T13:03:53Z

container/docker/handler.go

@@ -384,7 +384,7 @@ func (h *dockerContainerHandler) getFsStats(stats *info.ContainerStats) error {
 	case aufsStorageDriver, overlayStorageDriver, overlay2StorageDriver, vfsStorageDriver:
 		deviceInfo, err := h.fsInfo.GetDirFsDevice(h.rootfsStorageDir)
 		if err != nil {
-			return fmt.Errorf("unable to determine device info for dir: %v: %v", h.rootfsStorageDir, err)
+			return fmt.Errorf("Unable to determine device info for dir: %v: %v", h.rootfsStorageDir, err)


Errors shouldn't start with the capitalized words.

Creatone · 2021-02-01T13:08:15Z

container/docker/handler.go

+		if err == nil {
+			addDiskStats(fileSystems, fsInfo, &fsStat)
+		} else {
+			klog.V(5).Infof("Unable to obtain diskstats for filesystem %s: %v", fsStat.Device, err)


Should it be Errorf?

Creatone · 2021-02-02T14:49:44Z

container/docker/handler.go

+			fsStats.IoInProgress = fileSys.DiskStats.IoInProgress
+			fsStats.IoTime = fileSys.DiskStats.IoTime
+			fsStats.WeightedIoTime = fileSys.DiskStats.WeightedIoTime
+		}


Please, break iteration after finding your device.

Creatone · 2021-02-02T15:03:09Z

fs/fs_test.go

@@ -44,7 +44,7 @@ func TestMountInfoFromDir(t *testing.T) {
 func TestGetDiskStatsMap(t *testing.T) {
 	diskStatsMap, err := getDiskStatsMap("test_resources/diskstats")
 	if err != nil {
-		t.Errorf("Error calling getDiskStatMap %s", err)
+		t.Errorf("Error calling GetDiskStatMap %s", err)


The function didn't change the name, so in my opinion, you shouldn't change this error.

JohnnyG235711 · 2021-02-09T11:19:38Z

@Creatone Thanks for your feedback. I have amended the code in line with your review. Could you re-review?

Creatone · 2021-02-09T11:26:53Z

@JohnnyG235711 please squash commits :)

Added support for filesystem metrics on Docker Added support for filesystem metrics on Docker Added support for filesystem metrics on Docker Added support for filesystem metrics on Docker Added support for filesystem metrics on Docker Added support for filesystem metrics on Docker Added support for filesystem metrics on Docker Added support for filesystem metrics on Docker Added support for filesystem metrics on Docker Added support for filesystem metrics on Docker Added support for filesystem metrics on Docker

Creatone

LGTM

JohnnyG235711 · 2021-02-10T09:28:22Z

@CreateOne Thanks for looking at it
@iwankgb Looks like before merging can occur you also need to approve it. I believe that I have fixed the seg fault.

iwankgb · 2021-02-10T10:18:51Z

LGTM

JohnnyG235711 · 2021-03-01T16:50:03Z

@bobbypage I was wondering when this might be tested and then merged? Thanks very much.

bobbypage · 2021-03-05T07:38:42Z

LGTM, thanks.

google-cla bot added the cla: no label Dec 20, 2020

k8s-ci-robot added the needs-ok-to-test label Dec 20, 2020

iwankgb requested changes Dec 29, 2020

View reviewed changes

JohnnyG235711 force-pushed the docker-0filesystem-fix branch from 57f889b to 9ac164d Compare January 7, 2021 15:50

google-cla bot added cla: yes and removed cla: no labels Jan 7, 2021

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jan 10, 2021

iwankgb requested changes Jan 10, 2021

View reviewed changes

Creatone self-requested a review February 1, 2021 11:53

Creatone reviewed Feb 2, 2021

View reviewed changes

JohnnyG235711 force-pushed the docker-0filesystem-fix branch from fbf699a to 8c0666a Compare February 9, 2021 13:32

Creatone approved these changes Feb 9, 2021

View reviewed changes

JohnnyG235711 requested a review from iwankgb February 10, 2021 09:28

iwankgb approved these changes Feb 10, 2021

View reviewed changes

bobbypage self-assigned this Feb 17, 2021

bobbypage merged commit 1a019d7 into google:master Mar 5, 2021

Shaye-man mentioned this pull request Mar 23, 2022

Collecting Filesystem metrics makes extremely high CPU #3082

Open

Added support for filesystem metrics on Docker #2768

Added support for filesystem metrics on Docker #2768

Conversation

JohnnyG235711 commented Dec 20, 2020

google-cla bot commented Dec 20, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

k8s-ci-robot commented Dec 20, 2020

JohnnyG235711 commented Dec 20, 2020

google-cla bot commented Dec 20, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

iwankgb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-cla bot commented Jan 4, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

google-cla bot commented Jan 6, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

JohnnyG235711 commented Jan 6, 2021

iwankgb commented Jan 6, 2021

google-cla bot commented Jan 6, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

google-cla bot commented Jan 6, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

google-cla bot commented Jan 6, 2021

google-cla bot commented Jan 6, 2021

google-cla bot commented Jan 6, 2021

google-cla bot commented Jan 6, 2021

JohnnyG235711 commented Jan 6, 2021

google-cla bot commented Jan 6, 2021

JohnnyG235711 commented Jan 6, 2021

iwankgb commented Jan 6, 2021

JohnnyG235711 commented Jan 7, 2021

iwankgb commented Jan 10, 2021

iwankgb left a comment

Choose a reason for hiding this comment

iwankgb commented Jan 10, 2021

JohnnyG235711 commented Jan 13, 2021

JohnnyG235711 commented Jan 25, 2021

iwankgb commented Jan 26, 2021

JohnnyG235711 commented Jan 26, 2021

Creatone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnnyG235711 commented Feb 9, 2021

Creatone commented Feb 9, 2021

Creatone left a comment

Choose a reason for hiding this comment

JohnnyG235711 commented Feb 10, 2021

iwankgb commented Feb 10, 2021

JohnnyG235711 commented Mar 1, 2021

bobbypage commented Mar 5, 2021