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

implement iscsi persistent disk volume plugin #4612

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@rootfs
Member

rootfs commented Feb 19, 2015

This pull request implemented an iSCSI disk volume plugin. The idea is to allow on-premise kube setup to persist data in a block volume through iSCSI channel and help DevOps run test in house even without access to GCE.

The code is heavily inspired by GCE Persistent Disk.

A sample pod is provided. This pod runs on a Fedora 21 based node and persists to iSCSI backed volume.

Background information, use cases, high level design, and how-to can be found here

@thockin @markturansky @smarterclayton @eparis

@googlebot googlebot added the cla: yes label Feb 19, 2015

@thockin thockin self-assigned this Feb 19, 2015

"time"

"github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet/volume"
"github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet/volume/gce_pd"

This comment has been minimized.

@markturansky

markturansky Feb 19, 2015

Member

I don't think details from one plugin should leak into another.

If there are generic mounting utils in GCE that other volumes would benefit from, it is probably better to refactor those commons things into a package all volumes can use.

This comment has been minimized.

@rootfs

rootfs Feb 19, 2015

Member

add @thockin, i think we probably have some code to refactor and share.

@markturansky

This comment has been minimized.

Member

markturansky commented Feb 19, 2015

{
"id": "iscsipd",
"kind": "Pod",
"apiVersion": "v1beta1",

This comment has been minimized.

@markturansky

markturansky Feb 19, 2015

Member

It is encouraged to use v1beta3 for all examples going forward.

This comment has been minimized.

@rootfs

rootfs Feb 19, 2015

Member

will do, thanks

This comment has been minimized.

@thockin

thockin Feb 19, 2015

Member

I was not aware we had started that - some of v1b3 is still changing...

This comment has been minimized.

@markturansky

markturansky Feb 20, 2015

Member

I was told to use v1b3 examples. I assumed that was broadly applied. I can correct my examples...

This comment has been minimized.

@markturansky

markturansky Mar 6, 2015

Member

Please include this in examples_test.go.

Also, all other examples are being remade into v1beta3. Is it not time for this one, too?

This comment has been minimized.

@thockin

thockin Mar 6, 2015

Member

I see this comment is not addressed - we have started converting examples over to v1beta3...

return errors.New("path busy")
}
//glog.Infof("iSCSIPersistentDisk detach disk: umount:%s", globalPDPath)
if err := iscsi.mounter.Unmount(globalPDPath, 0); err != nil {

This comment has been minimized.

@calfonso

calfonso Feb 19, 2015

Contributor

Is there a benefit to using the mounter.Unmount/Mount approach over using syscall.Mount/Unmount?

This comment has been minimized.

@rootfs

rootfs Feb 19, 2015

Member

mounter is an interface that helps setup a unit test. Under the hood, it calls syscall.

@@ -221,6 +224,17 @@ type GCEPersistentDisk struct {
ReadOnly bool `json:"readOnly,omitempty"`
}

// A ISCSI Disk can only be mounted as read/write once.
type ISCSIDisk struct {

This comment has been minimized.

@thockin

thockin Feb 20, 2015

Member

iSCSI is a lot less cloud-specific than GCE PD, so maybe the answer is yes, but...

Do we really want to expose this as a discrete type? Maybe we want to glue this into @markturansky 's larger storage refactoring instead? This is just another form of pet storage...

This comment has been minimized.

@markturansky

markturansky Feb 20, 2015

Member

@thockin I understand and agree with the idea that users defining their own pets in VolumeSource will eventually go away (replacing the network volumes with a struct like you suggested in #4055).

This plugin and @calfonso NFSMount plugin are orthogonal issues that can be developed independently of what I'm doing, but then refactored into whatever design comes next. I have no problem doing that refactoring work as a follow-on task.

This comment has been minimized.

@rootfs

rootfs Feb 20, 2015

Member

Adding to that, iSCSI has some cloud affinity, it is the default implementation for OpenStack Cinder.

This comment has been minimized.

@thockin

thockin Feb 20, 2015

Member

Sure, but iSCSI and NFS could both be implemented by different kinds of backends in different environments. while GCE PD acts like a block device it does not implement an iSCSI protocol. :)

@@ -23,7 +23,7 @@ import (
// Examines /proc/mounts to find the source device of the PD resource and the
// number of references to that device. Returns both the full device path under
// the /dev tree and the number of references.
func getMountRefCount(mounter mount.Interface, mountPath string) (string, int, error) {
func GetMountRefCount(mounter mount.Interface, mountPath string) (string, int, error) {

This comment has been minimized.

@thockin

thockin Feb 20, 2015

Member

This should all be extracted to pkg/util/mount or something like that

@@ -26,7 +26,7 @@ import (
// Determine if a directory is a mountpoint, by comparing the device for the directory
// with the device for it's parent. If they are the same, it's not a mountpoint, if they're
// different, it is.
func isMountPoint(file string) (bool, error) {
func IsMountPoint(file string) (bool, error) {

This comment has been minimized.

@thockin

thockin Feb 20, 2015

Member

likewise


"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
"github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet/volume"
"github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet/volume/gce_pd"

This comment has been minimized.

@thockin

thockin Feb 20, 2015

Member

yeah, no cross-linkages, please

@thockin

This comment has been minimized.

Member

thockin commented Feb 20, 2015

API question aside, I have a big problem with the duplication. I think we can implement this in 2 layers.

First is a utility lib that handles mounting and unmounting an arbitrary block device - it will need a bunch of stuff injected to parameterize the control.

Second is GCEPD and iSCSI (and eventually more) which are implemented purely in terms of the block-device layer.

That way, when we realize we've (again) done something dumb in the unmount-and-cleanup path, we fix it once.

@googlebot

This comment has been minimized.

googlebot commented Feb 24, 2015

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot googlebot added cla: no and removed cla: yes labels Feb 24, 2015

@rootfs rootfs force-pushed the rootfs:iscsi-pd-merge branch from 5257571 to 8a4af65 Feb 24, 2015

@googlebot

This comment has been minimized.

googlebot commented Feb 24, 2015

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Feb 24, 2015

@rootfs rootfs force-pushed the rootfs:iscsi-pd-merge branch from 8a4af65 to 38d5062 Feb 24, 2015

@rootfs

This comment has been minimized.

Member

rootfs commented Feb 24, 2015

@thockin here is a refactor:

  • move mount_util from gce-pd to volume package
  • move pdManager and SetUp/TearDown utility methods into disk package, and let iscsi-pd to be the first consumer of this package.

I haven't changed gce-pd yet. If these look good to you, I'll update gce-pd in another commit.

@rootfs rootfs force-pushed the rootfs:iscsi-pd-merge branch 6 times, most recently from 1710b7d to d2d009c Feb 24, 2015

@rootfs rootfs force-pushed the rootfs:iscsi-pd-merge branch 2 times, most recently from 9c15a72 to 7812a67 Mar 6, 2015

return false
}

func makePDNameInternal(host volume.Host, portal string, iqn string, lun string) string {

This comment has been minimized.

@thockin

thockin Mar 6, 2015

Member

can you write a comment about the structure of the name

This comment has been minimized.

@thockin

thockin Mar 6, 2015

Member

e.g. why you use multiple levels of path - I am not sure teh cleanup & recovery path will handle this properly..

This comment has been minimized.

@rootfs

rootfs Mar 6, 2015

Member

will do


type ISCSIDiskUtil struct{}

func (util *ISCSIDiskUtil) MakeGlobalPDName(disk interface{}) string {

This comment has been minimized.

@thockin

thockin Mar 6, 2015

Member

Ugh, is there no way to factor this without bouncing through interface{} ?

This comment has been minimized.

@rootfs

rootfs Mar 6, 2015

Member

not to my awareness :)

This comment has been minimized.

@thockin

thockin Mar 6, 2015

Member

I have a proposal to make this an easier review.

First offer a PR that makes this transformation on GCE PD so we can evaluate things like interface{} - I bet there's a cleaner factoring.

Second, implement iscsi in those terms

This comment has been minimized.

@rootfs

rootfs Mar 6, 2015

Member

ok, let's do this way. any contact you know has ideas other than interface{}?

This comment has been minimized.

@thockin

thockin Mar 6, 2015

Member

I think the problem is that you are trying to go from type-specific code to type-agnostic code to type-specific code. I think there might be a different way to slice it, but I need to look at it more. I can probably live with interface{} right now, but I'd like to see how it changes the PD case.

exist := probeDevicePath(devicePath, 1)
if exist == false {
// discover iscsi target
_, err := iscsi.execCommand("iscsiadm", []string{"-m", "discovery", "-t", "sendtargets", "-p",

This comment has been minimized.

@thockin

thockin Mar 6, 2015

Member

maybe we should check for the existence of iscsiadm in the Plugin CanSupport() function, and log something to the effect of "I would handle this but I don't have iscsiadm"

This comment has been minimized.

@rootfs

rootfs Mar 6, 2015

Member

if no iscsiadm, will exec return error? will add a check for that.

This comment has been minimized.

@thockin

thockin Mar 6, 2015

Member

I'm 99% sure it will return an error - if iscsiadm has a -v flag or something that is ideal.

return err
}
// login to iscsi target
_, err = iscsi.execCommand("iscsiadm", []string{"-m", "node", "-p",

This comment has been minimized.

@thockin

thockin Mar 6, 2015

Member

don't line-wrap in arbitrary places - I'd rather have one long line to read

return refCount, nil
}

// given a device path, find the mount on that device from /proc/mounts

This comment has been minimized.

@thockin

thockin Mar 6, 2015

Member

Not godoc compatible (should start with function name). Please document the return values.

@@ -76,3 +80,40 @@ func GetMountRefs(mounter Interface, mountPath string) ([]string, error) {
}
return refs, nil
}

// given a device path, find its reference count from /proc/mounts
func GetDeviceRefCount(mounter Interface, deviceName string) (int, error) {

This comment has been minimized.

@thockin

thockin Mar 6, 2015

Member

Not godoc compatible (should start with function name).

This comment has been minimized.

@rootfs

rootfs Mar 6, 2015

Member

will do, and any hook can detect it?

This comment has been minimized.

@thockin

thockin Mar 6, 2015

Member

no, it's just general feedback - public functions should have godoc-compatible comments

This comment has been minimized.

@rootfs

rootfs Mar 6, 2015

Member

nice to know, thanks

@thockin

This comment has been minimized.

Member

thockin commented Mar 6, 2015

Thanks for undertaking this refactoring. The biggest problem I have is that the refactoring is tightly coupled to the iscsi changes. Also, we should refactor gce_pd to ensure the refactoring works. I'm worried about the interface{} and I think we can do better, but it's hard to see with the new iscsi code mixed in.

@swagiaal swagiaal referenced this pull request Mar 11, 2015

Merged

AWS EBS volume support #5138

@rootfs rootfs force-pushed the rootfs:iscsi-pd-merge branch 2 times, most recently from 8f6e29b to 79c1e22 Mar 13, 2015

@googlebot

This comment has been minimized.

googlebot commented Mar 13, 2015

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot googlebot added cla: no and removed cla: yes labels Mar 13, 2015

rootfs and others added some commits Feb 23, 2015

fix header so boilerpate can pass
Signed-off-by: Huamin Chen <hchen@redhat.com>
- refactor kubelet volume mount util
- refactor persistent disk:
   * move pdManager to package disk
   * extract common setup and teardown logic

Signed-off-by: Huamin Chen <hchen@redhat.com>
address review feedbacks
Signed-off-by: Huamin Chen <hchen@redhat.com>
Add Docker caching support to CoreOS' clusters.
    a more idiomatic take on #4378. as a bonus, and to be nice on
    resources, an ultra lightweight docker-registry container is
    consumed.

Signed-off-by: António Meireles <antonio.meireles@reformi.st>
simplify fleet invocation across CoreOS cloud-configs.
Signed-off-by: António Meireles <antonio.meireles@reformi.st>
add missing iscsi pod definition
Signed-off-by: Huamin Chen <hchen@redhat.com>

@rootfs rootfs force-pushed the rootfs:iscsi-pd-merge branch from 97bd38b to 3f6125b Mar 13, 2015

@googlebot

This comment has been minimized.

googlebot commented Mar 13, 2015

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@rootfs rootfs referenced this pull request Mar 16, 2015

Merged

add iscsi volume plugin #5506

@rootfs

This comment has been minimized.

Member

rootfs commented Mar 16, 2015

moved to a cleaner PR #5506
@thockin @eparis @markturansky

@rootfs rootfs closed this Mar 16, 2015

@thockin

This comment has been minimized.

Member

thockin commented Mar 16, 2015

Will review ASAP

On Mon, Mar 16, 2015 at 7:26 AM, Huamin Chen notifications@github.com
wrote:

Closed #4612 #4612
.


Reply to this email directly or view it on GitHub
#4612 (comment)
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment