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 glusterfs volume plugin #6174

Merged
merged 1 commit into from Apr 7, 2015

Conversation

Projects
None yet
10 participants
@rootfs
Copy link
Member

rootfs commented Mar 30, 2015

use glusterfs as a new volume plugin

@googlebot googlebot added the cla: yes label Mar 30, 2015

@rootfs rootfs force-pushed the rootfs:wip-gluster branch from c4b3ce1 to b3b0bfa Mar 30, 2015

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Mar 30, 2015

@vmarmol

This comment has been minimized.

Copy link
Contributor

vmarmol commented Mar 30, 2015

Assigning to @thockin, I know you're busy so feel free to re-assign :)

}
```

The parameters are explained as the followings. **hosts** is an array of Gluster hosts. **kubelet** is optimized to avoid mount storm, it will randomly pick one from the hosts to mount. If this host is unresponsive, the next host in the array is automatically selected. **path** is the Glusterfs volume name. **mountOption** is the mount time options. **helper** is used if mount is executed inside a Super Privileged Container. This **helper** assumes the name of the Super Proviliged Container is *gluster_spc*.

This comment has been minimized.

@erictune

erictune Mar 30, 2015

Member

What happens if one of the hosts goes away during the the time that the pod is running? Is there no way to mount in such a way that the glusterfs VFS driver knows to try multiple hosts?

This comment has been minimized.

@rootfs

rootfs Mar 30, 2015

Member

gluster client doesn't know the cluster topology before mount.

After mount, host disconnectivity won't bring the pod or app down. Once mount succeeds, glusterfs client knows the cluster layout through consistent hashing. And thus subsequent I/O won't be lost as long as at least one replica is alive.


See the [v1beta3/](v1beta3/) folder examples.

```js

This comment has been minimized.

@vishh

vishh Mar 30, 2015

Member

nit: Clarify that this is the volume spec for the POD.


### Create a POD

See the [v1beta3/](v1beta3/) folder examples.

This comment has been minimized.

@vishh

vishh Mar 30, 2015

Member

nit: Rephrase this sentence as "Detailed examples here"


The examples consist of:
- A pod that runs on hosts that install Glusterfs client package.
- A pod that runs on hosts that don't install Glusterfs client package and use [Super Privileged Container](http://developerblog.redhat.com/2014/11/06/introducing-a-super-privileged-container-concept/) to mount Glusterfs.

This comment has been minimized.

@vishh

vishh Mar 30, 2015

Member

Is it necessary to support the SPC use case? IIUC there is an implicit dependency on the SPC pod to be running.

This comment has been minimized.

@rootfs

rootfs Mar 31, 2015

Member

You are right, this implicit dependency is host (and vendor) specific. The SPC helper is provided for convenience for hosts that have SPC running.

Such dependency can be managed by the plugin, but I feel gluster is not the only use case requiring a utility SPC. Would love to address this issue in a more general framework.

This comment has been minimized.

@smarterclayton

smarterclayton Mar 31, 2015

Contributor

We need to roll SPC into the discussion on Security Contexts @pweil- - once paul has basic security contexts running we should look at how we integrate SPC.

----- Original Message -----

@@ -0,0 +1,44 @@
+## Glusterfs
+
+Glusterfs is an open source scale-out
filesystem. These examples provide information about how to allow Docker
containers use Glusterfs volumes.
+
+The examples consist of:
+- A pod that runs on hosts that install Glusterfs client package.
+- A pod that runs on hosts that don't install Glusterfs client package and
use Super Privileged
Container

to mount Glusterfs.

You are right, this implicit dependency is host (and vendor) specific. The
SPC helper is provided for convenience for hosts that have SPC running.

Such dependency can be managed by the plugin, but I feel gluster is not the
only use case requiring a utility SPC. Would love to address this issue in a
more general framework.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6174/files#r27481057

This comment has been minimized.

@erictune

erictune Mar 31, 2015

Member

