Knative ConfigMap Propagation by grac3gao-zz · Pull Request #2333 · knative/eventing

grac3gao-zz · 2019-12-18T19:17:37Z

Proposed Changes

This PR implements the design from @grantr in Knative ConfigMap Propagation
It includes two parts: 1) Creating a namespaced CRD ConfigMapPropagation and a corresponding controller to copy ConfigMaps from one namespace to another, 2) Updating Eventing Broker to adapt this CRD

Release Note

lionelvillard · 2019-12-18T21:06:08Z

Is this something Knative Serving also needs?

# Conflicts: # cmd/broker/ingress/main.go # pkg/reconciler/namespace/namespace.go # pkg/reconciler/namespace/namespace_test.go

grac3gao-zz · 2019-12-19T21:32:24Z

ready for review @grantr

yolocs

Good job, Grace! Next time, try to break up your PRs into smaller chunks. E.g. in this case, I would at least have two PRs - one for the new API and one for using the new API. It would be much more review-friendly :)

# Conflicts: # hack/update-codegen.sh

grac3gao-zz · 2019-12-21T00:10:38Z

Done. Ready for review again.

antoineco

Some initial comments. This is a very large PR so I'll need to find more time to review more in depth.

grantr

Fully reviewed now. I have a couple minor comments and some followup issues. Otherwise I think this is good to merge.

grantr · 2020-01-23T23:52:27Z

+	// Tell tracker to reconcile this ConfigMapPropagation whenever an original ConfigMap changes.
+	// Note: Temporarily set Selector to match all objects.
+	// If Selector is set to match specific labels (the required labels from ConfigMapPropagation),
+	// the tracker can't track changes when an original ConfigMap no longer has required labels.


I created an issue to track this: #2435

grantr · 2020-01-23T23:58:26Z

