Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use latest etcd from release-3.3 branch for dropping ugorji #76917

Conversation

@dims
Copy link
Member

commented Apr 23, 2019

Change-Id: I3fb80ac61cfc3b7c549142be2d6c6d36766a58d4

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
etcd has been updated to drop ugorji dependency and switch over to json-iterator just like what we did in the kubernetes repository. Let's pick that change up so we can get rid of a bunch of code.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE
@dims

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

/retest

@dims dims force-pushed the dims:try-json-iterator-go-instead-of-ugorji-coded branch 2 times, most recently from 58fc8ef to ab5b9b0 Apr 23, 2019

@dims

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

/retest

1 similar comment
@dims

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

/retest

@dims dims force-pushed the dims:try-json-iterator-go-instead-of-ugorji-coded branch from ab5b9b0 to 669b707 Apr 23, 2019

@dims

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

/retest

@@ -658,7 +656,7 @@ func unmarshalHTTPResponse(code int, header http.Header, body []byte) (res *Resp

func unmarshalSuccessfulKeysResponse(header http.Header, body []byte) (*Response, error) {
var res Response
err := codec.NewDecoderBytes(body, new(codec.JsonHandle)).Decode(&res)
err := jsoniter.ConfigFastest.Unmarshal(body, &res)

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

need to use stdlib-compatible one that is case-sensitive, etc

This comment has been minimized.

Copy link
@dims

dims Apr 23, 2019

Author Member

ack switching to jsoniter.ConfigCompatibleWithStandardLibrary

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

actually, ConfigCompatibleWithStandardLibrary is not case-sensitive, and we really want it to be (c.f. #65034, #64612)

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

for comparison, ugorji is case-sensitive

This comment has been minimized.

Copy link
@dims

dims Apr 23, 2019

Author Member

Ack @liggitt Let's do this in https://github.com/etcd-io/etcd/pull/10667/commits and then bring back the changes here for testing.

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

I actually think it would be better to pursue eliminating use of the ".../coreos/etcd/client" package entirely. We shouldn't be using the v2 client for anything at this point, and we should request the constants we're referencing be copied to the clientv3 package

@dims

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

@liggitt unfortunately that won't be enough as we indirectly pull in ugorji stuff as ".../coreos/etcd/client" is used in ".../coreos/etcd/integration" and ".../coreos/etcd/discovery"

@dims dims force-pushed the dims:try-json-iterator-go-instead-of-ugorji-coded branch 2 times, most recently from 8fea46d to 2ccdb5c Apr 23, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

setting this in the replace portion of go.mod and running hack/update-vendor.sh would let you try this with exactly your branch:

github.com/coreos/etcd => github.com/dims/etcd v0.0.0-20190423213535-99140863f9cc

@dims dims force-pushed the dims:try-json-iterator-go-instead-of-ugorji-coded branch from 9bb6ea4 to 6a1efe3 Apr 25, 2019

@dims dims changed the title [NOT-FOR-REVIEW][WIP] Try json-iterator/go instead of ugorji/codec Use latest etcd from release-3.3 branch for dropping ugorji Apr 25, 2019

@dims

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

/priority important-soon

@dims

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

size - savings

dims@bigbox:~/junk$ diff -Bbwu before.raw after.raw
--- before.raw	2019-04-25 17:06:56.865091360 -0400
+++ after.raw	2019-04-25 17:03:53.663533820 -0400
@@ -490,6 +490,7 @@
 528K	./k8s.io/kubernetes/vendor/github.com/Azure/go-autorest/autorest.a
 544K	./k8s.io/kubernetes/vendor/github.com/google/cadvisor/manager.a
 556K	./k8s.io/kubernetes/vendor/github.com/libopenstorage/openstorage/api.a
+560K	./k8s.io/kubernetes/vendor/github.com/coreos/etcd/client.a
 564K	./k8s.io/kubernetes/vendor/github.com/asaskevich/govalidator.a
 592K	./k8s.io/kubernetes/vendor/gopkg.in/square/go-jose.v2/json.a
 600K	./k8s.io/kubernetes/vendor/google.golang.org/api/tpu/v1.a
@@ -511,7 +512,6 @@
 792K	./k8s.io/kubernetes/vendor/github.com/gogo/protobuf/protoc-gen-gogo/descriptor.a
 812K	./k8s.io/kubernetes/vendor/gopkg.in/square/go-jose.v2.a
 896K	./k8s.io/kubernetes/vendor/github.com/pkg/sftp.a
-908K	./k8s.io/kubernetes/vendor/github.com/coreos/etcd/client.a
 956K	./k8s.io/kubernetes/vendor/github.com/gogo/protobuf/types.a
 964K	./k8s.io/kubernetes/vendor/github.com/go-openapi/spec.a
 968K	./k8s.io/kubernetes/vendor/github.com/containerd/containerd/api/services/tasks/v1.a
@@ -557,7 +557,6 @@
 4.4M	./k8s.io/kubernetes/vendor/github.com/rancher/go-rancher/client.a
 6.3M	./k8s.io/kubernetes/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud.a
 8.5M	./k8s.io/kubernetes/vendor/github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-03-01/compute.a
-9.7M	./k8s.io/kubernetes/vendor/github.com/ugorji/go/codec.a
 14M	./k8s.io/kubernetes/vendor/github.com/Azure/azure-sdk-for-go/services/network/mgmt/2017-09-01/network.a
 14M	./k8s.io/kubernetes/vendor/google.golang.org/api/compute/v1.a
 15M	./k8s.io/kubernetes/vendor/github.com/xanzy/go-cloudstack/cloudstack.a
@dims

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

size of hyperkube
before

-rwxrwxr-x 1 dims dims 193M Apr 25 17:10 _output/local/go/bin/hyperkube

after

-rwxrwxr-x 1 dims dims 190M Apr 25 17:12 _output/local/bin/linux/amd64/hyperkube
go.mod Outdated
@@ -37,7 +37,7 @@ require (
github.com/containerd/containerd v1.0.2 // indirect
github.com/containerd/typeurl v0.0.0-20190228175220-2a93cfde8c20 // indirect
github.com/containernetworking/cni v0.6.0
github.com/coreos/etcd v3.3.10+incompatible
github.com/coreos/etcd v3.3.13-0.20190425194549-ad7c2cddb000+incompatible

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 25, 2019

Member

would prefer not to move off a tagged version for an untagged one. anything in flight in the 3.3.x stream we should be concerned about picking up half-baked?

This comment has been minimized.

Copy link
@dims

dims Apr 25, 2019

Author Member

@liggitt how about i ask for a new release just to be sure :)

@dims

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

/retest

@dims

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

fyi, looks like we may see a 3.3.14 for etcd within a week.

/hold

Use latest etcd from release-3.3 branch for dropping ugorji
Pick up changes from:
etcd-io/etcd#10675

Change-Id: Ic4d6daa3c54824d3d27809a125b798e88db0bf7e

@dims dims force-pushed the dims:try-json-iterator-go-instead-of-ugorji-coded branch from 6a1efe3 to 8824e0f May 2, 2019

@dims

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

@liggitt updated using hack/pin-dependency.sh github.com/coreos/etcd 98d308426819d892e149fe45f6fd542464cb1f9d

3.3.14 tag was cut today at that SHA https://github.com/etcd-io/etcd/tree/v3.3.13

/hold cancel

Thanks!

@liggitt

This comment has been minimized.

Copy link
Member

commented May 2, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, liggitt

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

commented May 3, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

commented May 3, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit aff37ad into kubernetes:master May 3, 2019

20 checks passed

cla/linuxfoundation dims authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.