Subscription Controller basics. by vaikas · Pull Request #437 · knative/eventing

vaikas · 2018-09-19T14:49:00Z

Fixes #438

Proposed Changes

Add the barebones for getting a Subscription Controller wiring things into source channels, and call / sinkables using the duck types.

Release Note

knative-prow-robot · 2018-09-19T14:49:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [vaikas-google]

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

evankanderson · 2018-09-19T19:54:37Z

/assign @neosab , who was also interested in this.

@vaikas-google, I don't know if there is a way that we could split this work or collaborate with @neosab ?

neosab · 2018-09-19T21:33:28Z

-	// target.
-	// +optional
-	Result *ResultStrategy `json:"result,omitempty"`
+	Sinkable string `json:"sinkable"`


I thought the dispatcher acts upon the channel's resolved subscriptions to determine the target DNS [1] to send an event and optionally handle replies depending on whether the target was a Callable. With this change to a single Sinkable, I am failing to understand how the reply logic will be handled by the dispatcher?

[1]

Yes, that's exactly right. But this is really for connecting Subscriptions together, well, really Sources to Channels. Say I have the following:
Source -> Call -> Result
Result is really a channel and if another subscription wants to connect to that channel, the model will look more like this:
Source -> Call -> Result (Now becomes a Source2) -> Call -> Result2
But, we need to be able to support multiple subscriptions to the channel, so it really looks more like this:
Source -> Call -> Result -> SourceChannel -> Call -> Result2
-> SourceChannel2 -> Call -> Result3

In this model, the "Result" Channel will only get SInkable, since they represent "input (or Source)" channels fronting Subscriptions and will allow for Fanout. We do not want the Dispatcher to be handling failures in fanouts.

Does that make more sense?

I've added both entries back there, but in the form of a fully resolved string. I still don't think that Channel should be in the business of resolving refs, etc. but I understand why we probably need both of them here from control plane side of things. I've added that to the pkg/apis repo.

mattmoor · 2018-09-19T20:57:40Z


-// ChannelSubscriberSpec defines a single subscriber to a Channel. At least one
-// of Call or Result must be present.
+// ChannelSubscriberSpec defines a single subscriber to a Channel.


You should add:

var _ = duck.VerifyType(&Channel{}, &duckv1alpha1.Channelable{})

For all implemented interfaces.

vaikas · 2018-09-20T10:35:22Z

@neosab I'd like to get enough in so that we can then keep working in parallel in smaller chunks.

neosab · 2018-09-20T18:29:40Z

@neosab I'd like to get enough in so that we can then keep working in parallel in smaller chunks.

@vaikas-google 👍 Sounds good.

neosab · 2018-09-20T19:17:51Z

