Add support for HA control plane by pmorie · Pull Request #337 · knative/serving-operator

pmorie · 2020-03-10T18:53:41Z

Fixes #320

Proposed Changes

Adds KnativeServing.spec.highAvailability

Release Note

Initial support for highly available control plane

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

jcrossley3

Looks good, but this won't actually do anything until you add HighAvailabilityTransform to the Transformers function in extenstions.go

jcrossley3 · 2020-03-10T18:56:29Z

config/300-serving-v1alpha1-knativeserving-crd.yaml

                name:
                  description: The name of the ConfigMap or Secret
                  type: string
+            highAvailability:


We're mixing case convention here, should be high-availability to be consistent

jcrossley3 · 2020-03-10T19:02:36Z

pkg/reconciler/knativeserving/common/ha.go

+		if instance.Spec.HighAvailability == nil {
+			return nil
+		}


You could put this outside the closure for a skosh more performance. Transform will ignore a nil Transformer

I would rather do this in a follow-up that changes everywhere that does this, since this PR is blocking the release

jcrossley3 · 2020-03-10T19:14:11Z

pkg/reconciler/knativeserving/common/ha.go

+			if cm.Data == nil {
+				cm.Data = map[string]string{}
+			}
+			cm.Data[enabledComponentsKey] = componentsValue


You might want to log a warning if this value is also present in the instance.Config map... maybe?

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

jcrossley3

Less verbosity

jcrossley3 · 2020-03-10T23:04:35Z

pkg/reconciler/knativeserving/common/ha.go

+			if err := scheme.Scheme.Convert(cm, u, nil); err != nil {
+				return err
+			}


Suggested change

if err := scheme.Scheme.Convert(cm, u, nil); err != nil {

return err

}

return scheme.Scheme.Convert(cm, u, nil)

jcrossley3 · 2020-03-10T23:05:34Z

pkg/reconciler/knativeserving/common/ha.go

+			if err := scheme.Scheme.Convert(deployment, u, nil); err != nil {
+				return err
+			}


Suggested change

if err := scheme.Scheme.Convert(deployment, u, nil); err != nil {

return err

}

return scheme.Scheme.Convert(deployment, u, nil)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor · 2020-03-11T04:18:19Z

pkg/reconciler/knativeserving/common/ha_test.go

+		},
+	}
+
+	for i, _ := range cases {


Format Go code:

Suggested change

for i, _ := range cases {

for i := range cases {

jcrossley3 · 2020-03-11T10:20:21Z

pkg/reconciler/knativeserving/common/extensions.go

-	for _, fn := range platforms {
+	for i := range platforms {
+		fn := platforms[i]


Is this really necessary?

jcrossley3 · 2020-03-11T11:08:29Z

I think you've accidentally discovered a ginormous manifestival bug. Holding until I create a fix...
/hold

pmorie · 2020-03-11T18:17:00Z

/hold

pmorie · 2020-03-11T18:51:02Z

/hold cancel

jcrossley3 · 2020-03-11T21:02:09Z

pkg/reconciler/knativeserving/common/ha.go

+			if err := unstructured.SetNestedStringMap(u.Object, data, "data"); err != nil {
+				return err
+			}
+			log.Infof("Updated configmap/config-leader-election: %v", u.Object)


This is just noise. I would delete these logs since manifestival will log the changes from them anyway. But if you insist, make 'em debug?

It actually won't give you the full content of the object, which i needed, but I can make a PR for manifestival for that.

knative-metrics-robot · 2020-03-11T21:29:58Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/knativeserving/common/ha.go	Do not exist	81.2%

houshengbo · 2020-03-11T22:01:45Z

/lgtm
/approve

knative-prow-robot · 2020-03-11T22:01:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: houshengbo, pmorie

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [houshengbo]

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

pmorie · 2020-03-13T00:57:25Z

Part of knative/pkg#1007

googlebot added the cla: yes Author(s) signed a CLA. label Mar 10, 2020

knative-prow-robot added the size/L label Mar 10, 2020

knative-prow-robot requested review from jcrossley3 and trshafer March 10, 2020 18:54

mattmoor reviewed Mar 10, 2020

View reviewed changes

jcrossley3 reviewed Mar 10, 2020

View reviewed changes

mattmoor reviewed Mar 10, 2020

View reviewed changes

jcrossley3 reviewed Mar 10, 2020

View reviewed changes

mattmoor reviewed Mar 11, 2020

View reviewed changes

pmorie force-pushed the ha branch from 18de8e7 to 5960753 Compare March 11, 2020 04:17

mattmoor reviewed Mar 11, 2020

View reviewed changes

jcrossley3 reviewed Mar 11, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold label Mar 11, 2020

jcrossley3 mentioned this pull request Mar 11, 2020

Bump to manifestival 0.4.0 #338

Merged

pmorie added 3 commits March 11, 2020 14:21

Add KnativeServing.spec.availableReplicas

19ba8c4

pr feedback

b266bd7

pr feedback

ddadd49

pmorie force-pushed the ha branch from 5960753 to ddadd49 Compare March 11, 2020 18:31

knative-prow-robot removed the do-not-merge/hold label Mar 11, 2020

jcrossley3 reviewed Mar 11, 2020

View reviewed changes

pmorie added 2 commits March 11, 2020 17:16

pr feedback

57d451e

pr feedback

e88f04a

knative-prow-robot assigned houshengbo Mar 11, 2020

knative-prow-robot added the lgtm label Mar 11, 2020

knative-prow-robot added the approved label Mar 11, 2020

knative-prow-robot merged commit 0255420 into knative:master Mar 11, 2020

aliok mentioned this pull request Apr 4, 2020

Operator support for HA deployments #279

Open

Conversation

pmorie commented Mar 10, 2020

Proposed Changes

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

jcrossley3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

jcrossley3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcrossley3 commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmorie commented Mar 11, 2020

Uh oh!

pmorie commented Mar 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Mar 11, 2020

Uh oh!

houshengbo commented Mar 11, 2020

Uh oh!

knative-prow-robot commented Mar 11, 2020

Uh oh!

pmorie commented Mar 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jcrossley3 commented Mar 11, 2020 •

edited

Loading