Suggest leaving out the SPC example from the initial PR -- require preinstalled GlusterFS client.
Later, we can iterate on SPC and Security Contexts and such.

This comment has been minimized.

@rootfs

rootfs Mar 31, 2015

Member

@erictune i am going to remove all references to SPC, but still leave helper in place.

@@ -375,6 +375,7 @@ func TestValidateVolumes(t *testing.T) {
{Name: "gcepd", VolumeSource: api.VolumeSource{GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{"my-PD", "ext4", 1, false}}},
{Name: "gitrepo", VolumeSource: api.VolumeSource{GitRepo: &api.GitRepoVolumeSource{"my-repo", "hashstring"}}},
{Name: "secret", VolumeSource: api.VolumeSource{Secret: &api.SecretVolumeSource{"my-secret"}}},
{Name: "glusterfs", VolumeSource: api.VolumeSource{Glusterfs: &api.GlusterfsVolumeSource{[]string{"host1", "host2"}, "path", "", ""}}},

This comment has been minimized.

@vishh

vishh Mar 30, 2015

Member

Add tests for the failure cases as well.

This comment has been minimized.

@rootfs
return nil
}

func (glusterfsVolume *glusterfs) execMount(hosts []string, path string, mountpoint string, option string, helper string) error {

This comment has been minimized.

@vishh

vishh Mar 30, 2015

Member

Is it possible to avoid execing mount ... and instead use syscall.Mount?

This comment has been minimized.

@rootfs

rootfs Mar 31, 2015

Member

The difficulty of calling syscall.Mount is in injecting helper utility.

glog.Errorf("Glusterfs: IsMountpoint check failed: %v", mntErr)
return err
}
if mountpoint {

This comment has been minimized.

@vishh

vishh Mar 30, 2015

Member

nit: How about refactoring this to be:

defer os.Remove(dir)
if !mountpoint {
   return err
}
Unmount(...)
ValidateUnmount(...)

This comment has been minimized.

@rootfs

rootfs Mar 31, 2015

Member

defer is a good idea :)

This comment has been minimized.

@rootfs

rootfs Mar 31, 2015

Member

it seems other volume plugins are doing the same. we'll have another project to refactor common code.

path := glusterfsVolume.path
os.MkdirAll(dir, 0750)
err = glusterfsVolume.execMount(glusterfsVolume.hosts, path, dir, glusterfsVolume.option, glusterfsVolume.helper)
if err != nil {

This comment has been minimized.

@vishh

vishh Mar 30, 2015

Member

nit: To improve readability why not return nil if err == nil.

return err
}
if !mountpoint {
return os.Remove(dir)

This comment has been minimized.

@vishh

vishh Mar 30, 2015

Member

Suggestion: Use os.RemoveAll since it will not return error if dir is not found. This helps make TearDown idempotent.

return false
}

func (plugin *glusterfsPlugin) GetAccessModes() []api.AccessModeType {

This comment has been minimized.

@vishh

vishh Mar 30, 2015

Member

Is this plugin really a PersistentVolumePlugin? Persistent Volumes follow the claim and use model. This plugin seems like a VolumePlugin

This comment has been minimized.

@pires

pires Mar 31, 2015

Member

This plugin seems like a VolumePlugin

Agreed.

This comment has been minimized.

@rootfs

rootfs Mar 31, 2015

Member

loop in @markturansky
there is a followup on the persistent volume front - once gluster is made to be able to handle it.

This comment has been minimized.

@markturansky

markturansky Mar 31, 2015

Member

All volumes are VolumePlugins. Some are PersistentVolumePlugins if they are going to be provisioned by the admin and claimed by the user.

Is there a use case where Gluster is best for pod authors who also know it is Gluster? As opposed to asking for a persistent volume and getting one (might be Gluster, might be EBS, only the Admin knows).

This comment has been minimized.

@vishh

vishh Apr 1, 2015

Member

Since none of the existing examples treat Gluster as a Persistent Volume, shall we treat it as a VolumePlugin in this PR?

This comment has been minimized.

@rootfs

rootfs Apr 6, 2015

Member

the gluster is going to be provisioned by the admin and claimed by the user.

This comment has been minimized.

@vishh

vishh Apr 6, 2015

Member

This PR seems to implement Gluster as a node volume. Once gluster becomes a persistent volume, can we add this part?

This comment has been minimized.

@rootfs

rootfs Apr 6, 2015

Member

there will be a followup PR once persistent volume is completed.

This comment has been minimized.

@markturansky

markturansky Apr 6, 2015

Member

GetAccessModes as a persistent volume function is already merged and @rootfs implemented and tested it correctly.

The only thing needed to truly make it a persistent volume is to add a pointer to the PersistentVolumeSource struct in types. That bit along with examples of persistent usage make a good follow-on PR, IMO.

This comment has been minimized.

@rootfs

@vishh vishh assigned vishh and unassigned thockin Mar 30, 2015

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Mar 30, 2015

@rootfs: Thanks for the PR! Can you also add an e2e test? Even if the test is not run as part of our regular runs, it will be useful to validate any future changes to the plugin.

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Mar 30, 2015

sure, will update the PR with these feedbacks.

@rootfs rootfs force-pushed the rootfs:wip-gluster branch from 2ef0ac2 to eb38204 Mar 31, 2015

// Optional: helper is the helper utility to mount command
// it can encapsulate the mount command,
// so mount can be run in a different namespace, if necessary
Helper string `json:"helper,omitempty"`

This comment has been minimized.

@erictune

erictune Mar 31, 2015

Member

Suggest leaving this out of first version. We may be able to come up with more general mechanism.


// Optional: mountOptions is the mount time options
// including readonly
MountOpt string `json:"mountOptions,omitempty"`

This comment has been minimized.

@erictune

erictune Mar 31, 2015

Member

Do you anticipate options other than readonly? If so, which? If not, make this just ReadOnly bool to match other volume sources.

This comment has been minimized.

@rootfs

rootfs Mar 31, 2015

Member

readonly is an option, i.e. -o ro, although this is necessary for disk based volumes (pd or iscsi, for the sake of bind mount), singling it out as a special option for a filesystem mount, IMHO, is unnecessary.

This comment has been minimized.

@erictune

erictune Mar 31, 2015

Member

Are there other mount options that are useful with glusterfs?

This comment has been minimized.

@rootfs

rootfs Mar 31, 2015

Member

Yes, there are a few mount time options (mostly from FUSE) that have security and performance impact.

// GlusterfsVolumeSource represents a Glusterfs Mount that lasts the lifetime of a pod
type GlusterfsVolumeSource struct {
// Required: hosts are Glusterfs hosts address array
Hosts []string `json:"hosts"`

This comment has been minimized.

@erictune

erictune Mar 31, 2015

Member

This seems like not an ideal thing to require every user to put in a pod spec. It requires specific knowledge of the cluster's nodes, which is something we would like to hide from the users where possible. And, the set of Hosts might change quite a bit over time, making pod specs get out of date.

What are some other alternatives?

  • require user to setup dynamic DNS and put that dns name here?
  • setup a kubernetes service for the set of hosts (whether or not the hosts actually run in pods, this is possible). Refer to that service here. Kubelet pulls the service's endpoints at mount time.
  • maintain a list of Hosts in a kubenetes endpoints object and refer to that here. Kubelet can pull endpoints at mount time.

This comment has been minimized.

@rootfs

rootfs Mar 31, 2015

Member

I'll see what I can do, thanks for all the great ideas!

This comment has been minimized.

@yllierop

yllierop Mar 31, 2015

Contributor

👍 I personally like @erictune's suggestion requiring dynamic DNS. Seems like a clean solution to the issue.

This comment has been minimized.

@rootfs

rootfs Mar 31, 2015

Member

The dynamic DNS solution is good for services that expose 1-N name-to-IP mappings. This is however not ideal solution for glusterfs.

  • Operation complexity. The gluster cluster may not be in the same managed network domain. Sync-up DNS entries between gluster cluster and kube cluster adds complexity.
  • Scaling. If you have a large glusterfs setup, scaling DNS mapping tables will be tremendous.
  • Performance. In a large kube setup, when all minions mount a gluster filesystem and they all query DNS for resolution, there is a good chance for something going wrong.
  • Mount storm. If DNS servers reply to all the minons the same order of gluster hosts, then minions could end up mounting the same gluster host, this is something I don't want to see.
  • HA. a DNS lookup may not be able to get all the entries. If a unfortunate query gets only a handful entries that happens to point to a unresponsive gluster hosts, mount then fail.

This comment has been minimized.

@vishh

vishh Mar 31, 2015

Member

@rootfs: +1 on @erictune's thought on not requiring hosts in the API. What about using a kubernetes service or an endpoints object?

This comment has been minimized.

@rootfs

rootfs Mar 31, 2015

Member

working on the endpoint idea now. stay tuned :)

This comment has been minimized.

@pires

pires Mar 31, 2015

Member

I agree with @rootfs comments regarding DNS, as it may be tricky to have. Service with static Endpoints should be the way to go - so a service declaration example is needed.

This comment has been minimized.

@smarterclayton

smarterclayton Mar 31, 2015

Contributor

----- Original Message -----

@@ -396,6 +398,24 @@ type NFSVolumeSource struct {
ReadOnly bool json:"readOnly,omitempty"
}

+// GlusterfsVolumeSource represents a Glusterfs Mount that lasts the
lifetime of a pod
+type GlusterfsVolumeSource struct {

  • // Required: hosts are Glusterfs hosts address array
  • Hosts []string json:"hosts"

The dynamic DNS solution is good for services that expose 1-N name-to-IP
mappings. This is however not ideal solution for glusterfs.

  • Operation complexity. The gluster cluster may not be in the same managed
    network domain. Sync-up DNS entries between gluster cluster and kube cluster
    adds complexity.

Today. Probably not in the future.

  • Scaling. If you have a large glusterfs setup, scaling will be tremendous.
  • Performance. In a large kube setup, when all minions mount a gluster
    filesystem and they all query DNS for resolution, there is a good chance for
    something goes wrong.
  • Mount storm. If DNS servers reply to all the host the same order of gluster
    hosts, then minions could end up mounting the same gluster host, this is
    something I don't want to see.

With SkyDNS we are round-robining, but agree this could be an issue.

  • HA. a DNS lookup may not be able to get all the entries. If a unfortunate
    query gets only a handful entries that happens to point to a unresponsive
    gluster hosts, mount then fail.

This ties into headless services exposed via DNS, but we have this working already in OpenShift and I'm fairly certain that it will scale to at least the cluster levels we're discussing now.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6174/files#r27501247

This comment has been minimized.

@rootfs

rootfs Mar 31, 2015

Member

@erictune @vishh let me know what you think about the new endpoints code.

@@ -193,6 +193,8 @@ type VolumeSource struct {
Secret *SecretVolumeSource `json:"secret"`
// NFS represents an NFS mount on the host that shares a pod's lifetime
NFS *NFSVolumeSource `json:"nfs"`
// Glusterfs represents a Glusterfs mount on the host that shares a pod's lifetime
Glusterfs *GlusterfsVolumeSource `json:"glusterfs"`

This comment has been minimized.

@markturansky

markturansky Mar 31, 2015

Member

You'll also want to add this to PersistentVolumeSource to make it a provisionable resource.

If Gluster is not to be exposed to the end user (only the admin provisions it, users claim it), then after the PV framework is fully merged you can remove GFS from VolumeSource and leave it in PVS. This hides it completely from the pod author.

@rootfs rootfs force-pushed the rootfs:wip-gluster branch from dbdd993 to 287c801 Mar 31, 2015

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Apr 1, 2015

@vishh @erictune if there is anything else, please let me know.

// GlusterfsVolumeSource represents a Glusterfs Mount that lasts the lifetime of a pod
type GlusterfsVolumeSource struct {
// Required: hosts is the endpoint name that details Glusterfs topology
Hosts string `json:"hosts"`

This comment has been minimized.

@erictune

erictune Apr 1, 2015

Member

Let's not call it Hosts anymore, since that doesn't immediately make people think of a Kubernetes Endpoint name,

This comment has been minimized.

@rootfs

rootfs Apr 1, 2015

Member

@erictune i am terrible with names, any suggestions?

This comment has been minimized.

@erictune

erictune Apr 1, 2015

Member

Endpoints?

This comment has been minimized.

@rootfs

rootfs Apr 1, 2015

Member

deal :)

Path string `json:"path"`

// Optional: mountOptions is the mount time options such as "rw" or "ro"
MountOpt string `json:"mountOptions,omitempty"`

This comment has been minimized.

@erictune

erictune Apr 1, 2015

Member

I don't think we should do it this way. I think we should have one Go struct member for each option we want to support. Here's why:

  • individual options like "ReadOnly" can have individual description: tags
  • a user browsing the ui in someplace like swaggerui can see the options without diving into the glusterfs.mount man page
  • we probably don't want to allow users to set all the possible glusterfs mount options, for example direct-io-mode. Having individual json options is a way of whitelisting options.
  • we may need to do validation on individual options. I'd rather that validation code not have to parse a mount option string.
  • allows us to implement an option's effect differently (for example there might be some reason why we want to mount the glusterfs read-write, tweak it, and then bind mount it into the container readonly
  • allows kubernetes API to evolve independently of glusterfs and of varying versions of kernels and node support for glusterfs.
  • consistency with other volume types that have separate struct member for "readonly"

This comment has been minimized.

@rootfs

rootfs Apr 1, 2015

Member

this (whitelisting mount options) sounds a massive project better suited for a separate PR (along with making other volume plugins comply). I would rather like blacklist, if possible.

readonly can be resurrected, i agree this consistency breakage is my fault.

This comment has been minimized.

@erictune

erictune Apr 1, 2015

Member

I think figuring out which options is safe is a massive project. But we should not expose unsafe ones in our first step. Readonly is obviously safe. So, lets just have Readonly bool.

This comment has been minimized.

@erictune

erictune Apr 1, 2015

Member

Problem with blacklist is that if glusterfs adds a dangerous option, and a cluster admin upgrades cluster, then there is a security hole. It puts the burden on Kubernetes project to track everything Gluster project does. Not the model we are used to.

This comment has been minimized.

@rootfs

rootfs Apr 1, 2015

Member

ok, i'll replace mountOption with readOnly for this iteration.

@@ -0,0 +1,19 @@
{

This comment has been minimized.

@erictune

erictune Apr 1, 2015

Member

call this file "glusterfs-endpoints" to avoid confusion with Service kind?

This comment has been minimized.

@rootfs

rootfs Apr 1, 2015

Member

deal


The parameters are explained as the followings. **hosts** is endpoint name that defines Gluster service. **kubelet** is optimized to avoid mount storm, it will randomly pick one from the hosts to mount. If this host is unresponsive, the next host in the array is automatically selected. **path** is the Glusterfs volume name. **mountOption** is the mount time options, it can cotains standard mount time options such as *ro* (readonly). **helper** can be a command that can be executed prior to mounting the filesystem.

Detailed POD information can be found at [v1beta3/](v1beta3/)

This comment has been minimized.

@erictune

erictune Apr 1, 2015

Member

You need to tell the user to edit glusterfs-service.json and put in the right IPs.

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Apr 1, 2015

@vishh all good points. latest push fixed them. thanks.

}
```

The parameters are explained as the followings. **endpoints** is endpoint name that defines Gluster service. **kubelet** is optimized to avoid mount storm, it will randomly pick one from the hosts to mount. If this host is unresponsive, the next host in the array is automatically selected. **path** is the Glusterfs volume name. **readOnly** is the boolean that sets the mountpoint readOnly or readWrite. **helper** can be a command that can be executed prior to mounting the filesystem.

This comment has been minimized.

@vishh

vishh Apr 1, 2015

Member

nit s/that defines Gluster/that represents a Gluster/

This comment has been minimized.

@rootfs

@rootfs rootfs force-pushed the rootfs:wip-gluster branch from 908d6cb to a27ddb6 Apr 1, 2015

}
```

The parameters are explained as the followings. **endpoints** is endpoints name that represents a Gluster cluster configuration. **kubelet** is optimized to avoid mount storm, it will randomly pick one from the hosts to mount. If this host is unresponsive, the next Gluster host in the endpoints is automatically selected. **path** is the Glusterfs volume name. **readOnly** is the boolean that sets the mountpoint readOnly or readWrite. **helper** can be a command that can be executed prior to mounting the filesystem.

This comment has been minimized.

@vishh

vishh Apr 1, 2015

Member

nit: "randomly pick a host from 'endpoints' ".
How about describing one config item per line?

This comment has been minimized.

@rootfs

rootfs Apr 2, 2015

Member

nice reading, fixed.

ReadOnly bool `json:"readOnly,omitempty"`

// Optional: helper is the helper utility executed prior to mount command
Helper string `json:"helper,omitempty"`

This comment has been minimized.

@vishh

vishh Apr 1, 2015

Member

As @erictune mentioned earlier, can you get rid of "Helper" for now? If there is a real need, we can add it in the future.

This comment has been minimized.

@rootfs

rootfs Apr 2, 2015

Member

yes, we actually do need it on our setup :)

glog.Errorf("Glusterfs: failed to get endpoints %s[%v]", ep_name, err)
return nil, err
}
glog.Infof("Glusterfs: endpoints %v", ep)

This comment has been minimized.

@vishh

vishh Apr 1, 2015

Member

Does this have to be an info log? Can it be V(1) or V(2)?

arg := []string{"-t", "glusterfs", hosts.Subsets[i%l].Addresses[0].IP + ":" + path, mountpoint}
mountArgs = append(arg, opt...)
glog.Infof("Glusterfs: mount cmd: mount %v", strings.Join(mountArgs, " "))
command = glusterfsVolume.exe.Command("mount", mountArgs...)

This comment has been minimized.

@vishh

vishh Apr 1, 2015

Member

Use the mount util here instead of exec

This comment has been minimized.

@rootfs

rootfs Apr 2, 2015

Member

i love to use it but since helper is here, I want to reuse some of the struct.

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Apr 1, 2015

@rootfs: Almost there :) Your recent rebases are probably hiding some of the old comments.

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Apr 1, 2015

