Add ClusterBus status and refactor Bus status by neosab · Pull Request #414 · knative/eventing

neosab · 2018-09-04T21:23:18Z

Proposed Changes

Refactors Bus status update code
Adds status reporting for ClusterBus

TODO: Add tests.

This is an attempt at refactoring the bus status update code, I will add some tests to the PR if this is found reasonable.

Release Note

ClusterBus now reports Status

evankanderson · 2018-09-04T21:27:39Z

/ok-to-test

…

On Tue, Sep 4, 2018 at 2:23 PM Knative Prow Robot ***@***.***> wrote: Hi @neosab <https://github.com/neosab>. Thanks for your PR. I'm waiting for a knative <https://github.com/orgs/knative/people> member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org <https://github.com/orgs/knative/people> to skip this step. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#414 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHlyN3N619Lkoc-zsTHuG1GOtGl-C-G1ks5uXu9WgaJpZM4WZzHH> .

-- Evan Anderson <argent@google.com>

scothis

@neosab Thanks for picking this up.

I left comments inline. Let me know if you have any questions.

scothis · 2018-09-05T00:45:46Z

-			dispatcherDeployment, dispatcherDeplErr,
-			provisionerDeployment, provisionerDeplError)
-		return dispatcherServiceErr
+	err = c.syncDispatcherServiceBusStatus(bus)


While this portion of the syncHandler function is much cleaner, it has significant semantic changes which we don't want.

By moving the status update into the sync sync call for each sub resource, even in the most optimistic case, we'll have three calls to update the Bus resource. Worse, the second call to update will fail because the resource was mutated (by the first update). This means that not only are we making multiple update calls placing a higher burden on all api server and all other watchers of buses, but we're requiring at least three attempts to fully reconcile the bus.

Thanks for the feedback. I was trying to retain the existing control flow but misinterpreted it. Now, the Bus/ClusterBus Status is only updated once during the sync.

scothis · 2018-09-05T00:51:44Z


 	// Sync Service derived from the ClusterBus
-	dispatcherService, err := c.syncClusterBusDispatcherService(clusterBus)
+	err = c.syncDispatcherServiceClusterBusStatus(clusterBus)


The same general comment from the bus controller apply here as well.

scothis · 2018-09-05T19:17:31Z

-			dispatcherDeployment, dispatcherDeplErr,
-			provisionerDeployment, provisionerDeplError)
-		return dispatcherServiceErr
+	err = c.syncDispatcherServiceBusStatus(busCopy)


I'm generally not a fan of passing a struct to a function expecting the function to mutate that struct. Perhaps we should split out adding status to ClusterBus and refactoring the syncHandler function.

My general thought of how to cleanup the syncHandler function is to create a status factory where we can incrementally update the resources and then call update status on that.

Something like:

status := busStatusBuilder{} // Sync Service derived from the Bus status.dispatcherService, status.dispatcherServiceErr = c.syncBusDispatcherService(bus) if err := status.error(); err != nil { _ = status.apply() return err }

What do you think?

My original intent was the cleanup of the updateBusStatus method signature but ended up splitting it into different handlers. I agree it's not nice to mutate the struct. I restored it to what I initially intended, please take a look.

If you still think we need to break up updating the Status, the Builder approach seems reasonable to me.

scothis · 2018-09-06T19:40:34Z

-	var dispatcherService *corev1.Service
-	var dispatcherDeployment, provisionerDeployment *appsv1.Deployment
-	var dispatcherServiceErr, dispatcherDeplErr, provisionerDeplError error
+	err = c.updateBusStatus(bus)


updateBusStatus should only be concerned with updating the status field on the bus resource. With your change it is also syncing deployments and services.

My intention is to do both in the function, I can probably rename it to syncAndUpdateBusStatus or remove the function and add the code into syncHandler itself.

Do you have other recommendations?

the purpose of syncHandler is to sync the actual and desired state for the Bus, so it makes sense that it should delegate the syncing of controlled resources and then collect and update the status for the bus based on the results of the syncing.

Added them under syncHandler

neosab · 2018-09-07T19:45:37Z

/assign @evankanderson

scothis

/lgtm

I ran this locally with a couple different failure cases, seems to work well.

scothis · 2018-09-07T20:18:56Z

 // ClusterBusStatus (computed) for a clusterbus
-type ClusterBusStatus struct {
-}
+type ClusterBusStatus = BusStatus


Bus and ClusterBus may not always have the same status, but the they do at the moment and I don't foresee that changing in the short term.

I see we have this pattern already with ClusterBusSpec but I think we'd want a new type rather than an alias here (e.g. type ClusterBusStatus BusStatus). I'm curious if there's a reason for the alias?

No need to block this PR on that, can fix it later on, but interested to know :)

Not using an alias is a good idea as it will be easier to fork the two types in the future should a distinction arise. Right now someone could do something silly like:

bus := Bus{ Spec: ClusterBusSpec{}, }

I also agree it doesn't need to block this PR.

This was also done to re-use some of the bus_util functions that deals with Status for ClusterBus. I agree ClusterBusStatus can be its own type when it diverges from BusStatus.

@evankanderson

@evankanderson

From [ROLES.MD](https://github.com/knative/docs/blob/dfc53c67c8e80d30b8863353c9e9b4ad00c41fa0/community/ROLES.md#approver): > Reviewer of the codebase for at least 3 months or 50% of project lifetime, whichever is shorter - [First Issue](#80). Opened 6/11 - [First PR](#66). Opened 5/31 - [First Review](#79 (review)) 6/11 > Primary reviewer for at least 10 substantial PRs to the codebase - #422 (review) - #414 (review) - #325 (review) - #225 (review) - #189 (review) - #168 (review) - #165 (review) - #99 (review) - #79 (review) - #111 (review) > Reviewed or merged at least 30 PRs to the codebase - [Reviewed 23 PRs](https://github.com/knative/eventing/pulls?utf8=✓&q=is%3Apr+reviewed-by%3Ascothis) - [Authored 34 merged PRs](https://github.com/knative/eventing/pulls?utf8=✓&q=is%3Apr+author%3Ascothis+is%3Amerged) - [Authored 5 open PRs](https://github.com/knative/eventing/pulls/scothis) > Nominated by an area lead From [WORKING_GROUPS.MD](https://github.com/knative/docs/blob/dfc53c67c8e80d30b8863353c9e9b4ad00c41fa0/community/WORKING-GROUPS.md#events) /assign @vaikas-google > With no objections from other leads 🤞 /cc @evankanderson @grantr @inlined @mattmoor

neosab · 2018-09-12T22:49:37Z

@evankanderson Will this PR be accepted in the light of the new spec? If not, I can close it.

grantr · 2018-09-27T20:40:37Z

/approve

Sorry for the extreme delay @neosab. The community docs state that reviews are expected to be "timely", and we missed that mark here. We'll try to be better about this. Expanding the list of designated approvers should help.

Will this PR be accepted in the light of the new spec? If not, I can close it.

We want to keep the currently documented objects and controllers working while we're building the new ones, so this PR is definitely acceptable.

TODO: Add tests.

Are you still planning to work on tests?

knative-prow-robot · 2018-09-27T20:40:43Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

neosab · 2018-09-27T21:04:26Z

Sorry for the extreme delay @neosab

No issues!

Are you still planning to work on tests?

I will follow up with tests in a new PR

neosab · 2018-09-27T21:05:12Z

/test pull-knative-eventing-integration-tests

@evankanderson

…ve#414) The default 2-minute timeout might not be enough in some scenarios where deploying takes longer (like with Istio).

neosab added 2 commits September 4, 2018 13:47

Refactor bus controller

97cbb95

Add ClusterBus Status

d187be0

knative-prow-robot requested review from grantr and inlined September 4, 2018 21:23

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 4, 2018

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 4, 2018

scothis suggested changes Sep 5, 2018

View reviewed changes

Address review comments

136306c

scothis reviewed Sep 5, 2018

View reviewed changes

neosab added 3 commits September 6, 2018 11:55

Remove multiple sync handlers for bus status

013b692

Restore an unnecessary function name change

75ad097

Update Bus's Copy instead of the actual Bus object

7201196

scothis reviewed Sep 6, 2018

View reviewed changes

Unify login into syncHandler

4743fcc

knative-prow-robot assigned evankanderson Sep 7, 2018

scothis reviewed Sep 7, 2018

View reviewed changes

knative-prow-robot assigned scothis Sep 7, 2018

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

scothis mentioned this pull request Sep 11, 2018

Add scothis as an approver #426

Merged

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

knative-prow-robot merged commit 07b12ab into knative:master Sep 27, 2018

neosab deleted the clusterbus_add_status branch September 27, 2018 21:24

Conversation

neosab commented Sep 4, 2018

Proposed Changes

Uh oh!

evankanderson commented Sep 4, 2018 via email

Uh oh!

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

neosab commented Sep 7, 2018

Uh oh!

scothis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greghaynes Sep 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neosab commented Sep 12, 2018

Uh oh!

grantr commented Sep 27, 2018

Uh oh!

knative-prow-robot commented Sep 27, 2018

Uh oh!

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

Uh oh!

Uh oh!

neosab commented Sep 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

greghaynes Sep 11, 2018 •

edited

Loading

neosab commented Sep 27, 2018 •

edited

Loading