etcd3: use PrevKV to remove additional get #34246

hongchaodeng · 2016-10-06T16:29:36Z

We are trying to test using PrevKV feature and see if it improves performance.
In order to test this, we will need etcd v3.1 (alpha) image.

Blockers:

update gcr.io image (version v3.0.12)

etcd3: use PrevKV to remove additional get

This change is

hongchaodeng · 2016-10-06T16:31:59Z

cc @wojtek-t @timothysc

timothysc · 2016-10-06T16:36:47Z

Godeps/Godeps.json

@@ -353,263 +353,273 @@
 		},
 		{
 			"ImportPath": "github.com/coreos/etcd/alarm",
-			"Comment": "v3.0.10",
-			"Rev": "546c0f7ed65523c24390f0f26c7e4af2232f52d2"
+			"Comment": "v3.1.0-alpha.1-86-gb980ab0",


Won't @wojtek-t need to bump their uber-container to test this?

xiang90 · 2016-10-06T16:41:55Z

Do not merge this. If this solves the problem, we will back port this feature to 3.0 release branch next week.

timothysc · 2016-10-06T16:59:24Z

pkg/storage/etcd3/watcher.go

+		}
+		var prevVal []byte
+		if len(prevResp.Kvs) > 0 {
+			prevVal = prevResp.Kvs[0].Value


just for edification but if you said clientv3.WithRev(0) you would get the history of a key?

"0" will returns latest state.

timothysc · 2016-10-06T19:30:29Z

/cc @smarterclayton fyi.

wojtek-t · 2016-10-06T19:33:15Z

@xiang90 @hongchaodeng - just to clarify, what I should do is to:

build etcd Docker image for 3.1 release
patch this PR
build from this patch and check the results
Or am I missing something?

hongchaodeng · 2016-10-06T19:35:20Z

Yes. Plan matched.

xiang90 · 2016-10-06T19:42:31Z

build from this patch and check the results

If the result is good, we will backport this feature back to etcd 3.0.

wojtek-t · 2016-10-07T11:05:00Z

@hongchaodeng @xiang90 - I did an experiment with this PR patched (and etcd in 3.1.0-alpha.1 version - I build the container: "gcr.io/google_containers/etcd:3.1.0-alpha.1" for this purpose)

And the results are waaaaaaaay better - please take a look into this comment:
#33653 (comment)

So if you could backport this feature to 3.0.x next week it would be great !!!

wojtek-t · 2016-10-07T11:07:51Z

I'm reassigning it back to you. Once you build the new 3.0.x release, we should probably update this PR and then merge it, right?

hongchaodeng · 2016-10-07T18:10:11Z

@wojtek-t
Sure. I will update the PR once the new release with the backport comes.

k8s-ci-robot · 2016-10-07T20:52:20Z

Jenkins GKE smoke e2e failed for commit cfd8685a12e06e56e26b294dabffb469edc40eaa. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-07T20:53:20Z

Jenkins GCI GKE smoke e2e failed for commit cfd8685a12e06e56e26b294dabffb469edc40eaa. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

hongchaodeng · 2016-10-11T03:33:56Z

Running into this issue many times:

error running up: exit status 1

hongchaodeng · 2016-10-11T05:09:39Z

@k8s-bot gce e2e test this

k8s-ci-robot · 2016-10-11T05:59:06Z

Jenkins GCE e2e failed for commit 2516ab0. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

thockin · 2016-10-11T06:04:07Z

regarding godep: did you follow the EXACT instructions in /~https://github.com/kubernetes/kubernetes/blob/master/docs/devel/godep.md ?

If so, please help me with instructions for the smallest repro I can make...

hongchaodeng · 2016-10-11T06:18:18Z

@thockin
Thanks for offering the help.
We have fixed the godep problem actually :)

wojtek-t · 2016-10-11T06:20:42Z

@k8s-bot gce e2e test this, issue: #IGNORE (Failed to create firewall)

k8s-github-robot · 2016-10-11T07:33:43Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-10-11T08:16:58Z

Automatic merge from submit-queue

timothysc · 2016-10-11T16:14:21Z

