Implement StatsProvider interface using CRI stats #51557

yguo0905 · 2017-08-29T20:18:50Z

This is the follow up of #50932

I include the cadvisor dependency changes in this PR for now to make it build. @dashpole will update the cadvisor dependency very soon, and I will remove the change once it's updated.
Please take a closer look at the implementation in cri_stats_provider.go since we currently don't have any runtime implementing the CRI stats interface and the changes here cannot be enabled in e2e tests.
Pod level network stats and container level logs stats are not provided.
In cadvisor_stats_provider.go, we are able to remove the call to getCgroupStats in ImageFsStats for getting the timestamp of the stats, given that we've changed cadvisor to include the timestamp in FsInfo.
Fixed the usage of assert.Equal in unit tests.

Release note:

Support getting container stats from CRI.

/assign @yujuhong
/assign @Random-Liu

yujuhong

Looks good overall.

yujuhong · 2017-08-30T00:05:31Z

pkg/kubelet/stats/cri_stats_provider.go

+	fsInfo, err := p.cadvisor.GetFsInfoByFsUUID(storageID.Uuid)
+	if err != nil {
+		msg := fmt.Sprintf("Failed to get the info of the filesystem with id %q: %v", storageID.Uuid, err)
+		if cadvisorfs.ErrNoSuchDevice == err {


nit: err == cadvisorfs.ErrNoSuchDevice

yujuhong · 2017-08-30T00:12:47Z

pkg/kubelet/stats/cri_stats_provider.go

+		glog.V(2).Infof("Failed to get filesystem info: storageID is nil")
+		return nil
+	}
+	fsInfo, err := p.cadvisor.GetFsInfoByFsUUID(storageID.Uuid)


Is omitting the filesystem info on cadvisor error the current behavior or a new one introduced in this PR (due to the storage id mapping)?

It's only here in this PR.

When will GetFsInfoByFsUUID return an error (assuming the uuid exists)?

GetFsInfoByFsUUID and GetDirFsInfo (used for retrieving the rootFs information from cAdvisor) convert the UUID and the directory name to the device name and call getFsInfoByDeviceName to get the filesystem stats. So GetFsInfoByFsUUID can fail with the same reasons as GetDirFsInfo could fail.

Do you suggest we return the error instead of partial summary when the UUID does exist but something wrong when fetching the stats?

Add a comment to explain we return partial results. Also check (or file an issue against) heapster to make sure 1) it is capable of processing partial results, and 2) the monitoring pipeline prefers partial results.

yguo0905 · 2017-08-30T01:01:46Z

pkg/kubelet/stats/cri_stats_provider.go

+		return nil, err
+	}
+
+	for _, fs := range resp.ImageFilesystems {


I forgot to mention this in the PR description. CRI may return the stats of multiple image filesystems but here we only return the first one. Is this an issue?

Add a comment here to make it clear.

yujuhong · 2017-08-30T18:35:53Z

PR looks good to me except for the one nit: #51557 (comment)

yujuhong · 2017-08-31T18:49:11Z

@yguo0905 could you add a comment that we return partial results and link to the heapster issue?
Will lgtm once that's done.

yguo0905 · 2017-08-31T19:51:32Z

Done. I'm waiting for @dashpole to update the cadvisor dependency, and then I will remove the changes in vendor from this PR.

yujuhong · 2017-08-31T20:31:04Z

lgtm pending @dashpole updating the cadvisor deps

Random-Liu · 2017-08-31T20:09:22Z

pkg/kubelet/cadvisor/cadvisor_linux.go

@@ -258,3 +258,7 @@ func (cc *cadvisorClient) HasDedicatedImageFs() (bool, error) {
 	}
 	return imageFsInfo.Device != rootFsInfo.Device, nil
 }
