Implement vfs with openstack swift #3708

zengchen1024 · 2017-10-27T10:18:54Z

Add a new implementation of VFS to store the state of cluster in order to support deploying k8s on openstack.
Which issue this PR fixes: #3676

k8s-ci-robot · 2017-10-27T10:18:56Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

k8s-ci-robot · 2017-10-27T10:19:02Z

Hi @zengchen1024. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

justinsb · 2017-10-27T14:59:21Z

@zengchen1024 scanned this and it looks great (are you able to sign the CLA?)

If you want to be able to use swift paths transparently throughout kops (and you probably do!), you'll have to plug it into the VFSContext here: https://github.com/kubernetes/kops/blob/master/util/pkg/vfs/context.go#L95

I don't know if swift has a "default" url prefix, like s3 has s3:// and Google cloud storage has gs:// ? (is it swift://) It doesn't really matter, we just want to use something that is consistent and unambiguous for users, and so if the normal swift CLI tools accept "swift://..." we should use that :-)

zengchen1024 · 2017-10-28T02:49:10Z

@justinsb Thank you very much for your comments! I have added the codes in "context.go". Regarding to the url of "swift://...". as my understanding, the prefix is just used to separate it from other different cloud storages, so I use the name of "swift". Besides, the Openstack is different from other cloud platform, like aws, google etc. There are many Openstack cloud platform, but only one aws/google cloud platform. Because everyone can build his own Openstack cloud platform, but he can not build aws/google.

edisonxiang · 2017-10-28T03:24:37Z

It's better to refer your issue number in your comments.
Which issue this PR fixes: #

zengchen1024 · 2017-10-30T09:16:29Z

/cc @chrislovecnm @dims Could please take a look at this pr. Thanks! @chrislovecnm

justinsb · 2017-10-30T13:37:08Z

util/pkg/vfs/context.go

+		return nil, fmt.Errorf("invalid openstack cloud storage path: %q", p)
+	}
+
+	swiftClient, err := NewSwiftClient()


In future, you might want to cache this, for 2 reasons:

efficiency in reusing the swift client

it means we can configure the swift client specially, either for testing purposes or because we want to pass special options to it

But we can definitely wait until it is needed!

good comments! I understand and will update it!

justinsb · 2017-10-30T13:37:28Z

util/pkg/vfs/swiftfs.go

@@ -0,0 +1,453 @@
+/*
+Copyright 2016 The Kubernetes Authors.


Nit: 2017 :-)

got it. Thanks!

justinsb · 2017-10-30T13:39:54Z

util/pkg/vfs/swiftfs.go

+func (_ OpenstackConfig) filename() (string, error) {
+	name := os.Getenv("OPENSTACK_CREDENTIAL_FILE")
+	if name != "" {
+		return name, nil


As a tip, you might want to (for example) glog.V(2).Infof("using openstack config from credentials found in OPENSTACK_CREDENTIAL_FILE: %s", name) etc

The idea being that if someone reports "I don't know where openstack is getting its configuration from" or "My openstack is not picking up the right credentials" we can ask them to run with --v=2 and get a hint as to why.

But just a suggestion for the future :-)

good suggestion! will update it!

justinsb · 2017-10-30T13:41:03Z

util/pkg/vfs/swiftfs.go

+	if homeDir == "" {
+		return "", fmt.Errorf("can not find home directory")
+	}
+	return filepath.Join(homeDir, ".openstack", "config"), nil


And I would probably glog.V(2) it here also then

will update it!

justinsb · 2017-10-30T13:52:40Z

util/pkg/vfs/swiftfs.go

+	if !strings.HasSuffix(prefix, "/") {
+		prefix += "/"
+	}
+	opt := swiftobject.ListOpts{


So ReadTree is supposed to be recursive (ls -R), and ReadDir is supposed to be non-recursive (ls). Does swift support that? On S3 for example, we set Delimiter for ReadDir, but not for ReadTree.

yes, Swift support it. But it should pass different option to the method of "swiftobject.List". In my test environment, I create several objects as following
bucket: kops
a/a.txt
a/b/b1.txt
a/b/b2.txt
a/b/c/c.txt
if key=a, ReadDir will return a/a.txt; ReadTree will return all of the 4 objects.
if key=a/b, ReadDir will return a/b/b1.txt and a/b/b2.txt; ReadTree will return a/b/b1.txt, a/b/b2.txt and a/b/c/c.txt
if key=a/b/c, both ReadDir and ReadTree will return a/b/c/c.txt

That's absolutely correct. But looking at the two functions (ReadDir & ReadTree) I don't see a difference - am I just missing it?

Yes, there is a very small difference. The variable of 'prefix' is assigned to the different member of 'CreatOpts' in these two method respectively. In ReadDir, CreateOpts.Path = prefix; but in ReadTree, CreateOpts.Prefix= prefix. Sorry to cause you misunderstanding.

Oops - I see it now! Path vs Prefiix. Thank you :-)

justinsb · 2017-10-30T13:56:56Z

Looks great - a few nits / suggestions, but the only real blocker is that ReadDir should behave differently from ReadTree.

I'm afraid I also merged preliminary ACL support, so you need to add an acl ACL argument to your CreateFile function. No need to do anything with it yet though!

Also this is still marked as WIP in the PR title here :-)

zengchen1024 · 2017-10-31T01:51:33Z

@justinsb Thank you very much for review! Your suggestions are very good! First, regarding to the method of 'ReadDir' and 'ReadTree', see my comment. Second, I see you have added a new parameter of 'acl ACL' to method of 'CreateFile', I will try to update my codes. Last, this PR is still marcked as WIP, because the dependency of 'gophercloud' should be updated first, otherwise it will be broken if using 'swiftfs'.

justinsb · 2017-10-31T04:07:22Z

For the dependencies, if you want to try you can try adding this: https://github.com/kubernetes/kops/blob/release/docs/development/dependencies.md

If you have trouble though, just let me know and I can send a PR to add the openstack dependency.

We should probably add the same one as used in k8s 1.8 https://github.com/kubernetes/kubernetes/blob/release-1.8/Godeps/Godeps.json#L1625 , which looks like 2bf16b94fdd9b01557c4d076e567fe5cbbe5a961

zengchen1024 · 2017-10-31T06:48:23Z

@justinsb Yes, I am trying to add a PR to update 'gophercloud' to the newest version. If you have a PR, it is great and could you send me the link of your PR? There is one question why do you update it to the version of '2bf16b94fdd9b01557c4d076e567fe5cbbe5a961' but not the newest version. In my test, It can work with the newest 'gophercloud'. And I need to test to see whether it is right with your special version one.

zengchen1024 · 2017-10-31T07:22:28Z

@justinsb Sorry, I did not understand your comment about updating the dependency of 'gophercloud'. The current version of 'gophercloud' is '2bf16b94fdd9b01557c4d076e567fe5cbbe5a961'. I will add a PR to update it to the newest version. Thanks for your guidance!

zengchen1024 · 2017-10-31T08:06:52Z

/assign @zengchen1024

k8s-github-robot · 2017-11-02T08:06:26Z

@zengchen1024 PR needs rebase

zengchen1024 · 2017-11-02T08:44:24Z

/hold cancel

zengchen1024 · 2017-11-02T11:34:21Z

/cc @chrislovecnm @justinsb , Please, take a look at this PR, thanks!

zengchen1024 · 2017-11-03T06:38:32Z

/ok-to-test

k8s-ci-robot · 2017-11-03T06:38:46Z

@zengchen1024: you can't request testing unless you are a kubernetes member.

In response to this:

/ok-to-test

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.

justinsb · 2017-11-04T04:41:43Z

/ok-to-test

/lgtm

k8s-github-robot · 2017-11-04T04:43:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-04T05:21:39Z

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

justinsb · 2017-11-04T05:52:25Z

/retest

k8s-github-robot · 2017-11-04T06:58:16Z

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

k8s-github-robot · 2017-11-04T07:24:20Z

Automatic merge from submit-queue.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 27, 2017

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 27, 2017

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 27, 2017

justinsb self-assigned this Oct 27, 2017

zengchen1024 force-pushed the openstack_vfs branch from 830fef5 to 4341985 Compare October 28, 2017 02:20

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 28, 2017

zengchen1024 force-pushed the openstack_vfs branch from 4341985 to e0d03fa Compare October 30, 2017 08:34

justinsb reviewed Oct 30, 2017

View reviewed changes

zengchen1024 force-pushed the openstack_vfs branch from e0d03fa to ce6ef6a Compare October 31, 2017 07:49

k8s-ci-robot assigned zengchen1024 Oct 31, 2017

zengchen1024 force-pushed the openstack_vfs branch from ce6ef6a to b110557 Compare October 31, 2017 11:49

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2017

zengchen1024 force-pushed the openstack_vfs branch from b110557 to 3707f87 Compare November 2, 2017 08:32

implement vfs with openstack swift

bbfd1e1

zengchen1024 force-pushed the openstack_vfs branch from 3707f87 to bbfd1e1 Compare November 2, 2017 09:08

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2017

zengchen1024 changed the title ~~[WIP]implement vfs with openstack swift~~ Implement vfs with openstack swift Nov 2, 2017

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 2, 2017

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 4, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2017

k8s-github-robot merged commit 2b3ac16 into kubernetes:master Nov 4, 2017

Implement vfs with openstack swift #3708

Implement vfs with openstack swift #3708

Conversation

zengchen1024 commented Oct 27, 2017 • edited Loading

k8s-ci-robot commented Oct 27, 2017

k8s-ci-robot commented Oct 27, 2017

justinsb commented Oct 27, 2017

zengchen1024 commented Oct 28, 2017

edisonxiang commented Oct 28, 2017

zengchen1024 commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Oct 30, 2017

zengchen1024 commented Oct 31, 2017

justinsb commented Oct 31, 2017

zengchen1024 commented Oct 31, 2017

zengchen1024 commented Oct 31, 2017

zengchen1024 commented Oct 31, 2017

k8s-github-robot commented Nov 2, 2017

zengchen1024 commented Nov 2, 2017

zengchen1024 commented Nov 2, 2017

zengchen1024 commented Nov 3, 2017

k8s-ci-robot commented Nov 3, 2017

justinsb commented Nov 4, 2017

k8s-github-robot commented Nov 4, 2017

k8s-github-robot commented Nov 4, 2017

justinsb commented Nov 4, 2017

k8s-github-robot commented Nov 4, 2017

k8s-github-robot commented Nov 4, 2017

zengchen1024 commented Oct 27, 2017 •

edited

Loading