-			return apis.ErrMissingField("call", "result").ViaField(fmt.Sprintf("subscriber[%d]", i))
+	if cs.Channelable != nil {
+		for i, subscriber := range cs.Channelable.Subscribers {
+			if subscriber.SinkableDomain == "" {


We should error if both CallableDomain and SinkableDomain are empty

…n-controller

mattmoor · 2018-09-21T19:16:28Z

+		for i, subscriber := range cs.Channelable.Subscribers {
+			if subscriber.SinkableDomain == "" && subscriber.CallableDomain == "" {
+				fe := apis.ErrMissingField("sinkableDomain", "callableDomain")
+				fe.Details = "expected at least one of, got none"


cc @n3wscott I think there's a helper for this?

I couldn't find. I thought I found it but the name was a bit misleading (IMHO). I created one but didn't want to block this on it and instead would prefer to clean it up later.

What about this:

apis.ErrMissingField("sinkableDomain", "callableDomain").ViaField(fmt.Sprintf("subscribers[%d]", i))

There is no helper for adding details to a Err* type method, there is a helper for setting the index.

it should be:

errs = errs.Also(apis.ErrMissingField("sinkableDomain", "callableDomain").ViaFieldIndex("subscribers", i))

grantr

/lgtm

Couple minor issues, but I'm 👍 to get this in to unblock experiments. Thanks @vaikas-google! The dynamic client stuff seems elegant to me.

grantr · 2018-09-21T19:46:14Z

+		for i, subscriber := range cs.Channelable.Subscribers {
+			if subscriber.SinkableDomain == "" && subscriber.CallableDomain == "" {
+				fe := apis.ErrMissingField("sinkableDomain", "callableDomain")
+				fe.Details = "expected at least one of, got none"


What about this:

apis.ErrMissingField("sinkableDomain", "callableDomain").ViaField(fmt.Sprintf("subscribers[%d]", i))

grantr · 2018-09-21T19:50:39Z

+
+	// Subscription might be Subscribable. This depends if there's a Result channel
+	// In that case, this points to that resource.
+	Subscribable duckv1alpha1.Subscribable `json:"subscribable,omitempty"`


IIUC, if a Subscription has a Result Channel, then chained Subscriptions are allowed. It would be possible to create another Subscription that looks like this:

from: ref: kind: Subscription name: some-sub call: // ... result: // ...

Is this the intent?

YES! :) That's exactly right.

YES! That's exactly right, and only if a Subscription has a Result. Just need to fill the status up for that and Result.Status would then point to the resolved ResultChannel

grantr · 2018-09-21T19:51:12Z

 }

+const (
+	// SubscriptionConditionReady has status True when all subcondtions below have been set to True.


nonblocking typo nit: subconditions

grantr · 2018-09-21T19:51:59Z

+
+	// SubscriptionConditionFromReady has status True when controller has successfully added a subscription to From
+	// resource.
+	SubscriptionConditionFromReady duckv1alpha1.ConditionType = "Subscribed"


Why is the name of this FromReady internally but Subscribed externally?

haha, when I was writing code it seemed like a better name. Forgot to change here, will do...

grantr · 2018-09-21T20:05:09Z

+// converge the two. It then updates the Status block of the Subscription resource
+// with the current status of the resource.
+func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) {
+	glog.Infof("Reconciling subscription %v", request)


It can be a future PR, but this should probably use zap.

ack. Let's do in a followup so others can start experimenting / partying on this code as well :)

grantr · 2018-09-21T20:42:36Z

+	return nil
+}
+
+func (r *reconciler) CreateResourceInterface(namespace string, ref *corev1.ObjectReference) (dynamic.ResourceInterface, error) {


Not sure if this is useful, or even how it works, but controller-runtime just gained the ability to work with unstructured objects: kubernetes-sigs/controller-runtime#101

Awesome! Let's investigate on a followup pr, ok?

grantr · 2018-09-21T20:44:21Z

+	// Add our subscription to the object
+	after := c.DeepCopyObject().(*duckv1alpha1.Channel)
+	after.Spec.Channelable = &duckv1alpha1.Channelable{
+		Subscribers: []duckv1alpha1.ChannelSubscriberSpec{{CallableDomain: "foobar"}},


I think this should probably use the given callableDomain and resultDomain

haha, yes :) Didn't get a push out before I had to go get to the plane and I lost wifi.

grantr · 2018-09-21T20:44:57Z

+	return resourceClient.Get(ref.Name, metav1.GetOptions{})
+}
+
+func (r *reconciler) reconcileFromChannel(namespace string, subscribable corev1.ObjectReference, callDomain string, resultDomain string, deleted bool) error {


How is deleted used in this method? Will that be a future enhancement?

yes, added todo.

grantr · 2018-09-21T20:46:42Z

+	return jsonpatch.CreatePatch(rawBefore, rawAfter)
+}
+
+const (


I think all of these constants could be deleted - they don't seem to be used anywhere.

grantr · 2018-09-21T20:49:58Z

+	duckv1alpha1.AddToScheme(scheme.Scheme)
+}
+
+var injectDomainInternalMocks = controllertesting.Mocks{


Thanks @adamharwayne for writing the mock client for this!

…ng accepted

vaikas · 2018-09-22T08:43:26Z

Thanks, sorry for the duplicate comments, sometimes I make a comment, then they seem to disappear just to come back. Hopefully the dupes don't contradict each other at least.
Things work on a real cluster, but the json patch is not working against the fake dynamic client in unit tests so I'll try to keep working on that some more, but if I can't figure it out, perhaps that can be picked up by somebody else so we can get experimentation and other things like channel controllers getting ramped up, provisioners and so forth with this moving forward.

vaikas · 2018-09-22T12:37:10Z

Ok, pushed one more version and added more unit tests and set some rudimentary conditions. If it's not horribly wrong, please merge it and we'll continue after the merge. I'm turning into a pumpkin until Wednesday, but would be nice to see it land before then.

knative-metrics-robot · 2018-09-22T12:37:59Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/eventing/v1alpha1/subscription_defaults.go	0.0%	100.0%	100.0
pkg/apis/eventing/v1alpha1/subscription_types.go	0.0%	100.0%	100.0
pkg/controller/eventing/subscription/provider.go	Do not exist	0.0%
pkg/controller/eventing/subscription/reconcile.go	Do not exist	72.2%

n3wscott · 2018-09-24T16:58:18Z

+	Subscribable duckv1alpha1.Subscribable `json:"subscribable,omitempty"`
 }

+const (


You need to update line 173 with:

var subCondSet = duckv1alpha1.NewLivingConditionSet(SubscriptionConditionReferencesResolved, SubscriptionConditionFromReady)

n3wscott · 2018-09-24T16:59:23Z

+)
+
 // GetSpecJSON returns spec as json
 func (s *Subscription) GetSpecJSON() ([]byte, error) {


you can delete GetSpecJSON now that the duck work has gone in.

n3wscott · 2018-09-24T17:04:21Z


-func isValidResultStrategy(r *ResultStrategy) *apis.FieldError {
-	return isValidObjectReference(*r.Target).ViaField("target")
+func isValidResultStrategy(r ResultStrategy) *apis.FieldError {


FYI, I am taking some of these and moving it to a common place to reuse the logic:

https://github.com/knative/eventing/pull/457/files#diff-96c67291a6d32860d990069d9d9d5cd3

@n3wscott if this PR goes in first, can you fix up the duplication?

n3wscott · 2018-09-24T17:07:14Z

+
+	// SubscriptionConditionFromReady has status True when controller has successfully added a subscription to From
+	// resource.
+	SubscriptionConditionFromReady duckv1alpha1.ConditionType = "FromReady"


After you add these, it is helpful to expose an IsReady method using the Conditions helper to be the judge.

https://github.com/knative/eventing/pull/457/files#diff-67a9c730f0d592911dfa317323fea9e5R131

Also a InitializeConditions

https://github.com/knative/eventing/pull/457/files#diff-67a9c730f0d592911dfa317323fea9e5R137

And then Mark* style methods as you need them.

grantr · 2018-09-24T20:13:31Z


 func (r *reconciler) reconcile(subscription *v1alpha1.Subscription) error {
+	// TODO: Should this just also set up a defer call for subscription.SetConditions.
+	// No time right now as I'm turning into a pumpking but seems reasonable.


I'll switch these to using Mark* methods in a followup PR.

grantr · 2018-09-24T20:15:34Z

 	}
+	// Everything went well, set the fact that subscriptions have been modified
+	conditions = append(conditions, duckv1alpha1.Condition{
+		Type:   duckv1alpha1.ConditionReady,


Was this supposed to be setting ConditionFromReady?

grantr

I've got a followup PR on the way that addresses @n3wscott's comments.

/lgtm

…pagation removing patch, b/c merged upstream

…t. This allows us to mount in custom ca bundles via ConfigMap/VolumeSource. (knative#437) OCP documentation is here: https://docs.openshift.com/container-platform/4.14/networking/configuring-a-custom-pki.html#certificate-injection-using-operators_configuring-a-custom-pki Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 19, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 19, 2018

knative-prow-robot requested review from inlined and mattmoor September 19, 2018 14:49

neosab mentioned this pull request Sep 19, 2018

Round 1: Subscription Controller #438

Closed

neosab reviewed Sep 19, 2018

View reviewed changes

mattmoor reviewed Sep 19, 2018

View reviewed changes

Ville Aikas added 3 commits September 20, 2018 05:30

checkpoint before rebasing

e340872

Updates after the revisions in the pkg repo

bc1bfc3

address more pr comments, clean things up

48774f4

vaikas force-pushed the subscription-controller branch from a194864 to 48774f4 Compare September 20, 2018 10:34

neosab reviewed Sep 20, 2018

View reviewed changes

Ville Aikas added 5 commits September 21, 2018 01:03

Merge branch 'master' of github.com:knative/eventing into subscriptio…

9ab7921

…n-controller

screwed up the merge and left this out by accident

b0eda0e

update tests to work with the new model

2900300

Fix up the unit tests

7fb9c0d

clean up the dynamic client a bit

66c4ca3

mattmoor reviewed Sep 21, 2018

View reviewed changes

grantr approved these changes Sep 21, 2018

View reviewed changes

knative-prow-robot assigned grantr Sep 21, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2018

tests now work with the fake dynamic client but json patch is not bei…

c68633b

…ng accepted

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2018

vaikas changed the title ~~WIP: Subscription Controller~~ Subscription Controller basics. Sep 22, 2018

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 Sep 22, 2018

Ville Aikas added 2 commits September 22, 2018 11:10

Add unit tests covering missing or incorrect references

d53db39

set conditions on happy path only

a219571

n3wscott reviewed Sep 24, 2018

View reviewed changes

grantr reviewed Sep 24, 2018

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2018

grantr approved these changes Sep 24, 2018

View reviewed changes

knative-prow-robot merged commit 1ac89e4 into knative:master Sep 24, 2018

grantr mentioned this pull request Sep 24, 2018

Subscription controller tweaks #459

Merged

n3wscott mentioned this pull request Sep 25, 2018

adding n3wscott to approvers. #465

Merged

vaikas deleted the subscription-controller branch September 26, 2018 06:42

matzew added a commit to matzew/eventing that referenced this pull request Feb 26, 2020

Merge pull request knative#437 from matzew/remove_patch_broker_cm_pro…

4756303

…pagation removing patch, b/c merged upstream

Conversation

vaikas commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot commented Sep 19, 2018

Uh oh!

evankanderson commented Sep 19, 2018

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!

vaikas commented Sep 20, 2018

Uh oh!

neosab commented Sep 20, 2018

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 Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

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!

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!

vaikas commented Sep 22, 2018

Uh oh!

vaikas commented Sep 22, 2018

Uh oh!

vaikas commented Sep 19, 2018 •

edited

Loading

n3wscott Sep 24, 2018 •

edited

Loading

grantr Sep 24, 2018 •

edited

Loading