Add mountpoint as CRI image filesystem storage identifier. #59475

Random-Liu · 2018-02-07T09:32:03Z

This PR changes CRI to use mountpoint as storage identifier. See #57356 (comment).

Note that:

This doesn't work with devicemapper for now. Please feel free to propose change for device mapper, we can discuss more about this after this first version is merged. @mrunalp @runcom
mountpoint is added as new field in StorageIdentifier now. After Update Container Runtime Interface to use enumerated namespace modes #58973 is merged, we can remove the UUID field in v1alpha2.

/cc @yujuhong @feiskyer @yguo0905 @dashpole @mikebrow @abhi @kubernetes/sig-node-api-reviews

Release note:

CRI starts using moutpoint as image filesystem identifier instead of UUID.

Random-Liu · 2018-02-07T09:39:22Z

A container runtime implementation is here containerd/cri#601.

I've manually tested it, and it works. The CI https://k8s-testgrid.appspot.com/sig-node-containerd will catch more issue if there is any.

dashpole

question

dashpole · 2018-02-07T17:18:58Z

pkg/kubelet/apis/cri/v1alpha1/runtime/api.pb.go

@@ -3183,6 +3183,9 @@ func (m *UInt64Value) GetValue() uint64 {
 type StorageIdentifier struct {
 	// UUID of the device.
 	Uuid string `protobuf:"bytes,1,opt,name=uuid,proto3" json:"uuid,omitempty"`


somewhat related: do we plan to keep the UUID field? If so, what is it used for now that we are passing the mount point?

read the PR description, ignore :)

dashpole · 2018-02-07T17:22:06Z

/lint

k8s-ci-robot

@dashpole: 5 warnings.

In response to this:

/lint

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.

k8s-ci-robot · 2018-02-07T17:22:34Z

pkg/kubelet/cadvisor/testing/cadvisor_mock.go

@@ -84,7 +84,7 @@ func (c *Mock) WatchEvents(request *events.Request) (*events.EventChannel, error
 	return args.Get(0).(*events.EventChannel), args.Error(1)
 }

-func (c *Mock) GetFsInfoByFsUUID(uuid string) (cadvisorapiv2.FsInfo, error) {
-	args := c.Called(uuid)
+func (c *Mock) GetDirFsInfo(path string) (cadvisorapiv2.FsInfo, error) {


Golint comments: exported method Mock.GetDirFsInfo should have comment or be unexported. More info.

k8s-ci-robot · 2018-02-07T17:22:34Z

pkg/kubelet/apis/cri/v1alpha1/runtime/api.pb.go

@@ -3196,6 +3199,13 @@ func (m *StorageIdentifier) GetUuid() string {
 	return ""
 }

+func (m *StorageIdentifier) GetMountpoint() string {


Golint comments: exported method StorageIdentifier.GetMountpoint should have comment or be unexported. More info.

k8s-ci-robot · 2018-02-07T17:22:34Z

pkg/kubelet/cadvisor/cadvisor_unsupported.go

@@ -77,6 +77,6 @@ func (cu *cadvisorUnsupported) WatchEvents(request *events.Request) (*events.Eve
 	return nil, unsupportedErr
 }

-func (c *cadvisorUnsupported) GetFsInfoByFsUUID(uuid string) (cadvisorapiv2.FsInfo, error) {
+func (c *cadvisorUnsupported) GetDirFsInfo(path string) (cadvisorapiv2.FsInfo, error) {


Golint naming: receiver name c should be consistent with previous receiver name cu for cadvisorUnsupported. More info.

k8s-ci-robot · 2018-02-07T17:22:34Z

pkg/kubelet/cadvisor/cadvisor_windows.go

@@ -77,6 +77,6 @@ func (cu *cadvisorClient) WatchEvents(request *events.Request) (*events.EventCha
 	return &events.EventChannel{}, nil
 }

-func (c *cadvisorClient) GetFsInfoByFsUUID(uuid string) (cadvisorapiv2.FsInfo, error) {
+func (c *cadvisorClient) GetDirFsInfo(path string) (cadvisorapiv2.FsInfo, error) {


Golint naming: receiver name c should be consistent with previous receiver name cu for cadvisorClient. More info.

k8s-ci-robot · 2018-02-07T17:22:34Z

pkg/kubelet/cadvisor/testing/cadvisor_fake.go

@@ -76,6 +76,6 @@ func (c *Fake) WatchEvents(request *events.Request) (*events.EventChannel, error
 	return new(events.EventChannel), nil
 }

-func (c *Fake) GetFsInfoByFsUUID(uuid string) (cadvisorapiv2.FsInfo, error) {
+func (c *Fake) GetDirFsInfo(path string) (cadvisorapiv2.FsInfo, error) {


Golint comments: exported method Fake.GetDirFsInfo should have comment or be unexported. More info.

yujuhong · 2018-02-07T18:24:39Z

Le'ts wait for the version bump PR to land first.

Random-Liu · 2018-02-07T22:24:21Z

Rebased and switched to v1alpha2.

yujuhong · 2018-02-07T22:31:59Z

pkg/kubelet/apis/cri/runtime/v1alpha2/api.proto

@@ -1105,8 +1105,8 @@ message UInt64Value {

 // StorageIdentifier uniquely identify the storage..
 message StorageIdentifier{
-    // UUID of the device.
-    string uuid = 1;
+    // Mountpoint of the image filesystem.


The StorageIdentifier is a more generic message type. The comment should not include "image filesystem".

On the other hand, is StorageIdentifier still an accurate name, or should we rename it to, say, FilesystemIdentifier?

Will do then.

Done. Changed to FilesystemIdentifier.

mrunalp · 2018-02-08T21:52:31Z

@sameo PTAL.

mrunalp · 2018-02-08T22:31:55Z

👍 We can add pool name in a follow-on for device mapper.

dashpole · 2018-02-09T00:48:39Z

Code changes lgtm. I also agree with the idea of using mountpoint as an identifier.

dashpole · 2018-02-09T01:19:27Z

/lgtm

k8s-ci-robot · 2018-02-09T01:19:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, Random-Liu

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

The pull request process is described here

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

~~pkg/kubelet/OWNERS~~ [Random-Liu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-02-09T03:04:03Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-09T03:41:57Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from sjpotter and tallclair February 7, 2018 09:32

Random-Liu assigned dashpole, yujuhong and yguo0905 Feb 7, 2018

Random-Liu mentioned this pull request Feb 7, 2018

Support new cri storage containerd/cri#601

Merged

dashpole reviewed Feb 7, 2018

View reviewed changes

k8s-ci-robot reviewed Feb 7, 2018

View reviewed changes

Random-Liu force-pushed the use-mountpoint branch from 816d427 to ca01b33 Compare February 7, 2018 22:21

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 7, 2018

Random-Liu force-pushed the use-mountpoint branch from ca01b33 to e8c243b Compare February 7, 2018 22:24

yujuhong reviewed Feb 7, 2018

View reviewed changes

Add mountpoint as CRI image filesystem storage identifier.

a77450e

Random-Liu force-pushed the use-mountpoint branch from e8c243b to a77450e Compare February 7, 2018 23:01

Random-Liu added this to the v1.10 milestone Feb 8, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2018

k8s-github-robot merged commit b24bc2c into kubernetes:master Feb 9, 2018

jberkus mentioned this pull request Feb 13, 2018

v1.10 known issues / FAQ accumulator #59764

Closed

paulnivin mentioned this pull request Sep 16, 2020

CRI getFsInfo logs errors for valid filesystems mounted after kubelet start #94825

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mountpoint as CRI image filesystem storage identifier. #59475

Add mountpoint as CRI image filesystem storage identifier. #59475

Random-Liu commented Feb 7, 2018 •

edited

Loading

Random-Liu commented Feb 7, 2018 •

edited

Loading

dashpole left a comment

dashpole Feb 7, 2018

dashpole Feb 7, 2018

dashpole commented Feb 7, 2018

k8s-ci-robot left a comment

k8s-ci-robot Feb 7, 2018

k8s-ci-robot Feb 7, 2018

k8s-ci-robot Feb 7, 2018

Random-Liu Feb 7, 2018

k8s-ci-robot Feb 7, 2018

Random-Liu Feb 7, 2018

k8s-ci-robot Feb 7, 2018

yujuhong commented Feb 7, 2018

Random-Liu commented Feb 7, 2018 •

edited

Loading

yujuhong Feb 7, 2018

Random-Liu Feb 7, 2018

Random-Liu Feb 7, 2018

mrunalp commented Feb 8, 2018

mrunalp commented Feb 8, 2018

dashpole commented Feb 9, 2018

dashpole commented Feb 9, 2018

k8s-ci-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

Add mountpoint as CRI image filesystem storage identifier. #59475

Add mountpoint as CRI image filesystem storage identifier. #59475

Conversation

Random-Liu commented Feb 7, 2018 • edited Loading

Random-Liu commented Feb 7, 2018 • edited Loading

dashpole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashpole commented Feb 7, 2018

k8s-ci-robot 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

yujuhong commented Feb 7, 2018

Random-Liu commented Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp commented Feb 8, 2018

mrunalp commented Feb 8, 2018

dashpole commented Feb 9, 2018

dashpole commented Feb 9, 2018

k8s-ci-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

Random-Liu commented Feb 7, 2018 •

edited

Loading

Random-Liu commented Feb 7, 2018 •

edited

Loading

Random-Liu commented Feb 7, 2018 •

edited

Loading