feature: stats of cri manager #1431

starnop · 2018-05-29T08:46:10Z

Signed-off-by: Starnop starnop@163.com

Ⅰ. Describe what this PR did

This PR implement ContainerStats(), ListContainerStats() and ImageFsInfo() of CRI manager.

All of these interfaces are used to retrieve the metrics of containers or image file system info.

Ⅱ. Does this pull request fix one issue?

None

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Use crictl tools:

# crictl stats
DEBU[0000] ListContainerStatsRequest: &ListContainerStatsRequest{Filter:&ContainerStatsFilter{Id:,PodSandboxId:,LabelSelector:map[string]string{},},} 

......

CONTAINER           CPU %               MEM                 DISK                INODES
0bbc13aa25d81       0.59                30.14MB             45.06kB             14
15eef79448135       0.00                286.7kB             24.58kB             9
2e257f09a2260       0.09                9.581MB             24.58kB             6
476ae4d251fe6       0.00                290.8kB             24.58kB             9
574b07cfd946c       0.03                10.21MB             8.192kB             3
587856b374711       0.44                11.49MB             24.58kB             10
6df66169f907a       1.18                37.26MB             61.44kB             19
725a1d78af2b0       0.00                290.8kB             24.58kB             9
8b44f5423bd64       0.02                6.955MB             12.29kB             4
a1f9ccae689ed       0.00                274.4kB             24.58kB             9
a42c9875b9e82       1.13                233.7MB             36.86kB             12
c4321c743307d       0.00                225.3kB             24.58kB             9
cf2dda929ea0b       0.00                286.7kB             24.58kB             9
eb400edd89bb5       0.00                5.947MB             28.67kB             9

or use curl tools:

# curl -X GET http://127.0.0.1:10255/stats/summary

Ⅴ. Special notes for reviews

codecov-io · 2018-05-29T09:42:29Z

Codecov Report

Merging #1431 into master will decrease coverage by 1.74%.
The diff coverage is 9.37%.

@@            Coverage Diff             @@
##           master    #1431      +/-   ##
==========================================
- Coverage   41.45%   39.71%   -1.75%     
==========================================
  Files         273      274       +1     
  Lines       17714    18584     +870     
==========================================
+ Hits         7344     7381      +37     
- Misses       9461    10294     +833     
  Partials      909      909

Impacted Files	Coverage Δ
cri/v1alpha2/cri_utils.go	`24.57% <0%> (-3.3%)`	⬇️
ctrd/snapshot.go	`44% <0%> (-17.12%)`	⬇️
cri/v1alpha2/cri.go	`0% <0%> (ø)`	⬆️
cri/v1alpha1/cri.go	`0% <0%> (ø)`	⬆️
cri/v1alpha1/cri_utils.go	`23.88% <0%> (-4.85%)`	⬇️
ctrd/wrapper_client.go	`77.14% <100%> (ø)`	⬆️
ctrd/client_opts.go	`50% <100%> (+4.54%)`	⬆️
ctrd/client.go	`54.93% <100%> (ø)`	⬆️
ctrd/container.go	`48.79% <11.11%> (-1.21%)`	⬇️
daemon/mgr/container.go	`35.59% <15%> (-14.8%)`	⬇️
... and 7 more

fuweid

First Round Review

fuweid · 2018-06-28T06:47:43Z

cri/v1alpha1/cri.go

@@ -127,6 +143,18 @@ func NewCriManager(config *config.Config, ctrMgr mgr.ContainerMgr, imgMgr mgr.Im
 		return nil, fmt.Errorf("failed to create sandbox meta store: %v", err)
 	}

+	imageFSPath := imageFSPath(path.Join(config.HomeDir, "containerd/root"), defaultSnapshotterName)


I think that we should add utils' function to retrieve the root folder in the project next step.

A separate PR will be submitted to deal with similar things.

fuweid · 2018-06-28T06:50:48Z

daemon/mgr/snapshot.go