+
+func (cc *cadvisorClient) GetFsInfoByFsUUID(uuid string) (cadvisorapiv2.FsInfo, error) {
+	return cc.GetFsInfoByFsUUID(uuid)


Why the function is calling itself? Did I miss anything?

Removed this function to expose the embedded one in manager.Manager.

Random-Liu · 2017-08-31T20:10:23Z

pkg/kubelet/stats/cadvisor_stats_provider.go


+	cadvisorapiv2 "github.com/google/cadvisor/info/v2"


nit: should be in the group above.

Random-Liu · 2017-08-31T20:15:06Z

pkg/kubelet/stats/cri_stats_provider_test.go

+
+	"github.com/stretchr/testify/assert"
+
+	cadvisorfs "github.com/google/cadvisor/fs"


nit: Please follow convention.

Random-Liu · 2017-08-31T20:15:18Z

pkg/kubelet/stats/cri_stats_provider.go


+	"github.com/golang/glog"
+
+	cadvisorfs "github.com/google/cadvisor/fs"


convention.

Random-Liu · 2017-08-31T20:24:06Z

pkg/kubelet/stats/cri_stats_provider.go

+	}
+
+	// CRI may return the stats of multiple image filesystems but we only
+	// return the first one.


Should we add TODO to support multiple ones?

Random-Liu · 2017-08-31T20:29:25Z

pkg/kubelet/stats/cri_stats_provider.go