I will try to take a look at this soon, sorry for the delay

@rootfs rootfs force-pushed the rootfs:wip-gluster branch 2 times, most recently from 9c49847 to 5781348 Apr 2, 2015

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Apr 2, 2015

@vishh i went through the hidden comments, the following two things I owe you and @erictune will be submitted separately as an enhancement, together with some other things in my backlog.

  • a script that generates endpoint
  • an e2e test
@erictune

This comment has been minimized.

Copy link
Member

erictune commented Apr 2, 2015

I think we need to talk more about helper field before this merges.
On Apr 2, 2015 7:17 AM, "Huamin Chen" notifications@github.com wrote:

@vishh https://github.com/vishh i went through the hidden comments, the
following two things I owe you and @erictune https://github.com/erictune
will be submitted separately as an enhancement, together with some other
things in my backlog.

  • a script that generates endpoint
  • an e2e test


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

@erictune

This comment has been minimized.

Copy link
Member

erictune commented Apr 3, 2015

We met and discussed this PR. rootfs and steve watt explained goal to run glusterfs easily on OSes that don't include glusterfs fuse daemon, such as RH Atomic. Vish and I liked the idea of a fuse daemon per pod, but didn't like the idea of the user providing part of the command line for a root binary. So, we agreed to remove Helper field from this PR, and try to cooperate to find a way to accomplish the goal another way in a future PR. That seemed to depend in part on a little clearer picture of how glusterfs file tree is to be mapped into pods (e.g. do all pods see whole file tree, or subset that is different per pod), and how this interacts with Claims.

