Adds ability to define a prefix for etcd paths #5707

kbeecher · 2015-03-20T13:43:13Z

The API server can be supplied (via a command line flag) with a custom
prefix that is prepended to etcd resources paths.

Refs: #3476

bgrant0607 · 2015-03-20T23:02:43Z

Looks reasonable at a glance. Thanks for adding the test.

@derekwaynecarr, were you interested in reviewing?

mbforbes · 2015-03-20T23:06:07Z

Optimistically assuming "yes" from @derekwaynecarr and assigning; please re-assign if desired, as always.

kbeecher · 2015-03-24T17:27:18Z

I don't know what the problem is with the failed build. I rebased my branch on the upstream master because it became out of date. It builds and passes tests locally. I can't get details by following the "Details" link.

Can anyone help?

derekwaynecarr · 2015-03-24T20:22:15Z

I can review

derekwaynecarr · 2015-03-24T20:22:28Z

The Shippable failure is due to shippable not being able to do a docker pull to even run the test.

bgrant0607 · 2015-03-24T20:24:20Z

Restarted the Shippable build.

derekwaynecarr · 2015-03-24T20:35:40Z

pkg/tools/etcd_helper.go

@@ -354,6 +374,17 @@ func (h *EtcdHelper) AtomicUpdate(key string, ptrToType runtime.Object, ignoreNo
 	}
 }

+// Examines the etcd key and prepends the prefix if it isn't already there.
+func (h *EtcdHelper) prefixEtcdKey(key string) string {


I am a little confused by this.

Do we pass around fully-qualified keys across methods, or are we always passing around the relative key path?

If I set my prefix to be "/registry/pods", then I would expect every key path in my repository to have that prefix prepended. Instead, this behavior results in saying that my pods will go in "/registry/pods/default", but my services will go in "/registry/pods/registry/services/..." which basically breaks usage of a namespace called "registry" in this example.

In general, I think we should always prefix the key so in my example:

pods: /registry/pods/registry/pods/default/...
services: /registry/pods/registry/services/default/...

Right now, you are basically making the default prefix = "/registry" when you set it as "" as a result.

Good question.

What breaks if this just does the join in the case that the PathPrefix is non-empty?

The use case I imagine is to prefix registry entries with something like "/kubernetes.io/clustername", so pods would be under /kubernetes.io/clustername/registry/pods.

derekwaynecarr · 2015-03-24T20:36:53Z

This LGTM aside from the comment on the has prefix check for an edge case. I think if we provide a prefix, it should prefix everything, even if it matches our default internal prefixing.

kbeecher · 2015-03-25T17:57:54Z

Patch has been updated as per request. The prefix is now added to all keys unconditionally.

derekwaynecarr · 2015-03-26T15:20:42Z

pkg/tools/etcd_helper.go

@@ -155,11 +164,14 @@ func (h *EtcdHelper) ExtractToList(key string, listObj runtime.Object) error {
 // a zero object of the requested type, or an error, depending on ignoreNotFound. Treats
 // empty responses and nil response nodes exactly like a not found error.
 func (h *EtcdHelper) ExtractObj(key string, objPtr runtime.Object, ignoreNotFound bool) error {
+	key = h.prefixEtcdKey(key)


So here we prefix the key, and then we pass the prefixed key to h.bodyAndExtractObj and we pass the already prefixed key. The first line in bodyAndExtractObj is to prefix the key again. Doesn't this result in a key with the prefix twice?

This is why I asked if we had a convention in place for passing prefixed or non-prefixed keys across each function so readers can make sense of it.

We need a test case that works against ExtractObj to validate the behavior here.

derekwaynecarr · 2015-03-26T15:30:07Z

I would be more comfortable with this PR if the majority of the test cases set a prefix so I could know we are or are not catching multiple prefixing of a key. Right now, it looks like only one test applies a key, and it does not cover all code paths.

bgrant0607 · 2015-03-26T20:47:18Z

The testing issue is a good point. One way to ensure that would be to absorb "/registry" into the prefix, and make that the default prefix. Obviously that's a much more invasive change, however. A grep for "/registry" has how many hits OOC?

kbeecher · 2015-03-30T12:08:55Z

OK, the tweaks Ill make:

Add more tests to cover other operations in the EtcdHelper
Absorbing "/registry" into the prefix and making that the default should be doable. At last count there were umpteen hard-coded instances of "registry", so not much work.

kbeecher · 2015-03-31T12:03:18Z

I think there's actually more work here than I thought, and it's complicated by the use of the FakeEtcdClient in the test code.

When the test code uses an etcd.REST type to manage keys, then the EtcdHelper gets used (the EtcdHelper is what's responsible for adding the path prefix).

However, when the test code uses the FakeEtcdClient to manage keys, then the EtcdHelper is not used and the path prefix is not added.

The problem is that usage of them is mixed up in the test code.

As an example, the function TestEtcdCreateControllerAlreadyExisting:

It first creates an entry using the FakeEtcdClient (/registry/controllers/default/foo),
It then creates another entry using the REST type (/myprefix/registry/controllers/default/foo).
The test then fails because the first creation didn't add the prefix properly.

I don't fully understand why the FakeEtcdClient is used. I would also say that if there is similar code in production (i.e. code that manages Etcd keys in a way that doesn't use the EtcdHelper, like the FakeEtcdClient does), then this would also put keys into etcd without the prefix.

Does it therefore make sense to change the behaviour of the FakeEtcdClient?

bgrant0607 · 2015-03-31T14:35:33Z

How hard would it be to use EtcdHelper in the tests, as is done here?
https://github.com/GoogleCloudPlatform/kubernetes/blob/97560c409f9a39baa19517efb77d6c67832aa413/pkg/registry/namespace/etcd/etcd_test.go#L35

kbeecher · 2015-04-02T14:31:52Z

I'm not sure. Are you suggesting replacing the use of FakeEtcdClient with EtcdHelper?

bgrant0607 · 2015-04-03T18:12:18Z

It needs to be possible to configure mock responses, such as in this example:
https://github.com/GoogleCloudPlatform/kubernetes/blob/97560c409f9a39baa19517efb77d6c67832aa413/pkg/registry/namespace/etcd/etcd_test.go#L279

I haven't looked at it closely enough. Maybe @lavalamp has an idea.

Fundamentally, we need something that munges all etcd keys everywhere.

lavalamp · 2015-04-03T18:48:54Z

pkg/master/master_test.go

@@ -30,7 +30,7 @@ func TestGetServersToValidate(t *testing.T) {
 	config := Config{}
 	fakeClient := tools.NewFakeEtcdClient(t)
 	fakeClient.Machines = []string{"http://machine1:4001", "http://machine2", "http://machine3:4003"}
-	config.EtcdHelper = tools.EtcdHelper{fakeClient, latest.Codec, nil}
+	config.EtcdHelper = tools.EtcdHelper{fakeClient, latest.Codec, nil, ""}


IMO it's a must to parameterize our tests for this change, like we did with apiversion. That final "" needs to come from a command line flag, and the test-go.sh file should run it with at least "" and "prefix" values.

This also happens to be a pretty easy thing to do to resolve the testing concerns, fortunately.

please use testetcd.Prefix()

Use testetcd.PathPrefix() instead of ""; also s/tools.PathPrefix/testetcd.PathPrefix/. Thanks.

I think you missed this comment?

etcdtest.PathPrefix()

lavalamp · 2015-04-03T18:51:41Z

cmd/integration also needs the parameterization. Ideally you'd add a tools/etcd/etcdtest package which defines the flag.

lavalamp · 2015-04-16T05:00:43Z

pkg/registry/generic/etcd/etcd_test.go

 	for name, item := range table {
 		fakeClient, registry := NewTestGenericEtcdRegistry(t)
 		registry.UpdateStrategy.(*testRESTStrategy).allowCreateOnUpdate = item.allowCreate
+		path, _ := registry.KeyFunc(ctx, "foo")
+		path = etcdtest.AddPrefix(path)


Same comment-- please fix the other locations too.

lavalamp · 2015-04-16T05:10:17Z

Unfortunately I can't finish this review right now-- hopefully this will keep you busy. I definitely feel like we're converging, these requests are all much more minor. Thanks!

(PS you're going to have to rebase...)

kbeecher · 2015-04-16T16:07:20Z

Thanks @lavalamp, good to know we're converging.

lavalamp · 2015-04-22T21:54:12Z

test/integration/etcd_tools_test.go

@@ -92,7 +92,7 @@ func TestExtractObj(t *testing.T) {

 func TestWatch(t *testing.T) {
 	client := newEtcdClient()
-	helper := tools.NewEtcdHelper(client, testapi.Codec())
+	helper := tools.NewEtcdHelper(client, testapi.Codec(), "")


etcdtest.PathPrefix()

lavalamp · 2015-04-22T22:15:10Z

I made another pass. Please note that there are two older comments that I think you haven't noticed.

I think this should be the last requests I have. Please fix & rebase and let's get this merged!

kbeecher · 2015-04-23T12:57:49Z

@lavalamp Thanks. I think I've found the two issues you referred and fixed them.

kbeecher · 2015-04-23T21:36:54Z

Struggling with a failing integration test at the moment. Having difficulty decoding the error trace - I think it's around the etcd_helper_watch but not sure. Any help would be appreciated.

lavalamp · 2015-04-23T23:28:31Z

hm-- the stuff about not making the kubernetes service is noise-- ignore it.

goroutine 1101 [chan receive, 1 minutes]:
github.com/GoogleCloudPlatform/kubernetes/test/integration.func·026(0xc20aa0d3a0, 0x19)
    /home/travis/gopath/src/github.com/GoogleCloudPlatform/kubernetes/_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/test/integration/etcd_tools_test.go:110 +0x4b7
github.com/GoogleCloudPlatform/kubernetes/test/integration.withEtcdKey(0xc20b295ea0)
    /home/travis/gopath/src/github.com/GoogleCloudPlatform/kubernetes/_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/test/integration/utils.go:52 +0x148
github.com/GoogleCloudPlatform/kubernetes/test/integration.TestWatch(0xc20b3ab050)
    /home/travis/gopath/src/github.com/GoogleCloudPlatform/kubernetes/_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/test/integration/etcd_tools_test.go:145 +0x282
testing.tRunner(0xc20b3ab050, 0x1b524c0)
    /usr/local/go/src/pkg/testing/testing.go:422 +0x110
created by testing.RunTests
    /usr/local/go/src/pkg/testing/testing.go:504 +0xb46

looks like it might be the relevant part of the stacktrace?

The API server can be supplied (via a command line flag) with a custom prefix that is prepended to etcd resources paths. Refs: kubernetes#3476

kbeecher · 2015-04-27T11:21:28Z

@lavalamp Yes, I fixed the problem you found. However, the integration test fails on the build machine for another reason: https://travis-ci.org/GoogleCloudPlatform/kubernetes/jobs/59856346

The problem is, I can't reproduce it. All the tests on my local machine pass.

lavalamp · 2015-04-27T20:37:39Z

LGTM. Travis is a known flake. Thanks for the change!

Adds ability to define a prefix for etcd paths

nikhiljindal · 2015-05-12T00:12:00Z

hack/test-go.sh

 for apiVersion in "${apiVersions[@]}"; do
  echo "Running tests for APIVersion: $apiVersion"
-  KUBE_API_VERSION="${apiVersion}" runTests "$@"
+  KUBE_API_VERSION="${apiVersion}" ETCD_PREFIX=${ETCD_STANDARD_PREFIX} runTests "$@"


Locally, unit tests are now being run 3 times!
For v1beta1, beta3 and etcdPrefix.
@bgrant0607 @lavalamp
Is this intended?

Maybe we can run with v1beta1 and standard prefix once and then v1beta3 and custom prefix (or vice-versa)?
Or if the test passes with a custom prefix, then it means that it will pass with the standard prefix as well?

Yes, this was intentional-- I don't mind if you combine the runs like you suggest, but we do need to test at least once with a custom prefix.

Its actually running 4 times on shippable and travis :)
Twice with go 1.3 (v1beta1 and then custom etcdPrefix)
and twice with go 1.4 (v1beta3 and custom etcdPrefix)
Sending a PR to fix this.

Being fixed in #8134

Thanks. I'd eventually like to get back to no extra test runs for this.

bgrant0607 · 2015-05-12T00:14:23Z

Yes, we already have enough problems with test bandwidth.

googlebot added the cla: yes label Mar 20, 2015

mbforbes assigned derekwaynecarr Mar 20, 2015

bgrant0607 assigned bgrant0607 and derekwaynecarr and unassigned derekwaynecarr and bgrant0607 Mar 20, 2015

kbeecher force-pushed the kbeecher/etcd_prefix branch 2 times, most recently from fb12eb3 to 616dacc Compare March 24, 2015 16:13

derekwaynecarr reviewed Mar 24, 2015
View reviewed changes

kbeecher force-pushed the kbeecher/etcd_prefix branch 2 times, most recently from 7d4ffa1 to 0c3f961 Compare March 25, 2015 17:56

derekwaynecarr reviewed Mar 26, 2015
View reviewed changes

lavalamp reviewed Apr 3, 2015
View reviewed changes

kbeecher force-pushed the kbeecher/etcd_prefix branch from 0c3f961 to 8ce9ab5 Compare April 8, 2015 15:34

lavalamp reviewed Apr 16, 2015
View reviewed changes

kbeecher force-pushed the kbeecher/etcd_prefix branch from bbee135 to 05fb3cf Compare April 16, 2015 16:02

kbeecher force-pushed the kbeecher/etcd_prefix branch from 05fb3cf to a360d31 Compare April 20, 2015 10:34

lavalamp reviewed Apr 22, 2015
View reviewed changes

kbeecher force-pushed the kbeecher/etcd_prefix branch 2 times, most recently from 9f32605 to 1ae79bb Compare April 23, 2015 12:56

kbeecher force-pushed the kbeecher/etcd_prefix branch from 1ae79bb to b27bcf5 Compare April 23, 2015 19:25

kbeecher force-pushed the kbeecher/etcd_prefix branch from b27bcf5 to e2edb4f Compare April 24, 2015 09:29

Adds ability to define a prefix for etcd paths

a7623ca

The API server can be supplied (via a command line flag) with a custom prefix that is prepended to etcd resources paths. Refs: kubernetes#3476

kbeecher force-pushed the kbeecher/etcd_prefix branch from e2edb4f to a7623ca Compare April 24, 2015 10:13

lavalamp added a commit that referenced this pull request Apr 27, 2015

Merge pull request #5707 from endocode/kbeecher/etcd_prefix

83093af

Adds ability to define a prefix for etcd paths

lavalamp merged commit 83093af into kubernetes:master Apr 27, 2015

kbeecher deleted the kbeecher/etcd_prefix branch April 29, 2015 12:18

nikhiljindal reviewed May 12, 2015
View reviewed changes

nikhiljindal mentioned this pull request May 12, 2015

Fixing the number of times our unit tests are run #8134

Merged

kbeecher unassigned lavalamp Aug 12, 2015

Navigation Menu

Adds ability to define a prefix for etcd paths #5707

Adds ability to define a prefix for etcd paths #5707

Conversation

kbeecher commented Mar 20, 2015

bgrant0607 commented Mar 20, 2015

mbforbes commented Mar 20, 2015

kbeecher commented Mar 24, 2015

derekwaynecarr commented Mar 24, 2015

derekwaynecarr commented Mar 24, 2015

bgrant0607 commented Mar 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Mar 24, 2015

kbeecher commented Mar 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Mar 26, 2015

bgrant0607 commented Mar 26, 2015

kbeecher commented Mar 30, 2015

kbeecher commented Mar 31, 2015

bgrant0607 commented Mar 31, 2015

kbeecher commented Apr 2, 2015

bgrant0607 commented Apr 3, 2015

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

lavalamp commented Apr 3, 2015

Choose a reason for hiding this comment

lavalamp commented Apr 16, 2015

kbeecher commented Apr 16, 2015

Choose a reason for hiding this comment

lavalamp commented Apr 22, 2015

kbeecher commented Apr 23, 2015

kbeecher commented Apr 23, 2015

lavalamp commented Apr 23, 2015

kbeecher commented Apr 27, 2015

lavalamp commented Apr 27, 2015

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

bgrant0607 commented May 12, 2015