Enable protobuf in Origin for server-to-server #9814

smarterclayton · 2016-07-13T05:52:21Z

This builds on top of the other protobuf PR and tests full protobuf throughout the stack (client -> server, server -> server). Also fixes some conversion inefficiencies in authorization (which is surprisingly slow).

smarterclayton · 2016-07-13T05:52:32Z

[test]

smarterclayton · 2016-07-19T16:33:23Z

[test]

On Tue, Jul 19, 2016 at 12:30 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6462/)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9814 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p56MkNoVnxHJ1Rj9cQNdmZTDKvvqks5qXPu6gaJpZM4JLFc7
.

smarterclayton · 2016-07-19T17:50:46Z

[test]

smarterclayton · 2016-07-19T19:05:59Z

[test]

On Tue, Jul 19, 2016 at 3:00 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6482/)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9814 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p0s_DTT4LupxnOrPHXSeQIOMdiFhks5qXR7igaJpZM4JLFc7
.

smarterclayton · 2016-07-19T21:28:32Z

[test]

On Tue, Jul 19, 2016 at 3:21 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6491/)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9814 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p_l7qbJJZZ2R99Kvs4ap3wDtKtKDks5qXSOzgaJpZM4JLFc7
.

smarterclayton · 2016-07-22T05:46:15Z

@deads2k @liggitt this is ready for review (or delegation to someone else). This is running all of our tests with protobuf enabled for both client and server - prior to merge I'll drop the "DO NOT MERGE" commit that forces it on for clients. I need to backport a few comments from c983121 from the other PR - it's easier to make the changes in here because I have better round tripping tests for both proto and json and it has the optional names changes.

smarterclayton · 2016-07-22T05:46:34Z

All but 28933 is merged upstream.

smarterclayton · 2016-07-22T14:21:10Z

Omg

deads2k · 2016-07-22T17:08:01Z

Omg

had to have been a flake, no way it succeeded :)

deads2k · 2016-07-22T17:10:36Z

pkg/template/api/v1/conversion.go

-		panic(err)
+func (c *Template) EncodeNestedObjects(e runtime.Encoder) error {
+	for i := range c.Objects {
+		if err := extension.EncodeNestedRawExtension(runtime.UnstructuredJSONScheme, &c.Objects[i]); err != nil {


well this worked shockingly well.

Sometimes I get things right.

Has already been removed in the next rebase.

…sion generation Currently it only skips if the fields don't match, but that leaves no way for callers to say "no really, ignore this field".

Forces templates to round-trip to JSON, not any other serialization.

Force PATH on update-generated-protobuf Run verify-generated-protobuf.sh during test-end-to-end We don't assume docker elsewhere, so for now we'll put it in this step and then move it later.

Handle more optional API fields

Allows conversions to be automatically generated, and the name is not externally visible.

smarterclayton · 2016-07-24T18:40:32Z

[test] conformance docker fail

New clusters will get protobuf intracluster, old clusters will not.

smarterclayton · 2016-07-25T05:24:46Z

[test] secret mount failure

openshift-bot · 2016-07-25T05:25:18Z

Evaluated for origin test up to 92685ee

openshift-bot · 2016-07-25T06:38:55Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6806/)

smarterclayton · 2016-07-25T12:43:10Z

Updated, PTAL

deads2k · 2016-07-25T17:12:25Z

pkg/cmd/server/api/types.go

+	ExternalKubernetesClientConnectionOverrides *ClientConnectionOverrides
+}
+
+type ClientConnectionOverrides struct {


I don't really like this being all or nothing and we have to live with this config forever.

@liggitt do you object too or am I just being ornery?

deads2k · 2016-07-25T17:58:43Z

lgtm [merge]

openshift-bot · 2016-07-25T18:01:04Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6806/) (Image: devenv-rhel7_4663)

openshift-bot · 2016-07-25T18:01:05Z

Evaluated for origin merge up to 92685ee

smarterclayton force-pushed the protobuf_enabled branch from 9b88a33 to ccef526 Compare July 13, 2016 05:54

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2016

smarterclayton force-pushed the protobuf_enabled branch 2 times, most recently from 2152acf to 7f04141 Compare July 19, 2016 04:39

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2016

smarterclayton force-pushed the protobuf_enabled branch 2 times, most recently from 5d8d35e to ba54434 Compare July 20, 2016 03:40

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2016

smarterclayton force-pushed the protobuf_enabled branch 2 times, most recently from eba5cec to 50ca5c9 Compare July 22, 2016 03:30

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2016

smarterclayton force-pushed the protobuf_enabled branch 2 times, most recently from 3cdb26a to 7360833 Compare July 22, 2016 05:43

smarterclayton changed the title ~~WIP - Test enabling protobuf everywhere~~ Fully enable us for protobuf Jul 22, 2016

smarterclayton changed the title ~~Fully enable us for protobuf~~ Enable protobuf in Origin for server-to-server Jul 22, 2016

smarterclayton mentioned this pull request Jul 22, 2016

Follow on to the AuthorizationAttributes -> Action PR to fix authorization conversions #9838

Closed

deads2k reviewed Jul 22, 2016
View reviewed changes

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2016

smarterclayton added 10 commits July 24, 2016 12:01

UPSTREAM: 28933: Handle server errors more precisely

4c97110

UPSTREAM: 28932: Fail correctly in go-to-protobuf

70be02e

UPSTREAM: <drop>: Drop LegacyCodec in unversioned client

87e06a5

Has already been removed in the next rebase.

UPSTREAM: 28934: Unable to have optional message slice

96c2e49

UPSTREAM: 28931: genconversion=false should skip fields during conver…

8d0afb2

…sion generation Currently it only skips if the fields don't match, but that leaves no way for callers to say "no really, ignore this field".

Templates should use NestedObject* with Unstructured scheme

d71dfa7

Forces templates to round-trip to JSON, not any other serialization.

Verify protobuf during runs

4e0ba9b

Force PATH on update-generated-protobuf Run verify-generated-protobuf.sh during test-end-to-end We don't assume docker elsewhere, so for now we'll put it in this step and then move it later.

Fix issues running protobuf from clients

bf711f7

Handle more optional API fields

Rename AuthorizationAttributes -> Action

edcc5b4

Allows conversions to be automatically generated, and the name is not externally visible.

Make authorization conversions efficient

9aacfa7

smarterclayton force-pushed the protobuf_enabled branch from 9937762 to 4f81d31 Compare July 24, 2016 16:02

smarterclayton force-pushed the protobuf_enabled branch from 4f81d31 to 240c751 Compare July 24, 2016 19:19

smarterclayton added 2 commits July 24, 2016 22:11

Enable protobuf for new configurations, json for old

1f31ddc

New clusters will get protobuf intracluster, old clusters will not.

generated: For proto and authorization changes

92685ee

smarterclayton force-pushed the protobuf_enabled branch from 240c751 to 92685ee Compare July 25, 2016 02:11

deads2k reviewed Jul 25, 2016
View reviewed changes

smarterclayton mentioned this pull request Jul 25, 2016

Significant regression in API server due to policy code path #9368

Closed

openshift-bot merged commit 9c76951 into openshift:master Jul 25, 2016

deads2k mentioned this pull request Aug 12, 2016

applyClientConnectionOverrides should not fall back to 5qps for empty configs #10368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable protobuf in Origin for server-to-server #9814

Enable protobuf in Origin for server-to-server #9814

smarterclayton commented Jul 13, 2016

smarterclayton commented Jul 13, 2016

smarterclayton commented Jul 19, 2016

smarterclayton commented Jul 19, 2016

smarterclayton commented Jul 19, 2016

smarterclayton commented Jul 19, 2016

smarterclayton commented Jul 22, 2016

smarterclayton commented Jul 22, 2016

smarterclayton commented Jul 22, 2016

deads2k commented Jul 22, 2016

deads2k Jul 22, 2016

smarterclayton Jul 22, 2016

smarterclayton commented Jul 24, 2016 via email

smarterclayton commented Jul 25, 2016

openshift-bot commented Jul 25, 2016

openshift-bot commented Jul 25, 2016

smarterclayton commented Jul 25, 2016 via email

deads2k Jul 25, 2016

deads2k commented Jul 25, 2016

openshift-bot commented Jul 25, 2016 •

edited

openshift-bot commented Jul 25, 2016

Enable protobuf in Origin for server-to-server #9814

Enable protobuf in Origin for server-to-server #9814

Conversation

smarterclayton commented Jul 13, 2016

smarterclayton commented Jul 13, 2016

smarterclayton commented Jul 19, 2016

smarterclayton commented Jul 19, 2016

smarterclayton commented Jul 19, 2016

smarterclayton commented Jul 19, 2016

smarterclayton commented Jul 22, 2016

smarterclayton commented Jul 22, 2016

smarterclayton commented Jul 22, 2016

deads2k commented Jul 22, 2016

deads2k Jul 22, 2016

Choose a reason for hiding this comment

smarterclayton Jul 22, 2016

Choose a reason for hiding this comment

smarterclayton commented Jul 24, 2016 via email

smarterclayton commented Jul 25, 2016

openshift-bot commented Jul 25, 2016

openshift-bot commented Jul 25, 2016

smarterclayton commented Jul 25, 2016 via email

deads2k Jul 25, 2016

Choose a reason for hiding this comment

deads2k commented Jul 25, 2016

openshift-bot commented Jul 25, 2016 • edited

openshift-bot commented Jul 25, 2016

openshift-bot commented Jul 25, 2016 •

edited