Reconcile using pkg/controller by n3wscott · Pull Request #1057 · knative/eventing

n3wscott · 2019-04-15T23:37:59Z

Relates to #734

Proposed Changes

Move to pkg/controller style for eventing core.

Release Note

- channel.spec.subscribable.subscribers[n]**.ref** changes to **.uid** for patchMergeKey,
- SubscriberSpec Struct .Ref becomes .DeprecatedRef

n3wscott · 2019-04-15T23:39:04Z

/cc @mattmoor

n3wscott · 2019-04-15T23:42:19Z

/cc @vaikas-google

vaikas

I feel like we should document the change from ref to uid in the release notes section for sure? I assume there's a way to find the subscription given the URI alone? I'll go look at those next, just jotting it down though.

vaikas · 2019-04-17T20:09:44Z

+	eventingInformerFactory := informers.NewSharedInformerFactory(opt.EventingClientSet, opt.ResyncPeriod)
+
+	subscriptionInformer := eventingInformerFactory.Eventing().V1alpha1().Subscriptions()
+	//deploymentInformer := kubeInformerFactory.Apps().V1().Deployments()


I want to keep them until I am finished with the the other rec loops.

vaikas · 2019-04-17T20:10:30Z

+		logger.Fatalw("Error building kubeconfig", zap.Error(err))
+	}
+
+	const numControllers = 1


why not define this with the other constants around L72?

I am copying patterns from serving, I think because this code knows how many reconcilers it is adding right here.

Why can't we determine the qps values based on the size of the controller array?

I was going to suggest that as well, but these seem to be necessary before the controller array is defined. Then I was going to suggest that we should document below in the controller array creation, but then I noticed that the code will panic if you add a controller and forget to modify controller const here. So I'm fine with this.

This is a copy from serving. We can improve on it later. I would suggest not changing it until we do more integrations.

vaikas · 2019-04-17T20:11:35Z

+	}
+
+	// Watch the logging config map and dynamically update logging levels.
+	opt.ConfigMapWatcher.Watch(logging.ConfigMapName(), logging.UpdateLevelFromConfigMap(logger, atomicLevel, component))


this uses logging.ConfigMapName() but L177 uses logconfig.ConfigMapName() is that intentional? If so, perhaps add comments about that. Just seems goofy.

The two impls are split between the CR way and pkg way. The other path will be deleted when it is removed

vaikas · 2019-04-17T20:53:01Z

 type ChannelSubscriberSpec struct {
 	// +optional
-	Ref *corev1.ObjectReference `json:"ref,omitempty"`
+	UID types.UID `json:"uid,omitempty"`


So, these are Subscription UIDs? Just trying to think what ramifications this change has.

This is ONLY for patchMerge to work correctly.

Is there an upgrade plan that allows existing channels to continue to work? What about partners who are already relying on the ref field?

Just trying to think what ramifications this change has.

Doesn't this eliminate the ability for Channel provisioners to retrieve a list of their Subscription objects? Is there a way to retrieve an object by UID only in the Kubernetes api?

No upgrade plan needed, it just works.

Channel provisioners to retrieve a list of their Subscription objects

this is not done as far as I know and not part of the subscribable spec.

Yeah, this was what I was curious about also, does anybody else need to retrieve the subscription object. @n3wscott told me no, the ref here was only used for merging purposes.

I added back Ref as DeprecatedRef

n3wscott · 2019-04-17T21:01:19Z

/hold
last thing to do is to let the controllers run together.

n3wscott · 2019-04-17T23:30:32Z

/test pull-knative-eventing-integration-tests

vaikas · 2019-04-17T23:12:21Z

+		logger.Fatalw("Error building kubeconfig", zap.Error(err))
+	}
+
+	const numControllers = 1


I was going to suggest that as well, but these seem to be necessary before the controller array is defined. Then I was going to suggest that we should document below in the controller array creation, but then I noticed that the code will panic if you add a controller and forget to modify controller const here. So I'm fine with this.

vaikas · 2019-04-17T23:23:13Z

 type ChannelSubscriberSpec struct {
 	// +optional
-	Ref *corev1.ObjectReference `json:"ref,omitempty"`
+	UID types.UID `json:"uid,omitempty"`


Yeah, this was what I was curious about also, does anybody else need to retrieve the subscription object. @n3wscott told me no, the ref here was only used for merging purposes.

knative-metrics-robot · 2019-04-18T18:00:56Z

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

File	Old Coverage	New Coverage	Delta
contrib/kafka/pkg/dispatcher/dispatcher.go	71.8%	73.6%	1.8
contrib/natss/pkg/dispatcher/dispatcher/dispatcher.go	57.1%	56.3%	-0.8

vaikas · 2019-04-18T18:04:53Z

/approve
/lgtm
/hold

Thanks so much for doing this, really stoked about this.

Just putting the /hold since @grantr had some outstanding comments so don't want to merge without him getting a chance to veto :)

grantr · 2019-04-18T18:39:19Z

 					Authority: fmt.Sprintf("%s.%s.channels.%s", channelName, testNS, utils.GetClusterDomainName()),
 				},