+// getFsInfo returns the information of the filesystem with the specified
+// storageID. If any error occurs, this function logs the error and returns
+// nil.
+func (p *criStatsProvider) getFsInfo(storageID *runtimeapi.StorageIdentifier) *cadvisorapiv2.FsInfo {


Why don't we return error, and let the caller to decide whether to log error or not?

In this way, we want to have the logging logic in a shared function.

Random-Liu · 2017-08-31T20:35:21Z

pkg/kubelet/stats/cri_stats_provider.go

+		},
+		// UserDefinedMetrics is not supported by CRI.
+	}
+	if stats.Cpu != nil {


You can use stas.GetCpu().GetUsageCoreNanoSeconds().GetValue() to get rid of all the nil checks.

To avoid setting default values in the results, I still need to check whether stas.GetCpu().GetUsageCoreNanoSeconds().GetValue() is 0 or not.

yguo0905 · 2017-08-31T22:53:13Z

Removed the changed in vendor. PTAL.

yujuhong · 2017-08-31T22:55:34Z

/lgtm

fejta-bot · 2017-09-01T02:33:52Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta · 2017-09-01T04:21:17Z

/lgtm cancel

Does not build

yujuhong · 2017-09-01T22:57:00Z

This PR depends on the cadvisor update: #51751

The PR itself is good, but I added the do-not-merge label just so that the bot wouldn't kick off the tests repeatedly.

feiskyer · 2017-09-06T00:35:19Z

/retest

dims · 2017-09-06T11:08:19Z

@kubernetes/kubernetes-release-managers #51751 has merged. Looks like we need to remove the do-not-merge

dims · 2017-09-06T11:08:48Z

@yguo0905 : verify is failing, can you PTAL?

yguo0905 · 2017-09-06T15:46:07Z

@yguo0905 : verify is failing, can you PTAL?

Thanks. Looks like an error caught by govet. I will fix and rebase.

yujuhong · 2017-09-06T17:06:59Z

/lgtm

k8s-github-robot · 2017-09-06T17:07:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yguo0905, yujuhong

Associated issue: 46984

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubelet/OWNERS~~ [yujuhong]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-06T20:36:06Z

Automatic merge from submit-queue (batch tested with PRs 49133, 51557, 51749, 50842, 52018)

@dashpole

Automatic merge from submit-queue (batch tested with PRs 49133, 51557, 51749, 50842, 52018) Implement StatsProvider interface using CRI stats Ref: #46984 This is the follow up of #50932 - I include the cadvisor dependency changes in this PR for now to make it build. @dashpole will update the cadvisor dependency very soon, and I will remove the change once it's updated. - Please take a closer look at the implementation in `cri_stats_provider.go` since we currently don't have any runtime implementing the CRI stats interface and the changes here cannot be enabled in e2e tests. - Pod level network stats and container level logs stats are not provided. - In `cadvisor_stats_provider.go`, we are able to remove the call to `getCgroupStats` in `ImageFsStats` for getting the timestamp of the stats, given that we've changed cadvisor to include the timestamp in `FsInfo`. - Fixed the usage of `assert.Equal` in unit tests. **Release note**: ``` Support getting container stats from CRI. ``` /assign @yujuhong /assign @Random-Liu

liggitt · 2017-09-07T04:51:44Z

pkg/kubelet/cadvisor/types.go

+
+	// GetFsInfoByFsUUID returns the stats of the filesystem with the specified
+	// uuid.
+	GetFsInfoByFsUUID(uuid string) (cadvisorapiv2.FsInfo, error)


adding this without implementing in _unsupported and _windows broke cross-build. fix in #52073

Thanks. Forgot about the cross-build. It would be nice if this can be caught in presubmit test.

Automatic merge from submit-queue Fix cross-build **What this PR does / why we need it**: The cross-build was broken by the following PRs: #51728 #51557 This PR fixes the cross-build rather than revert them. Fixes #52074 **Release note**: ```release-note NONE ```

k8s-ci-robot assigned Random-Liu and yujuhong Aug 29, 2017

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 29, 2017

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 29, 2017

yguo0905 force-pushed the stats-cri branch from 3a39cf8 to 076d5c5 Compare August 29, 2017 21:07

yujuhong reviewed Aug 30, 2017

View reviewed changes

yguo0905 force-pushed the stats-cri branch from 076d5c5 to 77c8c21 Compare August 30, 2017 00:56

yguo0905 commented Aug 30, 2017

View reviewed changes

yguo0905 force-pushed the stats-cri branch 2 times, most recently from 3e3d627 to ecec2de Compare August 30, 2017 16:36

yguo0905 mentioned this pull request Aug 31, 2017

Handling best-effort result from the summary API? kubernetes-retired/heapster#1793

Closed

dashpole mentioned this pull request Aug 31, 2017

Enable CRI-O stats from cAdvisor #51728

Merged

yguo0905 force-pushed the stats-cri branch from ecec2de to dfc013e Compare August 31, 2017 19:50

Random-Liu reviewed Aug 31, 2017

View reviewed changes

yujuhong added this to the v1.8 milestone Aug 31, 2017

yguo0905 force-pushed the stats-cri branch from dfc013e to 78aab4b Compare August 31, 2017 22:39

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2017

k8s-ci-robot assigned fejta Sep 1, 2017

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017

yguo0905 mentioned this pull request Sep 1, 2017

Add Windows Server Containers Stats and Metrics to Kubelet #50396

Merged

yujuhong added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 1, 2017

yujuhong removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Sep 6, 2017

Implement StatsProvider using CRI stats

dfea03d

yguo0905 force-pushed the stats-cri branch from 78aab4b to dfea03d Compare September 6, 2017 16:12

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2017

k8s-github-robot merged commit 3b2e32e into kubernetes:master Sep 6, 2017

yguo0905 deleted the stats-cri branch September 6, 2017 22:35

feiskyer mentioned this pull request Sep 7, 2017

Kubelet: implements ImageFsInfo API #45549

Closed

This was referenced Sep 7, 2017

Fix cross-build #52073

Merged

Cross build broken #52074

Closed

liggitt reviewed Sep 7, 2017

View reviewed changes


		"github.com/stretchr/testify/assert"

		cadvisorfs "github.com/google/cadvisor/fs"


		"github.com/golang/glog"

		cadvisorfs "github.com/google/cadvisor/fs"

Implement StatsProvider interface using CRI stats #51557

Implement StatsProvider interface using CRI stats #51557

Conversation

yguo0905 commented Aug 29, 2017

yujuhong 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

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

Choose a reason for hiding this comment

yujuhong commented Aug 30, 2017

yujuhong commented Aug 31, 2017

yguo0905 commented Aug 31, 2017

yujuhong commented Aug 31, 2017

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

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

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

yguo0905 commented Aug 31, 2017

yujuhong commented Aug 31, 2017

fejta-bot commented Sep 1, 2017

fejta commented Sep 1, 2017

yujuhong commented Sep 1, 2017 • edited Loading

feiskyer commented Sep 6, 2017

dims commented Sep 6, 2017

dims commented Sep 6, 2017

yguo0905 commented Sep 6, 2017

yujuhong commented Sep 6, 2017

k8s-github-robot commented Sep 6, 2017

k8s-github-robot commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujuhong commented Sep 1, 2017 •

edited

Loading