Defaultcontroller by akashrv · Pull Request #843 · knative/eventing

akashrv · 2019-03-02T01:53:30Z

Fixes #779

Proposed Changes

Created a default channel controller. This controller will watch every Channel and set its ProvisionerInstalled condition to false if there is no provisioner that watches the channel
This condition will be set to true by individual controllers when they start watching the Channel
There were concerns about multiple controllers trying to update the status of the same object. However this is not a concern with the current implementation and it seems there are multiple cases in kubernetes where this is done.
The API server Update API support optimistic concurrency based on resource version. When using controller client to update, if the object on server side changes after reading the object and before updating it in the controller, then the update api will fail with CONFLICT error. This is handled by re-queueing the request for reconcile.
I have verified this behavior with current code on a GKE cluster.

Release Note

A new channel-default-controller will monitor every channel. It will check if ClusterChannelProvisioner is installed and that the channel-provisioner-specific controller monitors the channel.
If provisioner is not installed or corrupt and no controller monitors this channel,  then the ProvisionerInstalled condition in status will be set to False and the channel will be marked as not ready.
If the provisoner is installed and monitoring the channel then the ProvisionerInstalled condition in status will be set to True

and update its status in case the channel is not being watched by any controller This could happen if the end user creates a channel but doesn't install the provisioner. Issue#779: knative#779

akashrv · 2019-03-02T01:55:37Z

/assign @n3wscott

akashrv · 2019-03-02T01:56:18Z

/assign @grantr

vaikas · 2019-03-04T09:25:13Z

I'd actually update the Release Notes seciton, since it has user visible changes. Also, we should make sure to update documentation with this information and probably update the debugging also. Not as part of this PR, but we should probably create an issue to track or put out a PR to update those.

…roller Merge from upstream/master and missing dependency

n3wscott

I think there is a clarification that is required around our expectations of the Ready condition and the default provisioner controller. I would not expect it to set Ready at all.

n3wscott · 2019-03-05T16:43:40Z

There is also a question of if this controller should update this status on the normal re-reconcile period if it finds there to be no provisioner. It would sure be interesting to get that set on the channel that worked and then the provisioner went away and now my stuff is broken?

akashrv · 2019-03-05T19:16:59Z

I think there is a clarification that is required around our expectations of the Ready condition and the default provisioner controller. I would not expect it to set Ready at all.

I don't agree with not setting the Ready condition.
Ready/Happy condition is the final condition that tells the user whether the object is ready from control plane perspective. The channel may or may not be ready from a data plane perspective or due to some other resources. The dependent conditions tell the specific reason(s). Also as a user when I query the channel object using kubectl without -o, then it returns the Name, Ready, and Reason. In the case (which is the most likely case) that a Channel is installed but the provisioner was never installed and if I run kubectl get channels/chan then it will return Name:Chan, READY:False, REASON: Provisioner not found.
However Ready will not be set to True till all the dependent conditions are set to True.

I think this is the correct user experience that is expected. Thoughts?

…itialize() function from MarkProvisioned()

n3wscott

I can live with this version 😄 I think there could be a todo added or issue created to follow-up on if we should fetch the provisioner and see if it is having a bad day or not and then propagate it's status up.

/lgtm

knative-metrics-robot · 2019-03-06T19:07:26Z

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
pkg/apis/eventing/v1alpha1/channel_types.go	100.0%	91.7%	-8.3
pkg/reconciler/v1alpha1/channel/channel.go	Do not exist	65.6%

n3wscott · 2019-03-07T01:13:09Z

/lgtm

vaikas · 2019-03-07T07:14:44Z

/lgtm
/approve

knative-prow-robot · 2019-03-07T07:14:51Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

akashrv added 2 commits March 1, 2019 17:40

Creating a default channel controller that will watch every channel

4546b0e

and update its status in case the channel is not being watched by any controller This could happen if the end user creates a channel but doesn't install the provisioner. Issue#779: knative#779

Removed commetned lines

ba80a2e

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

knative-prow-robot requested review from Harwayne and evankanderson March 2, 2019 01:53

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 2, 2019

knative-prow-robot assigned n3wscott Mar 2, 2019

knative-prow-robot assigned grantr Mar 2, 2019

Fixed some typos

8813fc8

vaikas reviewed Mar 4, 2019

View reviewed changes

akashrv added 2 commits March 4, 2019 10:10

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

c5e8bf2

…roller Merge from upstream/master and missing dependency

Changes based on comments on code review (pull request)

9b790f5

akashrv mentioned this pull request Mar 4, 2019

Update the documentation and debugging section with info about new channel-default-controller #848

Closed

n3wscott reviewed Mar 5, 2019

View reviewed changes

akashrv added 2 commits March 5, 2019 15:31

Moved the code to set ChannelConditionProvisionerInstalled=True to In…

fbbdcc0

…itialize() function from MarkProvisioned()

Changed all %v to %s when logging strings

671056b

n3wscott approved these changes Mar 6, 2019

View reviewed changes

Comment thread pkg/apis/eventing/v1alpha1/channel_types.go Outdated

Comment thread pkg/reconciler/v1alpha1/channel/channel_test.go Outdated

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2019

akashrv mentioned this pull request Mar 6, 2019

Propagate the status that a provisioner is down or corrupt to the user. #854

Closed

vaikas reviewed Mar 6, 2019

View reviewed changes

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

Updating some formatting issues called out in PR

6720918

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2019

knative-prow-robot assigned vaikas Mar 7, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2019

knative-prow-robot merged commit 24bcdf8 into knative:master Mar 7, 2019

n3wscott mentioned this pull request Mar 18, 2019

adding n3wscott to approvers. #465

Merged

matzew added a commit to matzew/eventing that referenced this pull request Sep 25, 2020

using oc replace in pipe (knative#843)

ff252b2

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

Conversation

akashrv commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

akashrv commented Mar 2, 2019

Uh oh!

akashrv commented Mar 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vaikas commented Mar 4, 2019

Uh oh!

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

n3wscott commented Mar 5, 2019

Uh oh!

akashrv commented Mar 5, 2019

Uh oh!

n3wscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-metrics-robot commented Mar 6, 2019

Uh oh!

n3wscott commented Mar 7, 2019

Uh oh!

vaikas commented Mar 7, 2019

Uh oh!

knative-prow-robot commented Mar 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

akashrv commented Mar 2, 2019 •

edited

Loading