@smarterclayton - this patch is high-priority.

Automatic merge from submit-queue Avoid unnecessary decoding in etcd3 client Ref #33653 With the "Cacher" layer in Kubernetes, most of the watches processed by "pkg/storage/etcd3/watcher.go" have "filter = Everything()". That said, we generally don't need to decode previous value of the object (which is used only to get the value of filter of it), because we already know it will be true. This PR is basically fixing this problem. Should be merged after #34246

k8s-cherrypick-bot · 2016-10-19T08:52:57Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@hongchaodeng

#31704-#32831-#32907-#33003-#33349-#31190-#33581-#34089-#34234-#32822-#33393-#34246-#34435-#32477-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of #31189 #31704 #32831 #32907 #33003 #33349 #31190 #33581 #34089 #34234 #32822 #33393 #34246 #34435 #32477 upstream release 1.4 We are going to release etcd v3 in 1.4.x release. ``` Cherrypick etcd v3-related bug fixes to 1.4 branch ``` Those PRs include: - Updates of etcd Godeps - Update to pkg/storage/etcd directory - Two PR that were unnecessary to avoid conflicts: #31189 #31704 Automated cherry pick of #31189 #31704 #32831 #32907 #33003 #33349 #31190 #33581 #34089 #34234 #32822 #33393 #34246 #34435 #32477 @hongchaodeng @xiang90 @lavalamp @smarterclayton

k8s-cherrypick-bot · 2016-10-21T05:14:13Z

Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@hongchaodeng

…ck-of-#31189-kubernetes#31704-kubernetes#32831-kubernetes#32907-kubernetes#33003-kubernetes#33349-kubernetes#31190-kubernetes#33581-kubernetes#34089-kubernetes#34234-kubernetes#32822-kubernetes#33393-kubernetes#34246-kubernetes#34435-kubernetes#32477-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of kubernetes#31189 kubernetes#31704 kubernetes#32831 kubernetes#32907 kubernetes#33003 kubernetes#33349 kubernetes#31190 kubernetes#33581 kubernetes#34089 kubernetes#34234 kubernetes#32822 kubernetes#33393 kubernetes#34246 kubernetes#34435 kubernetes#32477 upstream release 1.4 We are going to release etcd v3 in 1.4.x release. ``` Cherrypick etcd v3-related bug fixes to 1.4 branch ``` Those PRs include: - Updates of etcd Godeps - Update to pkg/storage/etcd directory - Two PR that were unnecessary to avoid conflicts: kubernetes#31189 kubernetes#31704 Automated cherry pick of kubernetes#31189 kubernetes#31704 kubernetes#32831 kubernetes#32907 kubernetes#33003 kubernetes#33349 kubernetes#31190 kubernetes#33581 kubernetes#34089 kubernetes#34234 kubernetes#32822 kubernetes#33393 kubernetes#34246 kubernetes#34435 kubernetes#32477 @hongchaodeng @xiang90 @lavalamp @smarterclayton

googlebot added the cla: yes label Oct 6, 2016

k8s-github-robot assigned dchen1107 Oct 6, 2016

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Oct 6, 2016

timothysc reviewed Oct 6, 2016

View reviewed changes

xiang90 changed the title ~~etcd3: use PrevKV to remove additional get~~ Do Not Merge etcd3: use PrevKV to remove additional get Oct 6, 2016

timothysc reviewed Oct 6, 2016

View reviewed changes

hongchaodeng changed the title ~~Do Not Merge etcd3: use PrevKV to remove additional get~~ [Do Not Merge] etcd3: use PrevKV to remove additional get Oct 6, 2016

wojtek-t assigned wojtek-t and unassigned dchen1107 Oct 6, 2016

wojtek-t mentioned this pull request Oct 7, 2016

etcd3 watcher doesn't scale #33653

Closed

wojtek-t assigned hongchaodeng and unassigned wojtek-t Oct 7, 2016

hongchaodeng force-pushed the etcddep branch from f187e11 to cfd8685 Compare October 7, 2016 20:47

timothysc added the area/etcd label Oct 7, 2016

timothysc added priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 7, 2016

k8s-github-robot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 7, 2016

wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 10, 2016

wojtek-t mentioned this pull request Oct 10, 2016

Update etcd Godeps to 3.0.12 #34440

Closed

k8s-github-robot merged commit 0b62733 into kubernetes:master Oct 11, 2016

hongchaodeng deleted the etcddep branch October 11, 2016 14:38

wojtek-t added the cherrypick-candidate label Oct 19, 2016

k8s-cherrypick-bot removed the cherrypick-candidate label Oct 19, 2016

wojtek-t added this to the v1.4 milestone Oct 19, 2016

wojtek-t added the cherrypick-candidate label Oct 19, 2016

wojtek-t mentioned this pull request Oct 20, 2016

Automated cherry pick of #31189 #31704 #32831 #32907 #33003 #33349 #31190 #33581 #34089 #34234 #32822 #33393 #34246 #34435 #32477 upstream release 1.4 #35205

Merged

k8s-cherrypick-bot removed the cherrypick-candidate label Oct 21, 2016

liggitt mentioned this pull request Nov 10, 2016

Fix unidling controller with etcd3 openshift/origin#11847

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcd3: use PrevKV to remove additional get #34246

etcd3: use PrevKV to remove additional get #34246

hongchaodeng commented Oct 6, 2016 •

edited by jessfraz

Loading

hongchaodeng commented Oct 6, 2016

timothysc Oct 6, 2016

xiang90 commented Oct 6, 2016

timothysc Oct 6, 2016

hongchaodeng Oct 6, 2016

timothysc commented Oct 6, 2016

wojtek-t commented Oct 6, 2016

hongchaodeng commented Oct 6, 2016

xiang90 commented Oct 6, 2016

wojtek-t commented Oct 7, 2016

wojtek-t commented Oct 7, 2016

hongchaodeng commented Oct 7, 2016

k8s-ci-robot commented Oct 7, 2016

k8s-ci-robot commented Oct 7, 2016

hongchaodeng commented Oct 11, 2016

hongchaodeng commented Oct 11, 2016

k8s-ci-robot commented Oct 11, 2016

thockin commented Oct 11, 2016

hongchaodeng commented Oct 11, 2016 •

edited

Loading

wojtek-t commented Oct 11, 2016

k8s-github-robot commented Oct 11, 2016

k8s-github-robot commented Oct 11, 2016

timothysc commented Oct 11, 2016

k8s-cherrypick-bot commented Oct 19, 2016

k8s-cherrypick-bot commented Oct 21, 2016

etcd3: use PrevKV to remove additional get #34246

etcd3: use PrevKV to remove additional get #34246

Conversation

hongchaodeng commented Oct 6, 2016 • edited by jessfraz Loading

hongchaodeng commented Oct 6, 2016

timothysc Oct 6, 2016

Choose a reason for hiding this comment

xiang90 commented Oct 6, 2016

timothysc Oct 6, 2016

Choose a reason for hiding this comment

hongchaodeng Oct 6, 2016

Choose a reason for hiding this comment

timothysc commented Oct 6, 2016

wojtek-t commented Oct 6, 2016

hongchaodeng commented Oct 6, 2016

xiang90 commented Oct 6, 2016

wojtek-t commented Oct 7, 2016

wojtek-t commented Oct 7, 2016

hongchaodeng commented Oct 7, 2016

k8s-ci-robot commented Oct 7, 2016

k8s-ci-robot commented Oct 7, 2016

hongchaodeng commented Oct 11, 2016

hongchaodeng commented Oct 11, 2016

k8s-ci-robot commented Oct 11, 2016

thockin commented Oct 11, 2016

hongchaodeng commented Oct 11, 2016 • edited Loading

wojtek-t commented Oct 11, 2016

k8s-github-robot commented Oct 11, 2016

k8s-github-robot commented Oct 11, 2016

timothysc commented Oct 11, 2016

k8s-cherrypick-bot commented Oct 19, 2016

k8s-cherrypick-bot commented Oct 21, 2016

hongchaodeng commented Oct 6, 2016 •

edited by jessfraz

Loading

hongchaodeng commented Oct 11, 2016 •

edited

Loading