Enables status subresource in order to fix generation bumping #786

bsnchan · 2018-04-30T19:25:49Z

Fixes #642

Proposed Changes

all Elafros CRDs now use the status subresource - api server now bumps metadata.generation
remove all generation bumping logic from the webhook
controllers now reference metadata.generation instead of spec.generation
patching is used instead of update when adding route labels to the configuration
- prevents bumping the generation since nested template specs have a k8s
  Time struct which, during serialization, are not omitted
  when empty

Release Note

* all Elafros CRDs now use the status subresource to enable automatic generation bumping
* action required: Requires k8s 1.10
* action required: kube-apiserver has the feature gate `CustomResourceSubresources=true` set

cc @dprotaso

googlebot · 2018-04-30T19:25:52Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

evankanderson

Thanks, this looks good.

evankanderson · 2018-04-30T22:26:10Z

pkg/apis/ela/v1alpha1/configuration_types.go

@@ -122,14 +116,6 @@ type ConfigurationList struct {
 	Items []Configuration `json:"items"`
 }

-func (r *Configuration) GetGeneration() int64 {
-	return r.Spec.Generation


You could have left these and moved them to point to the ObjectMeta instead.

You don't need to since metav1.ObjectMeta has it here: https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/meta.go#L141

And metav1.ObjectMeta is inlined in Configuration here: https://github.com/pivotal-cf-experimental/elafros/blob/fda19f760c672e4b0f99f6efb20f71a8b5444d9c/pkg/apis/ela/v1alpha1/configuration_types.go#L39

evankanderson · 2018-04-30T22:28:38Z

pkg/apis/ela/v1alpha1/service_types_test.go

-      isReady: false,
-    },
-  }
+	cases := []struct {


@adrcunha @bobcatfish

Can we add a test that 'gofmt' is a no-op on all our go files? Somehow these got checked in with 2 spaces rather than tabs earlier.

@evankanderson @jonjohnsonjr @dprotaso It looks like the relevant issue is #582 - prow has a /lint command but there are obstacles to running automatically.

If you don't mind waiting I'm looking into something related which might help with this 😇

and then i never did anything about it :(

evankanderson · 2018-04-30T22:29:31Z

pkg/controller/configuration/configuration.go

@@ -265,7 +265,7 @@ func (c *Controller) syncHandler(key string) error {
 	if config.GetGeneration() == config.Status.ObservedGeneration {
 		// TODO(vaikas): Check to see if Status.LatestCreatedRevisionName is ready and update Status.LatestReady
 		glog.Infof("Skipping reconcile since already reconciled %d == %d",
-			config.Spec.Generation, config.Status.ObservedGeneration)
+			config.Generation, config.Status.ObservedGeneration)


Why isn't this config.ObjectMeta.Generation, like Annotations, below?

mostly just curious.

metav1.ObjectMeta being inlined - you could used config.Annotations directly actually. Might be worth a separate issue to define a convention.

evankanderson · 2018-04-30T22:30:18Z

pkg/controller/configuration/configuration.go

@@ -361,25 +361,12 @@ func (c *Controller) syncHandler(key string) error {
 }

 func generateRevisionName(u *v1alpha1.Configuration) (string, error) {
-	// TODO: consider making sure the length of the
-	// string will not cause problems down the stack
-	if u.Spec.Generation == 0 {


I'm assuming that this is now handled by the normal k8s machinery? This was really annoying before.

Yup. Though, like it's mentioned in the PR/commit message, it requires v1.10 and the feature gate.

evankanderson · 2018-04-30T22:37:39Z

pkg/controller/route/route.go

-		if config.Labels == nil {
-			config.Labels = make(map[string]string)
-		} else if _, ok := config.Labels[ela.RouteLabelKey]; ok {
+		if _, ok := config.Labels[ela.RouteLabelKey]; ok {


We didn't need the previous check because "A nil map behaves like an empty map when reading, but attempts to write to a nil map will cause a runtime panic; don't do that.", right?

https://blog.golang.org/go-maps-in-action

Yup - see example here: https://play.golang.org/p/Q5GRlc2ceES

You needed the make(map) before this PR since it was going to insert something into the map

evankanderson · 2018-04-30T22:39:34Z

pkg/controller/route/route.go

+	routeName,
+	configName string) error {
+
+	pathPart := strings.Replace(ela.RouteLabelKey, "/", "~1", -1)


What's the output here? The "~1" is confusing to me. Might be worth a comment.

Once this is PR merged in jsonpath we'll change this to be

patch := []jsonpatch.JsonPatchOperation{ { Operation: "add", Path: jsonpath.MakePath("metadata/labels", ela.RouteLabelKey), Value: routeName, }, ]}

+1 to comment, probably linking to the PR.

evankanderson · 2018-04-30T22:45:32Z

pkg/webhook/webhook.go

-		}(),
+		Patch:     patchBytes,
+		Allowed:   true,
+		PatchType: &patchType,


For my own curiosity, why doesn't &admissionv1beta1.PatchTypeJSONPatch work, and we need to make a local copy instead?

Go doesn't allow it see: https://stackoverflow.com/questions/35146286/find-address-of-constant-in-go

The inline function was doing the same thing before - this is more obvious in my opinion

+1 I prefer the local over the function.

evankanderson · 2018-04-30T23:30:39Z

/approve
/lgtm

You may need to chime in that everything is happy with CLAs (and it looks like our tests are unhappy on this PR for some reason). A sample error from the conformance test:

I0430 19:43:09.661] E0430 19:42:33.692063       1 route.go:618] Failed to patch Configuration configuration-example: jsonpatch add operation does not apply: doc is missing path: /metadata/labels/elafros.dev~1route
I0430 19:43:09.661] E0430 19:42:33.692113       1 route.go:295] error syncing "default/route-example": jsonpatch add operation does not apply: doc is missing path: /metadata/labels/elafros.dev~1route

dprotaso · 2018-04-30T23:53:38Z

@evankanderson the google bot is having issues when @bsnchan and I co-author commits/PRs. It's getting pretty annoying at this point.

The failing test is probably because the k8s test environment prow? is using kubernetes 1.9.6 vs. 1.10 with the feature-gate

evankanderson · 2018-05-01T00:04:22Z

That was my guess for the latter. For the CLA case, I don't know if having you do a single commit into the repo directly will convince the CLA-bot that everything is okay. If not, it may be worth filing an issue with @mchmarny to see if we can whitelist you somehow.

…

On Mon, Apr 30, 2018 at 4:53 PM David Protasowski ***@***.***> wrote: @evankanderson <https://github.com/evankanderson> the google bot is having issues when @bsnchan <https://github.com/bsnchan> and I co-author commits/PRs. It's getting pretty annoying at this point. The failing test is probably because the k8s test environment prow? is using kubernetes 1.9.6 vs. 1.10 with the feature-gate — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#786 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHlyN03-psFWZp0Y4dLmRgoxMbZ_IXvKks5tt6QDgaJpZM4TtFxh> .

-- Evan Anderson <argent@google.com>

dprotaso · 2018-05-01T00:07:21Z

I think it's more about @bsnchan opening PRs which have me as co-author. I type

/approve
/iapprove
/imokwithit

nm - it worked

tcnghia

/lgtm

adrcunha · 2018-05-02T23:47:29Z

/retest

mattmoor · 2018-06-04T16:01:31Z

/hold cancel

I think this is (finally) unblocked. @dprotaso @bsnchan thanks for your patience

google-prow-robot · 2018-06-06T14:40:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsnchan, dprotaso, evankanderson

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:

~~OWNERS~~ [evankanderson]

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

* This requires k8s 1.10 with CustomResourceSubresources feature gate enabled * Patching is used instead of update when adding labels - prevents bumping the generation since nested specs have a k8s Time struct which, during serialization, are not omitted when empty knative/serving#642 Signed-off-by: Brenda Chan <brchan@pivotal.io>

Signed-off-by: Brenda Chan <brchan@pivotal.io>

Signed-off-by: Dave Protasowski <dprotaso@gmail.com>

* This is required to enable CustomResourceSubresources knative/serving#642

knative-metrics-robot · 2018-06-06T14:42:48Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/configuration_types.go	96.0%	95.7%	-0.3%
pkg/apis/serving/v1alpha1/configuration_types_test.go	96.0%	95.7%	-0.3%
pkg/apis/serving/v1alpha1/revision_types.go	82.8%	81.5%	-1.3%
pkg/apis/serving/v1alpha1/revision_types_test.go	82.8%	81.5%	-1.3%
pkg/apis/serving/v1alpha1/route_types.go	58.3%	63.6%	5.3%
pkg/apis/serving/v1alpha1/service_types.go	95.8%	95.5%	-0.4%
pkg/apis/serving/v1alpha1/service_types_test.go	95.8%	95.5%	-0.4%
pkg/controller/configuration/configuration.go	76.9%	77.4%	0.5%
pkg/controller/configuration/configuration_test.go	76.9%	77.4%	0.5%
pkg/controller/route/route.go	77.9%	77.7%	-0.3%
pkg/controller/route/route_test.go	77.9%	77.7%	-0.3%
pkg/controller/revision/revision.go	74.8%	74.8%	-0.0%
pkg/webhook/webhook.go	44.6%	38.1%	-6.5%
pkg/webhook/webhook_test.go	44.6%	38.1%	-6.5%

*TestCoverage feature is being tested, do not rely on any info here yet

knative-metrics-robot · 2018-06-06T14:44:55Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	82.8%	81.5%	-1.3%
pkg/apis/serving/v1alpha1/revision_types_test.go	82.8%	81.5%	-1.3%
pkg/apis/serving/v1alpha1/route_types.go	58.3%	63.6%	5.3%
pkg/apis/serving/v1alpha1/service_types.go	95.8%	95.5%	-0.4%
pkg/apis/serving/v1alpha1/service_types_test.go	95.8%	95.5%	-0.4%
pkg/apis/serving/v1alpha1/configuration_types.go	96.0%	95.7%	-0.3%
pkg/apis/serving/v1alpha1/configuration_types_test.go	96.0%	95.7%	-0.3%
pkg/controller/configuration/configuration.go	76.9%	77.4%	0.5%
pkg/controller/configuration/configuration_test.go	76.9%	77.4%	0.5%
pkg/controller/route/route.go	77.9%	77.7%	-0.3%
pkg/controller/route/route_test.go	77.9%	77.7%	-0.3%
pkg/controller/revision/revision.go	74.8%	74.8%	-0.0%
pkg/webhook/webhook.go	44.6%	38.1%	-6.5%
pkg/webhook/webhook_test.go	44.6%	38.1%	-6.5%

*TestCoverage feature is being tested, do not rely on any info here yet

bsnchan · 2018-06-06T15:04:16Z

@mattmoor this should be good to go!

@dprotaso already approved the CLA business in this comment.

dprotaso · 2018-06-11T13:03:41Z

@mattmoor @vaikas-google can we mark this one blocked on k8s 1.11 when the subresource feature gate moves to beta.

evankanderson · 2018-06-11T20:41:49Z

To record for posterity the conversation as I know it:

The subresource support in k8s 1.10 is an Alpha feature locked behind a feature flag. Many k8s providers do not provide any access to alpha features, and requiring an alpha feature is likely to make it difficult to run the serving components on shared or production clusters. (GKE does provide access to alpha features, but limits alpha cluster lifespan to 30 days. AWS and Azure don't generally provide access to alpha features at all.)

We're going to hold this PR for 1.11 (maybe plus a week or two?) when CRD subresources should be a beta feature. Once we commit this to the repo and cut a release containing it, our customers will need to upgrade to k8s 1.11 in order to continue using the serving stack.

mattmoor · 2018-06-11T20:43:02Z

/hold

As @evankanderson said.

evankanderson · 2018-06-11T20:50:27Z

As such, if there are parts where we can factor out the actual subresource call and flag-protect it (e.g. replace the subresource call with a PATCH on the parent for now), we could land this sooner.

We'd probably have to keep the Generation in the object specs for now, but many of the helpers could go in now, and we could phase in the two write paths in a follow-on PR if this one gets too unwieldy.

bsnchan · 2018-06-28T17:27:39Z

/label k8s-1.11

bsnchan · 2018-07-04T14:23:53Z

/label k8s 1.11

bsnchan · 2018-07-04T14:25:34Z

/label "k8s 1.11"

bbrowning · 2018-09-28T19:39:21Z

Should we revisit this since Kubernetes 1.11 has been out for 3 months and Kubernetes 1.12 has since been released? Or would we like to hold off requiring Kubernetes 1.11 for the next release of Serving?

dprotaso · 2018-11-14T15:49:33Z

@bbrowning yup it's on me

dprotaso · 2018-11-14T15:50:08Z

GKE now has 1.11 - so our the Prow CI/CD can test the changes

bsnchan requested review from adrcunha, bobcatfish, evankanderson, grantr, mattmoor, tcnghia and vaikas as code owners April 30, 2018 19:25

google-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 30, 2018

evankanderson approved these changes Apr 30, 2018

View reviewed changes

google-prow-robot assigned evankanderson Apr 30, 2018

google-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 30, 2018

dprotaso mentioned this pull request May 1, 2018

No test framework #778

Merged

evankanderson assigned bobcatfish and grantr and unassigned evankanderson May 2, 2018

bsnchan force-pushed the 1-10-crd-subresources branch from fda19f7 to ef71832 Compare May 2, 2018 18:59

google-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 2, 2018

tcnghia approved these changes May 2, 2018

View reviewed changes

google-prow-robot assigned tcnghia May 2, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2018

evankanderson unassigned grantr May 2, 2018

bobcatfish approved these changes May 17, 2018

View reviewed changes

google-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2018

bsnchan force-pushed the 1-10-crd-subresources branch from cfdc7fe to 75f3655 Compare June 6, 2018 14:40

dprotaso and others added 4 commits June 6, 2018 10:41

Refactor adding/removing labels to a helper

5a37558

Signed-off-by: Brenda Chan <brchan@pivotal.io>

Use gke 1.10 and enable alpha features for conformance tests

dabed47

Signed-off-by: Dave Protasowski <dprotaso@gmail.com>

Enable Kubernetes alpha features for GKE 1.10

642ee1c

* This is required to enable CustomResourceSubresources knative/serving#642

bsnchan force-pushed the 1-10-crd-subresources branch from 75f3655 to 642ee1c Compare June 6, 2018 14:41

This was referenced Jun 11, 2018

Best practices for updating status #1107

Closed

Best practices for updating a CRD's metadata #1127

Closed

Best practices for updating a CRD's spec #1128

Closed

Consider using fine grained RBAC for components which interact with Knative CRDs #1131

Closed

google-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2018

bsnchan closed this Nov 14, 2018

grantr mentioned this pull request Nov 15, 2018

Use Status subresource knative/eventing-contrib#110

Merged

Enables status subresource in order to fix generation bumping #786

Enables status subresource in order to fix generation bumping #786

Conversation

bsnchan commented Apr 30, 2018

Proposed Changes

googlebot commented Apr 30, 2018

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Apr 30, 2018

dprotaso commented Apr 30, 2018

evankanderson commented May 1, 2018 via email

dprotaso commented May 1, 2018 • edited Loading

tcnghia left a comment

Choose a reason for hiding this comment

adrcunha commented May 2, 2018

mattmoor commented Jun 4, 2018

google-prow-robot commented Jun 6, 2018

knative-metrics-robot commented Jun 6, 2018

knative-metrics-robot commented Jun 6, 2018

bsnchan commented Jun 6, 2018 • edited Loading

dprotaso commented Jun 11, 2018

evankanderson commented Jun 11, 2018

mattmoor commented Jun 11, 2018

evankanderson commented Jun 11, 2018

bsnchan commented Jun 28, 2018

bsnchan commented Jul 4, 2018

bsnchan commented Jul 4, 2018

bbrowning commented Sep 28, 2018

dprotaso commented Nov 14, 2018

dprotaso commented Nov 14, 2018

dprotaso Apr 30, 2018 •

edited

Loading

dprotaso commented May 1, 2018 •

edited

Loading

bsnchan commented Jun 6, 2018 •

edited

Loading