Update etcd client to 3.3 for 1.13 #69322
Conversation
jpbetz
added
area/etcd
sig/api-machinery
k8s-ci-robot
added
the
release-note
|
@jpbetz: GitHub didn't allow me to request PR reviews from the following users: wenjiaswe, jingyih, gyuho. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
k8s-ci-robot
added
cncf-cla: yes
jpbetz
force-pushed
the
etcd-client-3.3.9
branch
from
cb296ea
to
4d7f3e4
Compare
|
/retest |
k8s-ci-robot
added
size/XXL
|
Sorting out some gogo/protobuf + generated code issues, hang tight. |
jpbetz
force-pushed
the
etcd-client-3.3.9
branch
from
6193681
to
cb63016
Compare
k8s-ci-robot
added
area/code-generation
sig/cli
|
etcd part LGTM. Just for reference, this does not change clientv3 behavior since we backported health check feature to https://github.com/etcd-io/etcd/blob/master/CHANGELOG-3.2.md#v3210-2017-11-16 client. In addition, this replaces |
jpbetz
force-pushed
the
etcd-client-3.3.9
branch
4 times, most recently
from
1436e18
to
f9354f8
Compare
|
This is ready for review. |
k8s-ci-robot
added
the
sig/cloud-provider
|
/test pull-kubernetes-e2e-kops-aws |
|
I looked at this, that's an enormous vendoring change. Is there any way to confine it to etcd only? Changing every protobuf file in the repo is making me very nervous. |
I was careful to limit the transitive dependency bumps to ones that are actually required by the etcd client. Without the gogo/protobuf bump, we had a incompatibility in the etcd client:
Yeah, I wasn't thrilled to find that we needed to regen our protobuf files. But we are lagging behind on gogo/protobuf and this bumps us up from And we're doing this at the very beginning of the 1.13 cycle, which gives us ample time to soak these changes. |
|
/test pull-kubernetes-local-e2e-containerized |
|
@jpbetz: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
There was a problem hiding this comment.
Generally LGTM, but I would split out your v2 compat changes into it's own PR, unless this update somehow forces the issue.
@lavalamp - This is normal. Every time we levelset protobuf changes we need to regenerate * which cascades through machinery. This has happened a number of times.
| @@ -314,12 +314,33 @@ func toTTLOptions(r *pb.Request) store.TTLOptionSet { | |||
| } | |||
|
|
|||
| func applyRequest(r *pb.Request, applyV2 etcdserver.ApplierV2) { | |||
| // TODO: find a sane way to perform this cast or avoid it in the first place | |||
| reqV2 := &etcdserver.RequestV2{ | |||
There was a problem hiding this comment.
This seems orthogonal to the main purpose of this PR which is level up the deps to 3.3
There was a problem hiding this comment.
the etcd2-specific tools are being removed in #69577, fyi
edit: actually, I think we need to keep this through 1.13, and will drop it in 1.14
There was a problem hiding this comment.
Unfortunately, This change is required by the version bump, due to a change in the etcd codebase. Look forward to stripping out the etcd2 code in 1.14
There was a problem hiding this comment.
Generally LGTM, but I would split out your v2 compat changes into it's own PR, unless this update somehow forces the issue.
^ seems to apply then.
| @@ -820,24 +819,6 @@ func (m *DeviceSpec) MarshalTo(dAtA []byte) (int, error) { | |||
| return i, nil | |||
| } | |||
|
|
|||
| func encodeFixed64Api(dAtA []byte, offset int, v uint64) int { | |||
There was a problem hiding this comment.
do we have any tests that ensure previously serialized data decodes correctly and changes like this stay wire-compatible?
There was a problem hiding this comment.
Yes, we have https://k8s-testgrid.appspot.com/sig-release-master-upgrade which I believe are all post-commit. We can watch that carefully after committing.
There was a problem hiding this comment.
that's good in one direction, but not exhaustive for all fields (we have a few oddities in our proto generation around nullable type aliases), and doesn't test in the opposite direction (new serialized data is readable by old servers). I don't see anything concerning in this PR, but it's pretty impossible to eyeball. @smarterclayton, any specific areas we should look at carefully?
There was a problem hiding this comment.
There are also downgrade tests (https://k8s-testgrid.appspot.com/sig-release-master-upgrade#gce-master-new-downgrade-cluster) that cover the other direction.
But it's certainly not exhaustive. I'm all in favor of capturing the test coverage gap we think we have here and contributing to get it fixed. Should we open a new issue for that? Should we block this PR?
There was a problem hiding this comment.
Yeah, that doesn't give me very much additional confidence. It wouldn't catch something that's used even moderately rarely.
|
Since it's early in the cycle, and it's pretty difficult to get additional confidence, I'm inclined to take a risk and merge this. Be prepared to roll it back or fix quickly if we find problems :) /approve |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
added
the
approved
|
BTW, what's the performance impact of not using the unsafe package? |
@wojtek-t Is there a recommended way to flag this PR for performance testing? It's a strong candidate for having noticeable performance implications. |
@jpbetz @lavalamp BTW - I didn't look carefully into the whole PR, but where exactly we stopped using unsafe package? |
gogo/protobuf: https://github.com/gogo/protobuf/releases/tag/v0.5 (gogo/protobuf#343 has benchmarks) |
|
Thanks - that's useful to know. FYI: the test from yesterday is green, so it looks promissing (I will try to look deeper into individual metrics early next week). |
|
Thanks for checking @wojtek-t ! |
Vendor in the etcd 3.3 client targeting kubernetes 1.13 release milestone. We had previously tested the etcd 3.3 client with kubernetes via #58551.
Most of the work here was due to the need to bump to
gogo/protobufv0.5since the etcd client includes protobuf bindings generated for this version ofgogo/protobufand is not compatible with thev0.4-3-gc0656eddversion previously in use by Kubernetes. Since we're at the very beginning of the k8s 1.13 release cycle, and we are already lagging behind ongogo/protobufupdates, and thev0.5version includes noteworthy improvements, like the elimination of unsafe code in the generated bindings, this seems like a good time do the bump and regen.Also, added
':(exclude)**.*.pb.go'tohack/verify-pkg-names.shto resolve this lint error:/area etcd
/sig api-machinery
/kind feature
/cc @gyuho @xiang90 @wenjiaswe @jingyih