-				Route: []istiov1alpha3.DestinationWeight{{
+				Route: []istiov1alpha3.HTTPRouteDestination{{


Why did this change? Is this an integration from one of the istio PRs?

This is a pkg change.

grantr · 2019-04-18T18:40:46Z

 					Authority: fmt.Sprintf("%s.%s.channels.%s", channelName, testNS, utils.GetClusterDomainName()),
 				},
-				Route: []istiov1alpha3.DestinationWeight{{
+				Route: []istiov1alpha3.HTTPRouteDestination{{


Same as above - why did this change?

pkg changed it.

grantr · 2019-04-18T18:42:23Z

 		}
 		s.logger.Sugar().Infof("NATSS message received from subject: %v; sequence: %v; timestamp: %v, headers: '%s'", msg.Subject, msg.Sequence, msg.Timestamp, message.Headers)
-		if err := s.dispatcher.DispatchMessage(&message, subscription.SubscriberURI, subscription.ReplyURI, provisioners.DispatchDefaults{Namespace: subscription.Namespace}); err != nil {
+		if err := s.dispatcher.DispatchMessage(&message, subscription.SubscriberURI, subscription.ReplyURI, provisioners.DispatchDefaults{Namespace: channel.Namespace}); err != nil {


Was this a bug? I guess they're always the same namespace in practice, so hard to see.

yes it was a bug

grantr · 2019-04-18T18:44:53Z

 					Authority: channelHostName(channel.Name, channel.Namespace),
 				},
-				Route: []istiov1alpha3.DestinationWeight{{
+				Route: []istiov1alpha3.HTTPRouteDestination{{


Same as above

pkg updated the names to the real names

grantr · 2019-04-18T18:44:58Z

 					Authority: fmt.Sprintf("%s.%s.channels.%s", channelName, testNS, utils.GetClusterDomainName()),
 				},
-				Route: []istiov1alpha3.DestinationWeight{{
+				Route: []istiov1alpha3.HTTPRouteDestination{{


Same as above

grantr · 2019-04-18T19:01:08Z

-			if equality.Semantic.DeepEqual(sub.Spec.Channel, s.Spec.Channel) {
-				subs = append(subs, s)
-			}
+	sl, err := r.subscriptionLister.Subscriptions(sub.Namespace).List(labels.Everything()) // TODO: we can use labels to help here


For future reference, the continue token handling can be removed based on #1044 (comment).

grantr · 2019-04-18T19:02:47Z

+			Name: "key not found",
+			// Make sure Reconcile handles good keys that don't exist.
+			Key: "foo/not-found",
+			//}, { // TODO: there is a bug in the controller, it will query for ""


Is this a new test you wrote @n3wscott? Good catch!

no, it is an existing test that was blind to what the reconciler was really doing.

grantr · 2019-04-18T19:03:41Z

+import (
+	"encoding/json"
+	"fmt"
+	testing2 "github.com/knative/eventing/pkg/reconciler/testing"


Can we call this import something more descriptive than testing2? I suggest rectesting

as a followup?

grantr · 2019-04-18T19:08:30Z

+		}, {
+			Name: "subscription, but subscriber does not exist",
+			Objects: []runtime.Object{
+				testing2.NewSubscription(subscriptionName, testNS,


Can you explain why you replaced the previous builder syntax with the functional options syntax?

serving uses mutation functions and go tends to not use builders. The functions are more flexible.

grantr · 2019-04-18T19:09:38Z

@@ -14,30 +14,26 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package v1alpha1


lol this is a weird diff

I think it is git being funny, it is a new file

yeah it's accidentally a rename because git can only detect renames by looking at file similarity.

grantr

Big change! Thanks for working through all this @n3wscott.

/lgtm

knative-prow-robot · 2019-04-18T20:20:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, n3wscott, vaikas-google

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,n3wscott,vaikas-google]

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

grantr · 2019-04-18T20:21:04Z

/hold cancel

n3wscott · 2019-04-18T20:25:24Z

🎉

Co-authored-by: Stavros Kontopoulos <skontopo@redhat.com>

…5 updates (knative#1057) * [release-v1.17][gomod]: Bump the patch group across 1 directory with 5 updates Bumps the patch group with 2 updates in the / directory: [k8s.io/api](https://github.com/kubernetes/api) and [k8s.io/apiextensions-apiserver](https://github.com/kubernetes/apiextensions-apiserver). Updates `k8s.io/api` from 0.31.4 to 0.31.5 - [Commits](kubernetes/api@v0.31.4...v0.31.5) Updates `k8s.io/apiextensions-apiserver` from 0.31.4 to 0.31.5 - [Release notes](https://github.com/kubernetes/apiextensions-apiserver/releases) - [Commits](kubernetes/apiextensions-apiserver@v0.31.4...v0.31.5) Updates `k8s.io/apimachinery` from 0.31.4 to 0.31.5 - [Commits](kubernetes/apimachinery@v0.31.4...v0.31.5) Updates `k8s.io/apiserver` from 0.31.4 to 0.31.5 - [Commits](kubernetes/apiserver@v0.31.4...v0.31.5) Updates `k8s.io/client-go` from 0.31.4 to 0.31.5 - [Changelog](https://github.com/kubernetes/client-go/blob/master/CHANGELOG.md) - [Commits](kubernetes/client-go@v0.31.4...v0.31.5) --- updated-dependencies: - dependency-name: k8s.io/api dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch - dependency-name: k8s.io/apiextensions-apiserver dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch - dependency-name: k8s.io/apimachinery dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch - dependency-name: k8s.io/apiserver dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch - dependency-name: k8s.io/client-go dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch ... Signed-off-by: dependabot[bot] <support@github.com> * Run generate release --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Scott Nichols added 2 commits April 15, 2019 16:20

Move subscription controller to pkg/controller.

c694125

fix finalizers.,

975b012

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 Apr 15, 2019

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

knative-prow-robot requested review from Harwayne and grantr April 15, 2019 23:38

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 15, 2019

knative-prow-robot requested a review from mattmoor April 15, 2019 23:39

Merge branch 'master' into reconcile_pkg

d216ba3

knative-prow-robot requested a review from vaikas April 15, 2019 23:42

Scott Nichols added 11 commits April 16, 2019 08:02

merge main funcs.

d16a942

need to shuffle some stuff.

34ad19c

Use pkg rec/testing.

4608b8b

oh man testing testing testing.

a5ae2c7

update deps.

c3400b7

update deps.

1bd482d

update deps.

2af9067

Found a bug in patchMergeKey for type struct, it has to be a key.

1ea41a4

clean up, adding final test.

c9dafbb

add pkg controller to main.

556f65b

fix up contrib.

87b527b

n3wscott changed the title ~~[WIP] Reconcile using pkg/controller~~ Reconcile using pkg/controller Apr 17, 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 Apr 17, 2019

vaikas reviewed Apr 17, 2019

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2019

Fix main.

cbfbc27

vaikas reviewed Apr 17, 2019

View reviewed changes

Scott Nichols added 4 commits April 18, 2019 08:37

adding back ref as deprecated ref.

bacab9a

update codegen.

7d11b12

remove untrue comment.

474c876

use subscription in comment.

6355748

n3wscott force-pushed the reconcile_pkg branch from e2af243 to 6355748 Compare April 18, 2019 17:09

vaikas reviewed Apr 18, 2019

View reviewed changes

Comment thread pkg/reconciler/v1alpha1/subscription/subscription.go

Scott Nichols added 3 commits April 18, 2019 10:33

don't use testing alias, move reconciler out of v1alpha1.

b377902

use logconfig.

8de7b10

update deps.

5e58632

knative-prow-robot assigned vaikas Apr 18, 2019

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Apr 18, 2019

grantr reviewed Apr 18, 2019

View reviewed changes

grantr mentioned this pull request Apr 18, 2019

Rename testing2 import in subscription tests #1068

Closed

knative-prow-robot assigned grantr Apr 18, 2019

grantr approved these changes Apr 18, 2019

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 18, 2019

knative-prow-robot merged commit 83b079f into knative:master Apr 18, 2019

n3wscott mentioned this pull request Apr 22, 2019

[BUG] Subscription Reconciler bugs found while migrating. #1081

Closed

matzew mentioned this pull request Nov 28, 2019

Use Namespace and name on kafka consumer group knative/eventing-contrib#762

Merged

grantr mentioned this pull request Feb 7, 2020

Add grantr as Eventing WG lead knative/community#83

Merged

6 tasks

matzew added a commit to matzew/eventing that referenced this pull request Mar 26, 2021

add prom host (knative#4661) (knative#1057)

6ffdd29

Co-authored-by: Stavros Kontopoulos <skontopo@redhat.com>

Conversation

n3wscott commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

n3wscott commented Apr 15, 2019

Uh oh!

n3wscott commented Apr 15, 2019

Uh oh!

vaikas left a comment

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

n3wscott commented Apr 17, 2019

Uh oh!

n3wscott commented Apr 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-metrics-robot commented Apr 18, 2019

Uh oh!

vaikas commented Apr 18, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Apr 15, 2019 •

edited

Loading