+		for {
+			err := s.Sync()
+			if err != nil {
+				logrus.Errorf("failed to sync snapshot stats: %v", err)


I think we should add TODO for this line, because it maybe print error message every 10 seconds.

The TODO is to track the error and report it to the monitor or something.

fuweid · 2018-06-28T06:57:45Z

daemon/mgr/snapshot.go

+	if sn, ok := s.snapshots[key]; ok {
+		return sn, nil
+	}
+	return Snapshot{}, fmt.Errorf("failed to get %q in snapshot store", key)


I think we should return const ErrNotFound error here. No need to custom right there.
Therefore, we can use the const error to check result of Get and make the code clear.

fuweid · 2018-06-28T07:02:19Z

daemon/mgr/snapshot.go

+		sn.Inodes = uint64(usage.Inodes)
+		s.store.Add(sn)
+	}
+	for _, sn := range s.store.List() {


Could you mind to add more information about this part? For example, about the delete logic. Why do you only delete the out of date snapshot?

The purpose of this operation is to keep the synchronization with client.SnapshotService.
A case:

when you remove a container,you also need to remove snapshot. However, SnapshotStore will not be notified. So we need to delete snapshots from SnapshotStore that doesn't exist actually.

fuweid · 2018-06-28T07:09:51Z

cri/v1alpha1/cri.go

-	return nil, fmt.Errorf("ContainerStats Not Implemented Yet")
+	containerID := r.GetContainerId()
+
+	container, err := c.ContainerMgr.Get(ctx, containerID)


Could we use a function to get the cs? If we have the function, we will reuse the function in the ListContainerStats. Don't repeat yourself.

Maybe it is not necessary to do that, the function getContainerMetrics has been encapsulated, what has been repeated is just error handling.

I don't think so. The function like generateContainerStatInfo wraps the c.ContainerMgr.Stats and c.getContainerMetrics to avoid the repeat the same thing. In the future, you only need to update the wrapped function.

Or use the getContainerMetrics to contains the c.ContainerMgr.Stats, WDTY?

I have to admit it's more elegant. Thank you.

fuweid · 2018-06-28T07:17:24Z

cri/v1alpha1/cri.go

+	timestamp := time.Now().UnixNano()
+	var usedBytes, inodesUsed uint64
+	for _, sn := range snapshots {
+		// Use the oldest timestamp as the timestamp of imagefs info.


Could you add more information about this logic?

Timestamp is latest time which the information were collected, we hava to be consistent. So I choose the oldest timestamp as the timestamp of imagefs info here.

fuweid · 2018-07-05T03:17:35Z

cri/v1alpha1/cri.go

-	return nil, fmt.Errorf("ContainerStats Not Implemented Yet")
+	containerID := r.GetContainerId()
+
+	container, err := c.ContainerMgr.Get(ctx, containerID)


I don't think so. The function like generateContainerStatInfo wraps the c.ContainerMgr.Stats and c.getContainerMetrics to avoid the repeat the same thing. In the future, you only need to update the wrapped function.

Or use the getContainerMetrics to contains the c.ContainerMgr.Stats, WDTY?

fuweid · 2018-07-05T03:24:03Z

cri/v1alpha1/cri_utils.go

+// deviceUUID gets device uuid of a device. The passed in rdev should
+// be linux device number.
+func deviceUUID(rdev uint64) (string, error) {
+	const uuidDir = "/dev/disk/by-uuid"


Could we put the const at the beginning of file?

fuweid · 2018-07-05T03:26:49Z

cri/v1alpha1/cri_utils.go

+	if err != nil {
+		return 0, err
+	}
+	stat := info.Sys().(*syscall.Stat_t)


https://golang.org/pkg/os/#FileInfo Sys() maybe nil and could we use stat, ok := info.Sys().(*syscall.Stat_t) to handle here? Because the assert failure will cause the panic.

fuweid · 2018-07-06T06:05:18Z

cri/v1alpha1/cri_utils.go

@@ -17,10 +19,16 @@ import (
 	"github.com/alibaba/pouch/pkg/utils"
 	"github.com/go-openapi/strfmt"


please move it into next group

YaoZengzeng · 2018-07-09T02:56:08Z

daemon/mgr/snapshot.go

+}
+
+// NewSnapshotsSyncer creates a snapshot syncer.
+func NewSnapshotsSyncer(store *SnapshotStore, cli ctrd.APIClient, period time.Duration) *SnapshotsSyncer {


NewSnapshotsSyncer will not be exported, rename it to newSnapshotsSyncer would be better.

YaoZengzeng · 2018-07-09T02:58:36Z

Except the minor issue mentioned above, LGTM

Signed-off-by: Starnop <starnop@163.com>

fuweid · 2018-07-09T04:19:02Z

LGTM

pouchrobot added kind/feature size/XXL labels May 29, 2018

starnop force-pushed the cri-stats branch 5 times, most recently from 198958c to f34e77e Compare May 29, 2018 09:18

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Jun 5, 2018

starnop force-pushed the cri-stats branch from f34e77e to 85c19c1 Compare June 11, 2018 02:42

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Jun 11, 2018

starnop force-pushed the cri-stats branch from 9f340f6 to e21adad Compare June 27, 2018 10:10

fuweid reviewed Jun 28, 2018

View reviewed changes

starnop force-pushed the cri-stats branch 5 times, most recently from e2690e7 to 6584c5d Compare July 2, 2018 06:54

fuweid reviewed Jul 5, 2018

View reviewed changes

starnop force-pushed the cri-stats branch from 6584c5d to c13ccac Compare July 5, 2018 09:33

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Jul 5, 2018

starnop force-pushed the cri-stats branch from c13ccac to 46d098d Compare July 6, 2018 03:45

pouchrobot removed the conflict/needs-rebase label Jul 6, 2018

starnop force-pushed the cri-stats branch from 46d098d to d3c5a66 Compare July 6, 2018 03:47

fuweid reviewed Jul 6, 2018

View reviewed changes

starnop force-pushed the cri-stats branch 5 times, most recently from cb1cdb3 to 02f17da Compare July 6, 2018 09:31

YaoZengzeng reviewed Jul 9, 2018

View reviewed changes

feature: add features about stats of cri manager

c869145

Signed-off-by: Starnop <starnop@163.com>

starnop force-pushed the cri-stats branch from 02f17da to c869145 Compare July 9, 2018 03:01

fuweid merged commit ebf3817 into AliyunContainerService:master Jul 9, 2018

pouchrobot mentioned this pull request Jul 12, 2018

Weekly Report of PouchContainer from 2018-07-05 to 2018-07-12 #1722

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: stats of cri manager #1431

feature: stats of cri manager #1431

starnop commented May 29, 2018 •

edited

Loading

codecov-io commented May 29, 2018 •

edited

Loading

fuweid left a comment

fuweid Jun 28, 2018

starnop Jul 2, 2018

fuweid Jun 28, 2018

starnop Jul 2, 2018

fuweid Jun 28, 2018

starnop Jul 2, 2018

fuweid Jun 28, 2018

starnop Jul 2, 2018 •

edited

Loading

fuweid Jun 28, 2018

starnop Jul 2, 2018

fuweid Jul 5, 2018

starnop Jul 5, 2018

fuweid Jun 28, 2018

starnop Jul 2, 2018

fuweid Jul 5, 2018

fuweid Jul 5, 2018

fuweid Jul 5, 2018

fuweid Jul 6, 2018

YaoZengzeng Jul 9, 2018

YaoZengzeng commented Jul 9, 2018

fuweid commented Jul 9, 2018

		@@ -17,10 +19,16 @@ import (
		"github.com/alibaba/pouch/pkg/utils"
		"github.com/go-openapi/strfmt"

feature: stats of cri manager #1431

feature: stats of cri manager #1431

Conversation

starnop commented May 29, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io commented May 29, 2018 • edited Loading

Codecov Report

fuweid 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

starnop Jul 2, 2018 • edited Loading

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

YaoZengzeng commented Jul 9, 2018

fuweid commented Jul 9, 2018

starnop commented May 29, 2018 •

edited

Loading

codecov-io commented May 29, 2018 •

edited

Loading

starnop Jul 2, 2018 •

edited

Loading