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 Ceph FS volume plugin #6649

Merged
merged 1 commit into from Sep 2, 2015

Conversation

Projects
None yet
@rootfs
Copy link
Member

rootfs commented Apr 9, 2015

@vishh @thockin

this is functionally ready. I understand Endpoints is still under discussion.

@googlebot googlebot added the cla: yes label Apr 9, 2015

"10.16.154.82:0",
"10.16.154.83:0"
]
}

This comment has been minimized.

@vishh

vishh Apr 9, 2015

Member

nit: Add new line.

@vishh vishh self-assigned this Apr 9, 2015

func (plugin *cephfsPlugin) newBuilderInternal(spec *api.Volume, ep *api.Endpoints, podRef *api.ObjectReference, mounter mount.Interface, exe exec.Interface) (volume.Builder, error) {
id := spec.Cephfs.RadosUser
if id == "" {
id = "admin"

This comment has been minimized.

@vishh

vishh Apr 9, 2015

Member

nit: Can you move all the strings to be const variables at the top of the file?

This comment has been minimized.

@thockin

thockin Apr 21, 2015

Member

Why is this defaulted here and not in the API defaults files? There may be a legit reason, but I want to understand

var mountArgs []string
var opt []string

// cephfs mount option

This comment has been minimized.

@vishh

vishh Apr 9, 2015

Member

Can you post a link to a ceph setup guide? This will help future contributors.

This comment has been minimized.

@rootfs

rootfs Apr 14, 2015

Member

@leseb and I created a containerized ceph here

This comment has been minimized.

@leseb

This comment has been minimized.

@vishh

vishh Apr 17, 2015

Member

Great! Will do!

On Thu, Apr 16, 2015 at 7:45 AM, Leseb notifications@github.com wrote:

In pkg/volume/cephfs/cephfs.go
#6649 (comment)
:

  • if !mountpoint {
  •   if err := os.RemoveAll(dir); err != nil {
    
  •       return err
    
  •   }
    
  • }
  • return nil
    +}

+func (cephfsVolume *cephfs) execMount(mountpoint string) error {

  • var errs error
  • var command exec.Cmd
  • var mountArgs []string
  • var opt []string
  • // cephfs mount option

@vishh https://github.com/vishh please use this container to test Ceph,
https://github.com/ceph/ceph-docker/tree/master/demo


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

- *readOnly*: Whether the filesystem is used as readOnly.


Here are my commands:

This comment has been minimized.

@vishh

vishh Apr 9, 2015

Member

nit: Drop my.

This comment has been minimized.

@rootfs

rootfs Apr 14, 2015

Member

sure :)

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Apr 9, 2015

Thanks for all the awesome work @rootfs. Just a few nits. I would really like to have end to end tests for these plugins. I opened a separate issue to discuss that.

// Optional: RadosUser is the rados user name, default is admin
RadosUser string `json:"user"`
// Optional: SecretFile is the path to key file for user, default is /etc/ceph/user.secret
SecretFile string `json:"secrete_file"`

This comment has been minimized.

@erictune

erictune Apr 14, 2015

Member

s/secrete/secret

RadosUser string `json:"user"`
// Optional: SecretFile is the path to key file for user, default is /etc/ceph/user.secret
SecretFile string `json:"secrete_file"`
// Optional: Secret is the authentication secret for Raods user, default is empty.

This comment has been minimized.

@erictune

erictune Apr 14, 2015

Member

s/Raods/Rados/

This comment has been minimized.

@rootfs

rootfs Apr 14, 2015

Member

nice catch @erictune

// CephfsVolumeSource represents a Cephfs Mount that lasts the lifetime of a pod
type CephfsVolumeSource struct {
// Required: EndpointsName is the endpoint name that represents the Ceph monitors
EndpointsName string `json:"endpoints"`

This comment has been minimized.

@thockin

thockin Apr 21, 2015

Member

This should get the same treatment as the RBD PR, yes?

This comment has been minimized.

@thockin

thockin Jun 1, 2015

Member

still open comment - shouldn't this be a LocalObjectRef ? @pmorie that is what we decided, right?

This comment has been minimized.

@thockin

thockin Jun 1, 2015

Member

Sorry, this should be CephMonitors []string like RBD

// Optional: SecretFile is the path to key file for user, default is /etc/ceph/user.secret
SecretFile string `json:"secret_file"`
// Optional: Secret is the authentication secret for Rados user, default is empty.
Secret string `json:"secret"`

This comment has been minimized.

@thockin

thockin Apr 21, 2015

Member

And this.

This comment has been minimized.

@thockin

thockin Jun 1, 2015

Member

Still open

This comment has been minimized.

@thockin

thockin Jun 1, 2015

Member

This should be LocalObjectRef

id = "admin"
}
secrete_file := spec.Cephfs.SecretFile
if secrete_file == "" {

This comment has been minimized.

@thockin

thockin Apr 21, 2015

Member

s/secrete/secret

// Required: EndpointsName is the endpoint name that represents the Ceph monitors
EndpointsName string `json:"endpoints" description:"ceph monitor endpoints name"`
// Optional: RBDUser is the rados user name, default is admin
// Optional: RadosUser is the rados user name, default is admin

This comment has been minimized.

@thockin

thockin Apr 21, 2015

Member

Duplicate comments

// Optional: RBDUser is the rados user name, default is admin
// Optional: RadosUser is the rados user name, default is admin
RadosUser string `json:"user"`
// Optional: SecretFile is the path to key file for user, default is /etc/ceph/user.secret

This comment has been minimized.

@thockin

thockin Apr 21, 2015

Member

This is a path within the users container? Or sometihng else?

Secret string `json:"secret" description:"secret is authentication secret. If provided, it overrides secret file"`
// Optional: Defaults to false (read/write). ReadOnly here will force
// the Cephfs volume to be mounted with read-only permissions
ReadOnly bool `json:"readOnly,omitempty" description:"Glusterfs volume to be mounted with read-only permissions"`

This comment has been minimized.

@ktdreyer

ktdreyer Apr 23, 2015

"Glusterfs volume" ?

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented May 22, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain @ixdy.

@erictune

This comment has been minimized.

Copy link
Member

erictune commented Jun 1, 2015

ok to test

@erictune erictune added this to the v1.0-post milestone Jun 1, 2015

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Jun 1, 2015

I'm concerned that the 'secrets' issues in here are just not resolved - this needs to resolved ASAP or miss 1.0

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Jun 2, 2015

I thought I've missed the train. I'll shape it up soon.

@rootfs rootfs force-pushed the rootfs:wip-cephfs branch 6 times, most recently from 17a4257 to 2044fbd Jun 2, 2015

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Jun 2, 2015

@vishh @thockin @smarterclayton @markturansky @pmorie

ready for your comments. Thanks

@rootfs rootfs force-pushed the rootfs:wip-cephfs branch 2 times, most recently from 8d1b0c5 to 5df7b37 Jun 2, 2015

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Jun 2, 2015

@rootfs rootfs force-pushed the rootfs:wip-cephfs branch from f7dbe98 to 8cb07c1 Aug 28, 2015

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Aug 28, 2015

GCE e2e build/test passed for commit 8cb07c1.

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented Aug 28, 2015

Thanks @rootfs One last file and then please squash your commits.

@rootfs rootfs force-pushed the rootfs:wip-cephfs branch from 8cb07c1 to 4a4c510 Aug 28, 2015

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Aug 28, 2015

@saad-ali squashed, wait for green :)

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented Aug 28, 2015

Thanks @rootfs

LGTM

@pmorie @markturansky if you're good, let's get this merged.

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Aug 28, 2015

GCE e2e build/test passed for commit 4a4c510.

@markturansky

This comment has been minimized.

Copy link
Member

markturansky commented Aug 28, 2015

@saad-ali I took a look today. I think we're good to go. We can support it from here.

LGTM.

@markturansky

This comment has been minimized.

Copy link
Member

markturansky commented Aug 28, 2015

@rootfs Not worth blocking this PR, but in #12599 Brian commented on your example (fiber channel) and said that YAML is preferred. Perhaps a follow-up PR to change these examples to YAML?

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Aug 28, 2015

@markturansky there are other json to yaml conversion in examples, let's get them in next PR(s)

@markturansky

This comment has been minimized.

Copy link
Member

markturansky commented Aug 28, 2015

SGTM

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented Sep 1, 2015

Adding LGTM label

@saad-ali saad-ali added the lgtm label Sep 1, 2015

@brendandburns

This comment has been minimized.

Copy link
Contributor

brendandburns commented Sep 1, 2015

@rootfs needs rebase (sorry!)

@rootfs rootfs force-pushed the rootfs:wip-cephfs branch 2 times, most recently from dad1124 to 20c284a Sep 1, 2015

implement Ceph FS volume plugin and add to e2e volume test
Signed-off-by: Huamin Chen <hchen@redhat.com>

@rootfs rootfs force-pushed the rootfs:wip-cephfs branch from 20c284a to fe559f2 Sep 1, 2015

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Sep 1, 2015

@brendandburns done, thanks!

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Sep 1, 2015

GCE e2e build/test passed for commit 20c284a.

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Sep 1, 2015

GCE e2e build/test passed for commit dad1124.

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Sep 1, 2015

GCE e2e build/test passed for commit fe559f2.

@k8s-merge-robot k8s-merge-robot added size/XL and removed size/XXL labels Sep 1, 2015

@k8s-merge-robot

This comment has been minimized.

Copy link
Contributor

k8s-merge-robot commented Sep 1, 2015

Labelling this PR as size/XL

@brendandburns

This comment has been minimized.

Copy link
Contributor

brendandburns commented Sep 2, 2015

Merging since the github api flaked and this was skipped.

brendandburns added a commit that referenced this pull request Sep 2, 2015

Merge pull request #6649 from rootfs/wip-cephfs
add Ceph FS volume plugin

@brendandburns brendandburns merged commit 74ef517 into kubernetes:master Sep 2, 2015

4 checks passed

Jenkins GCE e2e 148 tests run, 64 skipped, 0 failed.
Details
Shippable Shippable builds completed. 3798/3818 tests passed.
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment