Skip to content
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

Refactor util/mount interface in prep for moving out of k/k #68513

Merged
merged 1 commit into from Jun 28, 2019

Conversation

@codenrhoden
Copy link
Member

commented Sep 11, 2018

What this PR does / why we need it:
This patch refactors pkg/util/mount to be more usable outside of
Kubernetes. This is done by refactoring mount.Interface to only contain
methods that are not K8s specific. Methods that are not relevant to
basic mount activities but still have OS-specific implementations are
now found in a mount.HostUtils interface.

This is in preparation to migrate pkg/util/mount out of k/k, where it can be
more easily used by other storage related code, such as CSI drivers.

Which issue(s) this PR fixes:
partially addresses #64953

Special notes for your reviewer:
Take a close look at mount.Interface and mount.HostUtils interfaces to see if this is the correct separation.

This was originally 1 big PR that did everything, but was then broken into smaller pieces. All those smaller pieces have been merged.

Release Note:

NONE
@msau42

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

/ok-to-test
/assign

You can use hack/update-bazel.sh to automatically make the BUILD file updates

@codenrhoden codenrhoden force-pushed the codenrhoden:mount-refactor branch from 80f78e5 to e742d6e Sep 11, 2018

@codenrhoden

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

Thanks, BUILD files are updated and I pushed a new version with them.

@davidz627

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

thanks for working on this! I'll take a look soon.
I would suggest that the kubernetes specific mount packages stay as pkg/util/mount while the stuff thats going to go out of tree "soon" be called something else, maybe externalmount (thats not a great name I wouldn't recommend it) because from the kubernetes perspective the k8smount package is just the normal mount package.

/assign

@codenrhoden

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

Thanks @davidz627, your naming approach makes sense. Not sure what a better name than externalmount is, but I'll keep the K8s specific stuff at mount. I should perhaps change the title here to include WIP, as I was waiting to get feedback on the approach before doing all the needed changes.

@codenrhoden codenrhoden changed the title Refactor util/mount into common and K8s specific WIP: Refactor util/mount into common and K8s specific Sep 13, 2018

@msau42 msau42 referenced this pull request Sep 24, 2018

Merged

Containerized subpath #63143

@msau42 msau42 referenced this pull request Oct 16, 2018

Closed

Removing cloud provider dependencies to kubernetes/kubernetes #69585

39 of 39 tasks complete
@mcrute

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

/cc @mcrute

@k8s-ci-robot k8s-ci-robot requested a review from mcrute Oct 17, 2018

@codenrhoden codenrhoden force-pushed the codenrhoden:mount-refactor branch from e742d6e to 86f1125 Nov 7, 2018

@codenrhoden

This comment has been minimized.

Copy link
Member Author

commented Nov 7, 2018

I pushed some unfinished changes just to make sure the branch was up to date. Still have some specific in-tree volume providers to update, then work on failing tests. But this updates things such that the K8s specific mount code remains in a mount package, and the external stuff is now in externalmount (sorry, couldn't come up with a better name). Will keep working on this!

@mcrute

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

Migrating pkg/util/mount out of tree is also part of #69585 but has not been started. This package has other dependencies in pkg/util that would block its migration and I've got PRs open and some are merged for all of those which should make the actual migration out of tree for this a lot easier. Let me know if you run into any other blockers, I'm happy to lend a hand!

@codenrhoden codenrhoden force-pushed the codenrhoden:mount-refactor branch from 86f1125 to c5e9ee0 Nov 20, 2018

@codenrhoden

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

@msau42 updated based on your comments. 👍

Show resolved Hide resolved pkg/volume/csi/csi_mounter.go Outdated
err error
)
if !ok {
klog.V(2).Info(log("plugin volume host does not implement KubeletVolumeHost interface"))

This comment has been minimized.

Copy link
@msau42

msau42 Jun 11, 2019

Member

This seems like a fatal error if it fails. Can we try to initialize host util earlier and pass it in through csiMountMgr?

This comment has been minimized.

Copy link
@codenrhoden

codenrhoden Jun 11, 2019

Author Member

@msau42 I updated as you requested. the csiMountMgr struct is just created directly inside of csi_mounter.go, instead of a constructor of some type, so it's not quite as clean as I would have liked. There are a few other places in csi_mounter.go where the same cast is done, and they don't treat it as fatal. If they had all treated it as fatal, I would have just done the cast once and saved it inside of the csiPlugin struct instead.

This comment has been minimized.

Copy link
@msau42

msau42 Jun 11, 2019

Member

I think checking in NewMounter() and NewUnmounter() are fine.

@codenrhoden codenrhoden force-pushed the codenrhoden:mount-refactor branch from cbf509d to 6034c32 Jun 11, 2019

@msau42

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

/lgtm
/approve

Thanks so much for taking on this huge refactor!

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 11, 2019

@msau42

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

/assign @jsafrane

@codenrhoden

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

Awesome! I'm sorry I didn't get it in for 1.15... With KubeCon right before code freeze, I think reviewers were busy with many other things. :) I'll wrangle all the other needed approvers once code freeze lifts. Excited to move forward on this one.

@codenrhoden

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

/test pull-kubernetes-bazel-test

Refactor pkg/util/mount to be more reusable
This patch refactors pkg/util/mount to be more usable outside of
Kubernetes. This is done by refactoring mount.Interface to only contain
methods that are not K8s specific. Methods that are not relevant to
basic mount activities but still have OS-specific implementations are
now found in a mount.HostUtils interface.

@codenrhoden codenrhoden force-pushed the codenrhoden:mount-refactor branch from 6034c32 to be7da50 Jun 14, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jun 14, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@codenrhoden: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-local-e2e-containerized 3f87ee3 link /test pull-kubernetes-local-e2e-containerized

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@codenrhoden

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

/test pull-kubernetes-e2e-gce

@jsafrane

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 17, 2019

@@ -29,7 +29,6 @@ import (
type FakeMounter struct {

This comment has been minimized.

Copy link
@thockin

thockin Jun 17, 2019

Member

Fakes should go in a testing/ sub-pkg

This comment has been minimized.

Copy link
@codenrhoden

codenrhoden Jun 19, 2019

Author Member

I don't disagree. Since that part of the code wasn't modified as part of this PR, I'd be happy to try and restructure that in a follow-on PR, most likely after this package is moved out of k/k.

@codenrhoden

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

/assign @wojtek-t

@derekwaynecarr

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

/approve
/lgtm

@wojtek-t

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: codenrhoden, derekwaynecarr, jsafrane, msau42, wojtek-t

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

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

@codenrhoden

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

/test pull-kubernetes-bazel-build

@k8s-ci-robot k8s-ci-robot merged commit 2501a90 into kubernetes:master Jun 28, 2019

21 of 23 checks passed

pull-kubernetes-bazel-build Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation codenrhoden authorized
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@codenrhoden codenrhoden deleted the codenrhoden:mount-refactor branch Jun 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.