+		Namespace:  cmp.Spec.OriginalNamespace,
+		Selector:   metav1.SetAsLabelSelector(map[string]string{}),
+	}
+	if err := r.tracker.TrackReference(originalConfigMapObjRef, cmp); err != nil {


Since we know the GVK of this object, I'd prefer to use informer event handlers (set up in the constructor in controller.go) instead of the tracker here. I think that's a bit more clear about what watch events will cause the reconciler to run. I created a followup issue to track this: #2436

grantr · 2020-01-24T00:04:54Z

+	// Tell tracker to reconcile this ConfigMapPropagation whenever the a copy ConfigMap changes.
+	// Note: Temporarily set Selector to match all objects.
+	// If Selector is set to match specific labels (the required labels from creation),
+	// the tracker can't track changes when a copy ConfigMap no longer has required labels.


I believe we can replace this with an informer event handler that watches for ConfigMaps owned by CMPs OR with the copy label. The "OR label" clause capture copies that previously had their labels removed for user modifications, then re-added to revert those modifications. I mentioned this in #2435 also.

grantr · 2020-01-24T00:42:19Z

+			//  OwnerReference will be removed when the knative.dev/config-propagation:copy label is not set in copy configmap
+			//  so that this copy configmap will not be deleted if cmp is deleted.
+			expected = current.DeepCopy()
+			expected.OwnerReferences = nil


Here we should only delete the CMP's owner reference. Theoretically other owner references might exist by the time we reconcile. There might be helper methods for this (or if not, you could write one).

I think this is such a rare case that handling it is optional, so I created a followup issue: #2437

grantr · 2020-01-24T01:04:09Z

+				Eventf(corev1.EventTypeNormal, configMapPropagationReadinessChanged, "ConfigMapPropagation %q became ready", configMapPropagationName),
+			},
+		}, {
+			Name: "Copy ConfigMap no longer has required labels",


Related to the comment above and #2437: if we write code to only delete the CMP owner reference, we should add a test case for abandoning a ConfigMap with multiple owner references.

# Conflicts: # cmd/controller/main.go # config/200-addressable-resolvers-clusterrole.yaml # config/200-controller-clusterrole.yaml # config/config-logging.yaml # config/config-observability.yaml # config/config-tracing.yaml # hack/update-codegen.sh # pkg/reconciler/inmemorychannel/dispatcher/controller.go

# Conflicts: # cmd/broker/filter/main.go # cmd/broker/ingress/main.go # hack/update-codegen.sh # pkg/client/informers/externalversions/generic.go

grac3gao-zz · 2020-01-27T22:43:14Z

Thank you for reviewing it and opening subsequent issues! @grantr. I changed api group to be configs.internal.knative.dev. Together with modification according to reviews, ready for review.

mattmoor · 2020-01-28T08:09:09Z

@@ -0,0 +1 @@
+core/resources/configmappropagation.yaml


Add trailing newline:

Suggested change

core/resources/configmappropagation.yaml

core/resources/configmappropagation.yaml

Does symbolic link also need trailing newline?

No, don't edit this file. @mattmoor your bot should probably ignore symlinks

knative-metrics-robot · 2020-01-28T08:13:40Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/configs/v1alpha1/configmappropagation_defaults.go	Do not exist	100.0%
pkg/apis/configs/v1alpha1/configmappropagation_lifecycle.go	Do not exist	100.0%
pkg/apis/configs/v1alpha1/configmappropagation_types.go	Do not exist	100.0%
pkg/apis/configs/v1alpha1/configmappropagation_validation.go	Do not exist	95.8%
pkg/apis/configs/v1alpha1/register.go	Do not exist	100.0%
pkg/reconciler/configmappropagation/configmappropagation.go	Do not exist	89.2%
pkg/reconciler/configmappropagation/controller.go	Do not exist	100.0%
pkg/reconciler/configmappropagation/resources/configmap.go	Do not exist	100.0%
pkg/reconciler/configmappropagation/resources/labels.go	Do not exist	100.0%
pkg/reconciler/namespace/controller.go	94.1%	94.7%	0.6
pkg/reconciler/namespace/namespace.go	79.1%	79.4%	0.3

grantr · 2020-01-28T18:09:34Z

/kind feature
/area observability
/area api

knative-prow-robot · 2020-01-28T18:09:37Z

@grantr: The label(s) kind/feature cannot be applied, because the repository doesn't have them

Details

In response to this:

/kind feature
/area observability
/area api

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

grantr

Thanks @grac3gao! Let's merge this so it can get more testing in the wild.

/lgtm

knative-prow-robot · 2020-01-28T18:12:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grac3gao, grantr

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~~ [grantr]

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

grantr · 2020-01-28T18:14:07Z

/remove-area test-and-release

grac3gao added 4 commits December 6, 2019 14:32

API

d38ca4c

API-2

e95fb2f

API-3

610ac33

Controller

183fbf6

grac3gao-zz requested a review from grantr December 18, 2019 19:17

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 18, 2019

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Dec 18, 2019

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 18, 2019

mattmoor reviewed Dec 18, 2019

View reviewed changes

knative-prow-robot requested review from Harwayne and yt3liu December 18, 2019 19:17

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Dec 18, 2019

grac3gao added 2 commits December 18, 2019 11:28

Controller-2

4f9bf6e

update-codegen

86be672

grac3gao added 3 commits December 18, 2019 13:07

Add unit test

9414727

updating eventing

8f856dd

Merge remote-tracking branch 'upstream/master' into propagation

3164381

# Conflicts: # cmd/broker/ingress/main.go # pkg/reconciler/namespace/namespace.go # pkg/reconciler/namespace/namespace_test.go

nachocano mentioned this pull request Dec 19, 2019

Propagate ConfigMaps changes to data plane objects google/knative-gcp#457

Closed

add unit test & fix conformance test

b8b48b0

grac3gao-zz changed the title ~~[WIP]Knative ConfigMap Propagation~~ Knative ConfigMap Propagation Dec 19, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 19, 2019

yolocs suggested changes Dec 20, 2019

View reviewed changes

yolocs mentioned this pull request Dec 20, 2019

Let pkg also be the home for common knative API and binaries knative/pkg#956

Closed

grac3gao added 3 commits December 20, 2019 15:35

change after review

39ecc30

Merge remote-tracking branch 'upstream/master' into propagation

96a3773

# Conflicts: # hack/update-codegen.sh

update fmt.Errorf

05fe27a

antoineco reviewed Jan 2, 2020

View reviewed changes

grantr mentioned this pull request Jan 23, 2020

ConfigMapPropagation tracks all ConfigMaps in original namespace #2435

Closed

grantr reviewed Jan 24, 2020

View reviewed changes

mattmoor reviewed Jan 25, 2020

View reviewed changes

grac3gao-zz force-pushed the propagation branch from 6b710ba to 18a2832 Compare January 26, 2020 04:11

grac3gao added 5 commits January 27, 2020 11:21

modification

12c2f40

modification

fc09fc6

Merge remote-tracking branch 'upstream/master' into propagation

efdda0f

# Conflicts: # cmd/broker/filter/main.go # cmd/broker/ingress/main.go # hack/update-codegen.sh # pkg/client/informers/externalversions/generic.go

update unit test

bf279c1

change api group name

586f6f0

grantr reviewed Jan 28, 2020

View reviewed changes

Comment thread config/300-configmappropagation.yaml Outdated

add symlink

8ebc2c5

mattmoor reviewed Jan 28, 2020

View reviewed changes

knative-prow-robot added area/observability area/api labels Jan 28, 2020

grantr approved these changes Jan 28, 2020

View reviewed changes

knative-prow-robot assigned grantr Jan 28, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2020

knative-prow-robot removed the area/test-and-release Test infrastructure, tests or release label Jan 28, 2020

knative-prow-robot merged commit 622eafe into knative:master Jan 28, 2020

grantr mentioned this pull request Jan 30, 2020

Update ResolveChannelFromHostFunc to take in a string instead of URL knative/eventing-contrib#876

Merged

grantr mentioned this pull request Feb 7, 2020

Add grantr as Eventing WG lead knative/community#83

Merged

6 tasks

grac3gao-zz deleted the propagation branch March 25, 2020 20:45

		@@ -0,0 +1 @@
		core/resources/configmappropagation.yaml No newline at end of file

Conversation

grac3gao-zz commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

lionelvillard commented Dec 18, 2019

Uh oh!

grac3gao-zz commented Dec 19, 2019

Uh oh!

yolocs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grac3gao-zz commented Dec 21, 2019

Uh oh!

antoineco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grantr 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grac3gao-zz commented Jan 27, 2020

Uh oh!

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!

knative-metrics-robot commented Jan 28, 2020

Uh oh!

grantr commented Jan 28, 2020

Uh oh!

knative-prow-robot commented Jan 28, 2020

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Jan 28, 2020

Uh oh!

grantr commented Jan 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

grac3gao-zz commented Dec 18, 2019 •

edited

Loading