Use protobufs by default to communicate with apiserver (still store JSONs in etcd) #25738

wojtek-t · 2016-05-17T12:16:08Z

@lavalamp @kubernetes/sig-api-machinery

smarterclayton · 2016-05-17T14:10:47Z

pkg/client/restclient/client.go

@@ -87,7 +87,7 @@ func NewRESTClient(baseURL *url.URL, versionedAPIPath string, config ContentConf
 		config.GroupVersion = &unversioned.GroupVersion{}
 	}
 	if len(config.ContentType) == 0 {
-		config.ContentType = "application/json"
+		config.ContentType = "application/vnd.kubernetes.protobuf"


This will mean that kubectl 1.3 will fail by default when contacting a Kube 1.2 server on POSTs and PUTs.

We may have to hoist this to just controller-manager, kubelet, and kube-proxy initialization.

oh.. I though about "get" and this should work, because of renegotiation. But you're right, that it will fail for post and put.

Will fix that. (but we are waiting for other PRs anyway first :))

smarterclayton · 2016-05-17T15:19:58Z

cmd/kube-controller-manager/app/options/options.go

@@ -145,7 +146,7 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&s.Master, "master", s.Master, "The address of the Kubernetes API server (overrides any value in kubeconfig)")
 	fs.StringVar(&s.Kubeconfig, "kubeconfig", s.Kubeconfig, "Path to kubeconfig file with authorization and master location information.")
 	fs.StringVar(&s.RootCAFile, "root-ca-file", s.RootCAFile, "If set, this root certificate authority will be included in service account's token secret. This must be a valid PEM-encoded CA bundle.")
-	fs.StringVar(&s.ContentType, "kube-api-content-type", s.ContentType, "ContentType of requests sent to apiserver. Passing application/vnd.kubernetes.protobuf is an experimental feature now.")
+	fs.StringVar(&s.ContentType, "kube-api-content-type", s.ContentType, "ContentType of requests sent to apiserver.")


nit: "Content type"

smarterclayton · 2016-05-18T15:13:26Z

I thought of a possible problem here.

Etcd2 requires a base64 encoded body for proto. A user using proto on etcd2 will have base64 encoded data in etcd. A user in etcd3 does not need base64 encoding (because of binary values) but if they have proto in etcd2 and upgrade they'll have different data on disk or not.

Currently the recognizer only enables base64 decoding when you have a binary storage format. I'd prefer not to enable base64 except as a limited stopgap until folks move to etcd3. So we'd have to add a base64 decoder if we allow base64 encoded protobufs to be written to etcd2, and leave it in the product for a while, and force upgrades and testing.

If we are not going to recommend etcd3 by default for kube 1.3, then we may not want to recommend protobuf storage for kube 1.3 on etcd2. I lean a bit towards that option.

lavalamp · 2016-05-18T22:46:28Z

@smarterclayton is it so bad to turn on a base64 recognizer forever? Presumably it can just look for a fixed starting string just like the current binary recognizer?

@xiang90 @hongchaodeng what is the current etcd3 status?

I don't think we're ready to run on etcd3 (config, setup, upgrade testing, etc)

@roberthbailey since we were just talking about this.

hongchaodeng · 2016-05-18T23:04:42Z

what is the current etcd3 status?

@lavalamp
Write updates on #22448 (comment)

Thanks!

wojtek-t · 2016-05-19T12:10:04Z

@smarterclayton - we will not recommend etcd3 in 1.3 release.

However, without switching on protobufs, we won't be able to handle 2000-nodes. We need to check the results of switching protobufs only for communication with apiserver and not for storage.

smarterclayton · 2016-05-19T12:29:25Z

Is it terrible to support the recognizer? No. It's just messy because
it's a point in time thing, and does have a small fixed cost.

On Thu, May 19, 2016 at 8:10 AM, Wojciech Tyczynski <
notifications@github.com> wrote:

@smarterclayton https://github.com/smarterclayton - we will not
recommend etcd3 in 1.3 release.

However, without switching on protobufs, we won't be able to handle
2000-nodes. We need to check the results of switching protobufs only for
communication with apiserver and not for storage.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25738 (comment)

wojtek-t · 2016-05-19T12:45:04Z

@smarterclayton - I changed this PR to not touch default storage media type for now. Are you ok with merging this one (as soon as #25243 is merged)?

smarterclayton · 2016-05-19T12:53:49Z

Yes I am.

wojtek-t · 2016-05-20T09:35:49Z

@smarterclayton - can we proceed with this PR now?

smarterclayton · 2016-05-20T14:11:37Z

Yes, Lgtm

wojtek-t · 2016-05-21T09:38:56Z

Trivial rebase - reapplying lgtm.

k8s-github-robot · 2016-05-21T17:48:41Z

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

k8s-bot · 2016-05-21T18:33:42Z

GCE e2e build/test passed for commit 0f881d6.

k8s-github-robot · 2016-05-21T18:48:25Z

Automatic merge from submit-queue

wojtek-t · 2016-05-23T12:11:27Z

Ref #8132

wojtek-t assigned smarterclayton May 17, 2016

wojtek-t added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 17, 2016

googlebot added the cla: yes label May 17, 2016

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

wojtek-t added this to the v1.3 milestone May 17, 2016

smarterclayton reviewed May 17, 2016
View reviewed changes

wojtek-t force-pushed the default_protobuf branch from eb3a316 to 9a370eb Compare May 17, 2016 14:29

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 17, 2016

smarterclayton reviewed May 17, 2016
View reviewed changes

wojtek-t force-pushed the default_protobuf branch from 9a370eb to 76dbe1b Compare May 19, 2016 12:37

wojtek-t changed the title ~~[Do NOT merge] Use protobufs by default~~ [Do NOT merge] Use protobufs by default to communicate with apiserver May 19, 2016

wojtek-t force-pushed the default_protobuf branch from 76dbe1b to 2344d12 Compare May 20, 2016 08:24

wojtek-t changed the title ~~[Do NOT merge] Use protobufs by default to communicate with apiserver~~ Use protobufs by default to communicate with apiserver May 20, 2016

wojtek-t added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels May 20, 2016

k8s-github-robot added the kind/old-docs label May 20, 2016

wojtek-t changed the title ~~Use protobufs by default to communicate with apiserver~~ Use protobufs by default to communicate with apiserver (still store JSONs in etcd) May 20, 2016

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2016

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

Use protobufs by default to communicate with apiserver

0f881d6

wojtek-t force-pushed the default_protobuf branch from 2344d12 to 0f881d6 Compare May 21, 2016 09:38

wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 21, 2016

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

k8s-github-robot merged commit 682c188 into kubernetes:master May 21, 2016

wojtek-t mentioned this pull request May 23, 2016

Protobuf communications #8132

Closed

wojtek-t deleted the default_protobuf branch June 15, 2016 14:34

SEJeff mentioned this pull request Sep 8, 2017

Implement Quota/Rate Limiting northwesternmutual/kanali#56

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use protobufs by default to communicate with apiserver (still store JSONs in etcd) #25738

Use protobufs by default to communicate with apiserver (still store JSONs in etcd) #25738

wojtek-t commented May 17, 2016

smarterclayton May 17, 2016

smarterclayton May 17, 2016

wojtek-t May 17, 2016

smarterclayton May 17, 2016

smarterclayton commented May 18, 2016

lavalamp commented May 18, 2016

hongchaodeng commented May 18, 2016

wojtek-t commented May 19, 2016

smarterclayton commented May 19, 2016

wojtek-t commented May 19, 2016

smarterclayton commented May 19, 2016

wojtek-t commented May 20, 2016

smarterclayton commented May 20, 2016

wojtek-t commented May 21, 2016

k8s-github-robot commented May 21, 2016

k8s-bot commented May 21, 2016

k8s-github-robot commented May 21, 2016

wojtek-t commented May 23, 2016

Use protobufs by default to communicate with apiserver (still store JSONs in etcd) #25738

Use protobufs by default to communicate with apiserver (still store JSONs in etcd) #25738

Conversation

wojtek-t commented May 17, 2016

smarterclayton May 17, 2016

Choose a reason for hiding this comment

smarterclayton May 17, 2016

Choose a reason for hiding this comment

wojtek-t May 17, 2016

Choose a reason for hiding this comment

smarterclayton May 17, 2016

Choose a reason for hiding this comment

smarterclayton commented May 18, 2016

lavalamp commented May 18, 2016

hongchaodeng commented May 18, 2016

wojtek-t commented May 19, 2016

smarterclayton commented May 19, 2016

wojtek-t commented May 19, 2016

smarterclayton commented May 19, 2016

wojtek-t commented May 20, 2016

smarterclayton commented May 20, 2016

wojtek-t commented May 21, 2016

k8s-github-robot commented May 21, 2016

k8s-bot commented May 21, 2016

k8s-github-robot commented May 21, 2016

wojtek-t commented May 23, 2016