Anyhow It looks like Helper is gone, so this LGTM. I still will let Vish have a final look.

"endpoints": "glusterfs-cluster",
"path": "kube_vol",
"readOnly": true,
"helper": ""

This comment has been minimized.

@vishh

vishh Apr 6, 2015

Member

Remove helper from here.

This comment has been minimized.

@rootfs

rootfs Apr 6, 2015

Member

nice catch, fixed, thanks

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Apr 6, 2015

@rootfs: Just a few more comments to be resolved. Once you address them, I will merge this PR.

@rootfs rootfs force-pushed the rootfs:wip-gluster branch 3 times, most recently from 03ca9bd to c525667 Apr 6, 2015

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Apr 6, 2015

@rootfs: Just one last thing- can you squash your commits?

@rootfs rootfs force-pushed the rootfs:wip-gluster branch from c525667 to 14ac703 Apr 6, 2015

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Apr 6, 2015

LGTM

@vishh vishh added the lgtm label Apr 6, 2015

implement glusterfs volume plugin
Signed-off-by: Huamin Chen <hchen@redhat.com>

@rootfs rootfs force-pushed the rootfs:wip-gluster branch from 14ac703 to a278cee Apr 7, 2015

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Apr 7, 2015

@vishh rebased to latest master.

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Apr 7, 2015

