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.

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

Add mountpoint as CRI image filesystem storage identifier. #59475

Merged
merged 1 commit into from Feb 9, 2018

Conversation

@Random-Liu
Member

Random-Liu commented Feb 7, 2018

Fixes #57356.

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

Note that:

  1. 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
  2. mountpoint is added as new field in StorageIdentifier now. After #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

This comment has been minimized.

Member

Random-Liu commented Feb 7, 2018

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.

@@ -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"`

This comment has been minimized.

@dashpole

dashpole Feb 7, 2018

Contributor

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?

This comment has been minimized.

@dashpole

dashpole Feb 7, 2018

Contributor

read the PR description, ignore :)

@dashpole

This comment has been minimized.

Contributor

dashpole commented Feb 7, 2018

/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.

@@ -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) {

This comment has been minimized.

@k8s-ci-robot

k8s-ci-robot Feb 7, 2018

Contributor

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

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

This comment has been minimized.

@k8s-ci-robot

k8s-ci-robot Feb 7, 2018

Contributor

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

@@ -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) {

This comment has been minimized.

@k8s-ci-robot

k8s-ci-robot Feb 7, 2018

Contributor

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

This comment has been minimized.

@Random-Liu
@@ -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) {

This comment has been minimized.

@k8s-ci-robot

k8s-ci-robot Feb 7, 2018

Contributor

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

This comment has been minimized.

@Random-Liu
@@ -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) {

This comment has been minimized.

@k8s-ci-robot

k8s-ci-robot Feb 7, 2018

Contributor

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

@yujuhong

This comment has been minimized.

Contributor

yujuhong commented Feb 7, 2018

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

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels Feb 7, 2018

@Random-Liu

This comment has been minimized.

Member

Random-Liu commented Feb 7, 2018

Rebased and switched to v1alpha2.

@@ -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.

This comment has been minimized.

@yujuhong

yujuhong Feb 7, 2018

Contributor

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?

This comment has been minimized.

@Random-Liu

Random-Liu Feb 7, 2018

Member

Will do then.

This comment has been minimized.

@Random-Liu

Random-Liu Feb 7, 2018

Member

Done. Changed to FilesystemIdentifier.

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

@mrunalp

This comment has been minimized.

Contributor

mrunalp commented Feb 8, 2018

@sameo PTAL.

@mrunalp

This comment has been minimized.

Contributor

mrunalp commented Feb 8, 2018

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

@dashpole

This comment has been minimized.

Contributor

dashpole commented Feb 9, 2018

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

@dashpole

This comment has been minimized.

Contributor

dashpole commented Feb 9, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 9, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 9, 2018

[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:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 9, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 9, 2018

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

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

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation Random-Liu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment