Update to use latest etcd client library #18635

timothysc · 2015-12-14T02:21:53Z

This is the update for the new client library after all the storage refactoring. There is still an error I'm chasing down in the cache, but everything else appears green.

Final implementation for #11962

@wojtek-t & @jdef for review

/cc @liggitt

k8s-github-robot · 2015-12-14T02:25:10Z

Labelling this PR as size/L

k8s-bot · 2015-12-14T02:56:06Z

GCE e2e test build/test passed for commit 549ae8386443242a2554e0924452ac6c62437a73.

jdef · 2015-12-14T04:37:45Z

contrib/mesos/pkg/scheduler/service/service.go

-		client, err = etcd.NewClientFromFile(etcdConfigFile)
-	} else {
-		client = etcd.NewClient(etcdServerList)
+func newEtcd(etcdServerList []string) (etcd.Client, error) {


looks like etcdConfigFile should be completely removed as a state var (and flag) from SchedulerServer

were you going to update newEtcd in kube-apiserver/app/server.go? also, what are we going to do about etcdConfigFile? that's currently the only way to connect to a secured etcd

re: etcdConfigFile - it's completely removed in the nwo.
/cc @xiang90

@jdef Done.

were you going to update newEtcd in kube-apiserver/app/server.go ?

already done.

so what's the recommended way to connect securely to etcd right now? we should surface that somewhere if we haven't already, I see a good amount of confusion on slack.

jdef · 2015-12-14T04:50:02Z

/cc @sttts since I'm OOO after Monday

wojtek-t · 2015-12-14T08:58:35Z

pkg/storage/cacher_test.go

@@ -287,7 +287,6 @@ func TestFiltering(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Incorrect resourceVersion: %s", fooCreated.ResourceVersion)
 	}
-	initialVersion--


Why did you remove this?

liggitt · 2015-12-14T14:31:50Z

cmd/integration/integration.go

+	}
+	glog.Infof("Creating etcd client pointing to %v", cfg.Endpoints)
+
+	kAPI := etcd.NewKeysAPI(etcdClient)


nit, but I would assume kapi meant "kubernetes api"... maybe a clearer name?

wojtek-t · 2015-12-15T11:01:21Z

@timothysc - I think that this PR should fix the remaining issue in Go 1.5 - see #18701 for more details.

Can we prioritize it to have this merged ASAP?

timothysc · 2015-12-15T15:15:22Z

on it.

wojtek-t · 2015-12-15T15:16:57Z

@timothysc - thanks a lot!

timothysc · 2015-12-15T22:21:21Z

@wojtek-t I think I'll need to ping you tomorrow on the cacher tomorrow, there is some subtlety that I'm missing that is causing an issue there.

wojtek-t · 2015-12-16T08:39:32Z

@timothysc - sure

wojtek-t · 2015-12-16T15:31:43Z

@timothysc - can you please rebase this PR - it will be much simpler for me to patch it and debug the cacher problem

wojtek-t · 2015-12-16T16:00:00Z

@timothysc - cacher_test.go - line 205 (initialVersion--) should be removed

This solves the problem (at least when I'm running it locally).

wojtek-t · 2015-12-18T11:34:01Z

unit tests passed, e2e flake

@k8s-bot test this please

wojtek-t · 2015-12-18T11:38:01Z

infrastructure unit-test failure:

git checkout -f b43af14b26801071c65ee65bab22f5247840dd15
FATAL: Could not checkout b43af14b26801071c65ee65bab22f5247840dd15

@k8s-bot unit test this please

k8s-bot · 2015-12-18T12:04:14Z

GCE e2e test build/test passed for commit c505a5d.

wojtek-t · 2015-12-18T12:07:52Z

The same again:

git checkout -f b43af14b26801071c65ee65bab22f5247840dd15
FATAL: Could not checkout b43af14b26801071c65ee65bab22f5247840dd15

@k8s-bot test this please

k8s-bot · 2015-12-18T12:39:49Z

GCE e2e build/test failed for commit c505a5d.

wojtek-t · 2015-12-18T12:40:32Z

@k8s-bot e2e test this please

k8s-bot · 2015-12-18T13:13:03Z

GCE e2e test build/test passed for commit c505a5d.

wojtek-t · 2015-12-18T13:14:19Z

All tests are passing - merging manually, to unblock effort to update to Go 1.5

Update to use latest etcd client library

timothysc · 2015-12-18T14:19:20Z

Awesome!

ixdy · 2015-12-18T23:20:47Z

I think this PR started causing several etcd unit tests to data race or timeout, as linked in the above issues.

jdef · 2016-01-01T13:33:17Z

pkg/storage/etcd/etcd_helper.go

+	cfg := etcd.Config{
+		Endpoints: c.ServerList,
+		// TODO: Determine if transport needs optimization
+		Transport: &http.Transport{


why not use util.SetTransportDefaults here?

We are overwriting some things (e.g. MaxIdleConnsPerHost) - it was crucial for performance reasons.

You can still do that with SetTransportDefaults… only set MaxIdle… and the defaulting func will set all the other defaults

The defaults are explicitly specified so others will know the values on inspection, vs. hunt and peck.

jdef · 2016-01-12T21:10:05Z

xref #18153

streamnsight · 2016-04-21T05:08:41Z

@timothysc
@jdef
@wojtek-t

I am running into this issue:

#24578

trying to run apiserver with etcd TLS.

TLS worked fine with v1.1.2 I am trying to upgrade from.
I could get into the apiserver container and cat the files apiserver is complaining about not finding, so I don't understand what is wrong.

streamnsight · 2016-04-21T05:36:37Z

same problem with 1.2.1

streamnsight · 2016-04-21T05:49:14Z

in using TLS for the etcd client before I had to have something like

var cfg store.Config

    if caCert != "" && clientCert != "" && clientKey != "" {
        var tlsInfo = transport.TLSInfo{
            CAFile:   caCert,
            CertFile: clientCert,
            KeyFile:  clientKey,
            TrustedCAFile: caCert,
            ClientCertAuth: true,
        }
        t, err := transport.NewTransport(tlsInfo, 10 * time.Second)
        if err != nil {
            fmt.Fprintln(os.Stderr, err)
            os.Exit(1)
        }

        cfg = store.Config{
            ConnectionTimeout: 10 * time.Second,
            TLS: t.TLSClientConfig,
        }
    } else {
        cfg = store.Config{
                ConnectionTimeout: 10 * time.Second,
            }
    }

timothysc · 2016-04-21T15:35:26Z

@streamnsight The fix is here #21535, but it changes the default initialization. I believe the release note denotes as well.

streamnsight · 2016-04-21T17:18:33Z

@timothysc
Sorry Tim but I don't get it:
I don't see anything in the release notes, and I don't understand the 'fix': what I see is a PR but no explanation about how I'm now suppose to configure secured etcd properly.
If i go by the docs it doesn't work as intended.

Could you please explain what I'm supposed to do or point me to the proper documentation.
Thanks

liggitt · 2016-04-21T18:28:08Z

the updates needed for secured etcd were in the v1.2.0 release notes in the "action required" section:

Running against a secured etcd requires these flags to be passed to kube-apiserver (instead of --etcd-config):
    --etcd-certfile, --etcd-keyfile (if using client cert auth)
    --etcd-cafile (if not using system roots)

streamnsight · 2016-04-21T18:32:54Z

@liggitt Thanks for pointing this out.

streamnsight · 2016-04-21T19:34:08Z

FYI the problem I was having was having quotes around the file names in the etcd config.
This is a pretty stupid mistake, but I thought yaml supported both quoted or not quoted string, so just wanted to point it out here.

The only usage of this was removed via kubernetes#18635

googlebot added the cla: yes label Dec 14, 2015

k8s-github-robot assigned dchen1107 Dec 14, 2015

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 14, 2015

jdef reviewed Dec 14, 2015
View reviewed changes

wojtek-t reviewed Dec 14, 2015
View reviewed changes

wojtek-t assigned wojtek-t and unassigned dchen1107 Dec 14, 2015

liggitt reviewed Dec 14, 2015
View reviewed changes

wojtek-t mentioned this pull request Dec 15, 2015

Data race pkg/storage #18701

Closed

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

timothysc force-pushed the etcd_client_post_cleanup branch from 549ae83 to 5074bb1 Compare December 16, 2015 15:36

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

wojtek-t added a commit that referenced this pull request Dec 18, 2015

Merge pull request #18635 from timothysc/etcd_client_post_cleanup

d1e039b

Update to use latest etcd client library

wojtek-t merged commit d1e039b into kubernetes:master Dec 18, 2015

wojtek-t mentioned this pull request Dec 18, 2015

bump ci go version to 1.5.1 #13838

Merged

This was referenced Dec 18, 2015

data race in k8s.io/kubernetes/pkg/storage/etcd #18913

Closed

TestWatchEtcdError fairly consistently timing out #18914

Closed

jdef reviewed Jan 1, 2016
View reviewed changes

This was referenced Jan 12, 2016

Eliminate dependency on tools.EtcdClient from contrib/mesos/pkg #18153

Closed

Get rid of tools.etcdClient in the scheduler d2iq-archive/kubernetes-mesos#667

Closed

jonboulle mentioned this pull request Jan 15, 2016

Remove unused coreos/go-etcd fork from third_party #19706

Merged

jonboulle pushed a commit to jonboulle/kubernetes that referenced this pull request May 31, 2016

Remove unused coreos/go-etcd fork from third_party

4c437aa

The only usage of this was removed via kubernetes#18635

Update to use latest etcd client library #18635

Update to use latest etcd client library #18635

Conversation

timothysc commented Dec 14, 2015

k8s-github-robot commented Dec 14, 2015

k8s-bot commented Dec 14, 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

jdef commented Dec 14, 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

wojtek-t commented Dec 15, 2015

timothysc commented Dec 15, 2015

wojtek-t commented Dec 15, 2015

timothysc commented Dec 15, 2015

wojtek-t commented Dec 16, 2015

wojtek-t commented Dec 16, 2015

wojtek-t commented Dec 16, 2015

wojtek-t commented Dec 18, 2015

wojtek-t commented Dec 18, 2015

k8s-bot commented Dec 18, 2015

wojtek-t commented Dec 18, 2015

k8s-bot commented Dec 18, 2015

wojtek-t commented Dec 18, 2015

k8s-bot commented Dec 18, 2015

wojtek-t commented Dec 18, 2015

timothysc commented Dec 18, 2015

ixdy commented Dec 18, 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

jdef commented Jan 12, 2016

streamnsight commented Apr 21, 2016

streamnsight commented Apr 21, 2016

streamnsight commented Apr 21, 2016 • edited Loading

timothysc commented Apr 21, 2016

streamnsight commented Apr 21, 2016

liggitt commented Apr 21, 2016

streamnsight commented Apr 21, 2016

streamnsight commented Apr 21, 2016

streamnsight commented Apr 21, 2016 •

edited

Loading