Not sure why this was not merged yesterday. I apologize for the delay!

vishh added a commit that referenced this pull request Apr 7, 2015

Merge pull request #6174 from rootfs/wip-gluster
implement glusterfs volume plugin

@vishh vishh merged commit c1ef9be into kubernetes:master Apr 7, 2015

3 of 4 checks passed

Shippable Builds failed on Shippable
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 54.08%
Details
// GlusterfsVolumeSource represents a Glusterfs Mount that lasts the lifetime of a pod
type GlusterfsVolumeSource struct {
// Required: EndpointsName is the endpoint name that details Glusterfs topology
EndpointsName string `json:"endpoints"`

This comment has been minimized.

@thockin

thockin Apr 8, 2015

Member

Is the assumption that endpoints for gluster lie outside the kubernetes cluster? I am a bit anxious about direct creation of endpoints without a service (now that we have headless services) since it lays a trap for a later collision that won't be detected.

This comment has been minimized.

@rootfs

rootfs Apr 8, 2015

Member

the gluster cluster lies outside the kube cluster. what's the collision case?

This comment has been minimized.

@smarterclayton

smarterclayton Apr 8, 2015

Contributor

It's not unreasonable to force someone to create an external headless service if they want this behavior.

----- Original Message -----

@@ -421,6 +425,19 @@ type NFSVolumeSource struct {
ReadOnly bool json:"readOnly,omitempty"
}

+// GlusterfsVolumeSource represents a Glusterfs Mount that lasts the
lifetime of a pod
+type GlusterfsVolumeSource struct {

  • // Required: EndpointsName is the endpoint name that details Glusterfs
    topology
  • EndpointsName string json:"endpoints"

Is the assumption that endpoints for gluster lie outside the kubernetes
cluster? I am a bit anxious about direct creation of endpoints without a
service (now that we have headless services) since it lays a trap for a
later collision that won't be detected.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6174/files#r27942357

This comment has been minimized.

@thockin

thockin Apr 8, 2015

Member

The collision case is that you create endpoints called "foo" then I create
a service called "foo" and the endpoints controller fails.
On Apr 8, 2015 6:24 AM, "Huamin Chen" notifications@github.com wrote:

In pkg/api/types.go
#6174 (comment)
:

@@ -421,6 +425,19 @@ type NFSVolumeSource struct {
ReadOnly bool json:"readOnly,omitempty"
}

+// GlusterfsVolumeSource represents a Glusterfs Mount that lasts the lifetime of a pod
+type GlusterfsVolumeSource struct {

  • // Required: EndpointsName is the endpoint name that details Glusterfs topology
  • EndpointsName string json:"endpoints"

the gluster cluster lies outside the kube cluster. what's the collision
case?


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6174/files#r27968510
.

This comment has been minimized.

@rootfs

rootfs Apr 8, 2015

Member

got it, thanks. would a special namespace for storage help?

This comment has been minimized.

@thockin

thockin Apr 8, 2015

Member

Perhaps the real question is whether you expect an external-to-kubernetes gluster cluster to be namespace-scoped or not?

A) The set of gluster endpoints is namespaced - use a headless service
B) The set of gluster endpoints is not namespaced - use an object reference to a headless service
C ?

This comment has been minimized.

@smarterclayton

smarterclayton Apr 8, 2015

Contributor

On Apr 8, 2015, at 12:11 PM, Tim Hockin notifications@github.com wrote:

In pkg/api/types.go:

@@ -421,6 +425,19 @@ type NFSVolumeSource struct {
ReadOnly bool json:"readOnly,omitempty"
}

+// GlusterfsVolumeSource represents a Glusterfs Mount that lasts the lifetime of a pod
+type GlusterfsVolumeSource struct {

  • // Required: EndpointsName is the endpoint name that details Glusterfs topology
  • EndpointsName string json:"endpoints"
    Perhaps the real question is whether you expect an external-to-kubernetes gluster cluster to be namespace-scoped or not?

A) The set of gluster endpoints is namespaced - use a headless service
B) The set of gluster endpoints is not namespaced - use an object reference to a headless service
C ?

At least for persistent volumes (not namespaced), b) is required.

Only thing I can think of for C is "a DNS address" or "a list of ips".


Reply to this email directly or view it on GitHub.

This comment has been minimized.

@thockin

thockin Apr 8, 2015

Member

We do not currently have any concept of non-namespaced endpoints or services. Are we going to accumulate these things in a random namespace and then violate the cross-namespace principles?

This comment has been minimized.

@smarterclayton

smarterclayton Apr 8, 2015

Contributor

So hypothetically, an admin might run gluster in namespace "foo" and have a real service "gluster" (headless or no). They then want to use volumes from that gluster service in other namespaces. So an admin would automate / manually create persistent volumes that point to that gluster cluster. The volume settings would be "use the gluster cluster in namespace foo with name gluster". When a volume source is created for that persistent volume, it would be referencing that service.

----- Original Message -----

@@ -421,6 +425,19 @@ type NFSVolumeSource struct {
ReadOnly bool json:"readOnly,omitempty"
}

+// GlusterfsVolumeSource represents a Glusterfs Mount that lasts the
lifetime of a pod
+type GlusterfsVolumeSource struct {

  • // Required: EndpointsName is the endpoint name that details Glusterfs
    topology
  • EndpointsName string json:"endpoints"

We do not currently have any concept of non-namespaced endpoints or services.
Are we going to accumulate these things in a random namespace and then
violate the cross-namespace principles?


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6174/files#r28000861

This comment has been minimized.

@thockin

thockin Apr 8, 2015

Member

I can buy that argument for persistent volumes, where it is an admin that is crossing the namespace boundary. It's a bit less shiny when it's a user's pod that is referencing Endpoints or Services in another namespace.

As it stands, the endpoints must be in the same namespace as the pod. I don't think this is sufficient to handle what you are describing. This should probably become an ObjectRef, or else we should make it target a multi-record DNS name and treat that as an endpoints set (or something).

Then we have to decide if it is kosher to write an Endpoints object that does not have an associated Service object.

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