Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bus monitor polish #382

Merged
merged 7 commits into from
Aug 28, 2018
Merged

Conversation

scothis
Copy link
Contributor

@scothis scothis commented Aug 17, 2018

General bus enhancements lifted from #378, since that PR is blocked.

  • avoid unnessesary status updates if the status has not changed
  • avoid supressing errors when updating status
  • avoid supressing updates for unchanged resources as the local copy may not be fully synched
  • addressed all govet and golint reports from pkg/buses

/assign @n3wscott

@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 17, 2018
@@ -105,6 +112,8 @@ func (b *PubSubBus) CreateOrUpdateSubscription(sub *channelsv1alpha1.Subscriptio
return err
}

// DeleteSubscription removes a Subscription from Cloud Pub/Sub for a Knative
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet this is a bug based on the CreateOrUpdate comment: or idempotently updates a Subscription if it already exists. which means it will delete adopted topics where we don't mean to. We should be adding something to the metadata that states if we did real work on the remote topic/subscription or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buses should be in total control of their infrastructure, the subscription is prefixed to avoid collisions with a non bus-created resources. While collisions are possible (need to fix #253), it should be rare and is certainly not a feature.

@@ -122,6 +131,8 @@ func (b *PubSubBus) DeleteSubscription(sub *channelsv1alpha1.Subscription) error
return subscription.Delete(ctx)
}

// SendEventToTopic sends a message to the Cloud Pub/Sub Topic backing the
// Channel.
func (b *PubSubBus) SendEventToTopic(channel *channelsv1alpha1.Channel, message *buses.Message) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this suppose to be a public method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

// ReceiveEvents will receive events from a Cloud Pub/Sub Subscription for a
// Knative Subscription. This method will not block, but will continue to
// receive events until either this method or StopReceiveEvents is called for
// the same Subscription.
func (b *PubSubBus) ReceiveEvents(sub *channelsv1alpha1.Subscription, parameters buses.ResolvedParameters) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this suppose to be a public method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

@n3wscott
Copy link
Contributor

/lgtm

Minor comments mostly not related to this change but to the code in general.

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2018
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Letting @n3wscott finish reviewing and lgtm)

sInitial := parameters[InitialOffset]
if sInitial == Oldest {
func resolveInitialOffset(parameters buses.ResolvedParameters) (int64, error) {
sInitial := parameters[initialOffset]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch or a map would be slightly cleaner here; i.e.

const saramaOffset = map[string]int64{
    "newest": saramaNewest,
    "oldest": saramaOldest,
}

func resolveInitialOffset(...) (int64, error) {
  offset, ok := saramaOffset[parameters[initialOffset]
  if !ok {
    return 0, fmt.Errorf(...)
  }
  return offset, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched to a switch

glog.Warningf("Could not update status: %v", errS)
if !equality.Semantic.DeepEqual(subscription.Status, subscriptionCopy.Status) {
// status has changed, update subscription
_, errS := monitor.eventingClient.ChannelsV1alpha1().Subscriptions(subscription.Namespace).Update(subscriptionCopy)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels strange to be tracking two errors here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a bit funky. We want always want to update the resource status, if the original err being recorded in the resource status, we want to return that error since it's more meaningful. If the update failed, we still want to return that error to trigger the work queue to process the resource again as the update can fail because of a failed optimistic lock.

Will add a comment in the code

func (r *BusReference) String() string {
return fmt.Sprintf("%s/%s", r.Namespace, r.Name)
if len(r.Namespace) != 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight preference for r.Namespace != ""; it makes bugs like #377 slightly easier to catch, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Clarify and solidify the public interfaces for a Bus. Common behavior
has moved into composable units that can individually tests or injected
into the bus for testing.

Bus implementors generally only need to provide custom implementations
for up to six methods defined by EventHandlerFuncs.

Core components:
- [Bus|Channel|Subscription]Reference: decouples bus from the raw
    k8s resources
- Cache: lookup and presist raw resource types for a reference
- EventHandlerFuncs: the contract for bus implementations
- Reconciler: reads resource changes from the API server, saves the
    resource in the Cache and emits events to the EventHandlerFuncs
- MessageDispatcher: sends messages to a destination over the event
    delivery protocol
- MessageReceiver: receive messages for a channel on the bus over the
    event delivery protocol

Behavior changes:
- avoid unnessesary status updates if the status has not changed
- avoid supressing errors when updating status
- avoid supressing updates for unchanged resources as the local copy may
  not be fully reconciled
@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2018
@scothis
Copy link
Contributor Author

scothis commented Aug 22, 2018

@n3wscott @evankanderson PTAL

I addressed your feedback and also pulled in the other refactoring I've been working on. The diff is rather scary, but mostly is moving items around to create components that are similar, more cleanly defined and testable. There's more to do (especially adding tests and polishing rough spots), but I like the general direction and it's a better foundation.

From the commit message:

Clarify and solidify the public interfaces for a Bus. Common behavior
has moved into composable units that can individually tests or injected
into the bus for testing.

Bus implementors generally only need to provide custom implementations
for up to six methods defined by EventHandlerFuncs.

Core components:
- [Bus|Channel|Subscription]Reference: decouples bus from the raw
    k8s resources
- Cache: lookup and presist raw resource types for a reference
- EventHandlerFuncs: the contract for bus implementations
- Reconciler: reads resource changes from the API server, saves the
    resource in the Cache and emits events to the EventHandlerFuncs
- MessageDispatcher: sends messages to a destination over the event
    delivery protocol
- MessageReceiver: receive messages for a channel on the bus over the
    event delivery protocol

Behavior changes:
- avoid unnessesary status updates if the status has not changed
- avoid supressing errors when updating status
- avoid supressing updates for unchanged resources as the local copy may
  not be fully reconciled

Copy link
Contributor

@n3wscott n3wscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there are new directories, can you confirm there are new tests in each new directory?

pkg/buses/bus.go Outdated
// EventHandlerFuncs are used to be notified when a subscription is created,
// updated or removed, or a message is received.
func NewBusDispatcher(busRef BusReference, handlerFuncs EventHandlerFuncs, opts *BusOpts) BusDispatcher {
bus := &bus{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would invert this method to have bus := $bus{...} after the opts validation has happened, and then that condenses the calls like bus.cache = opts.Cache into just setting inside the struct creation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do, although it's not quite as clean as you'd sugest because there's a cyclic dependency between the bus and the receiver I still need to untangle.

pkg/buses/bus.go Outdated
handlerFuncs: handlerFuncs,
}

if opts.Cache == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opts is allowed to be nil, so you should wrap all the defaulting with a nil check on opts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

func (b *bus) dispatchMessage(subscription *channelsv1alpha1.Subscription, message *Message) error {
subscriber := subscription.Spec.Subscriber
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can subscription be nil here? should we check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a reason to why it should be allowed to be empty. I'll add a check to the validator.

// AddChannel adds, or updates, the provided channel to the cache for later
// retrieal by its reference.
func (c *Cache) AddChannel(channel *channelsv1alpha1.Channel) {
channelRef := NewChannelReference(channel)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nil check channel? return error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding a check and test


// RemoveChannel removes the provided channel from the cache.
func (c *Cache) RemoveChannel(channel *channelsv1alpha1.Channel) {
channelRef := NewChannelReference(channel)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nil check channel? return error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding a check and test

return nil
}

glog.Infof("Delete topic %q\n", topicID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this refactor is the correct time to move to zap or just plain log?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but in a follow-up PR

return err
} else if exists {
// TODO update subscription configuration
// _, err := subscription.Update(b.ctx, pubsub.SubscriptionConfigToUpdate{})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems important?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not important right now because there are no bus params, thus the pub/sub subscription will never need to be mutated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amending the current comment to make it more clear

err = h.ProvisionFunc(channelRef, parameters)
channelCopy := channel.DeepCopy()
var cond *channelsv1alpha1.ChannelCondition
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to see the if error != nil block directly after the setting of the involved err variable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case you could use the

if err := h.ProvisionFunc(channelRef, parameters); err != nil {
...
} else {
...
}

style

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i see, you use this error much further down as well...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updating

_, errS := reconciler.eventingClient.ChannelsV1alpha1().Channels(channel.Namespace).Update(channelCopy)
if errS != nil {
glog.Warningf("Could not update channel status: %v", errS)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is pretty strange. It feels real weird to have two errors and not report both of them. Maybe smush them together as a string in a new error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will smoosh them into a single error

func (h EventHandlerFuncs) onUnprovision(channel *channelsv1alpha1.Channel, reconciler *Reconciler) error {
if h.UnprovisionFunc != nil {
channelRef := NewChannelReference(channel)
err := h.UnprovisionFunc(channelRef)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general style would have me changing this to:

		if err := h.UnprovisionFunc(channelRef); err != nil {
			reconciler.RecordChannelEventf(channelRef, corev1.EventTypeWarning, errResourceSync, "Error unprovisioning channel: %s", err)
			// skip updating status conditions since the channel was deleted
			return err
		} else {
			reconciler.RecordChannelEventf(channelRef, corev1.EventTypeNormal, successSynced, "Channel unprovisioned successfully")
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updating

@scothis
Copy link
Contributor Author

scothis commented Aug 23, 2018

@n3wscott PTAL

@n3wscott
Copy link
Contributor

/lgtm

I think this is a really nice refactoring.

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2018
@scothis
Copy link
Contributor Author

scothis commented Aug 24, 2018

for approval:
/assign @evankanderson

Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

}

func (h EventHandlerFuncs) onSubscribe(subscription *channelsv1alpha1.Subscription, reconciler *Reconciler) error {
if h.SubscribeFunc != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, convert this to early-return:

if h.SubscribeFunc == nil {
 return nil
}

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, scothis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2018
- return early from handlerfuncs if func is nil
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2018
@scothis
Copy link
Contributor Author

scothis commented Aug 28, 2018

@evankanderson PTAL. I resolved the conflict with master and addressed your comment

@knative-metrics-robot
Copy link

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/channels/v1alpha1/subscription_validation.go 80.0% 84.2% 4.2
pkg/buses/bus.go Do not exist 0.0%
pkg/buses/cache.go Do not exist 100.0%
pkg/buses/gcppubsub/bus.go Do not exist 0.0%
pkg/buses/handler_funcs.go Do not exist 0.0%
pkg/buses/kafka/bus.go Do not exist 0.0%
pkg/buses/reconciler.go Do not exist 0.0%
pkg/buses/references.go 0.0% 100.0% 100.0
pkg/buses/stub/bus.go Do not exist 0.0%

@n3wscott
Copy link
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2018
@knative-prow-robot knative-prow-robot merged commit e9f182a into knative:master Aug 28, 2018
scothis added a commit to scothis/eventing that referenced this pull request Aug 30, 2018
knative-prow-robot pushed a commit that referenced this pull request Sep 11, 2018
matzew added a commit to matzew/eventing that referenced this pull request Jan 9, 2020
Adding back the incorrectly removed serving/servrless content
matzew pushed a commit to matzew/eventing that referenced this pull request Oct 24, 2023
* Apply vendor patches on make generate-release

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

* Align create-release-branch

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

---------

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
matzew added a commit to matzew/eventing that referenced this pull request Oct 24, 2023
…native#394)

* Apply vendor patches on make generate-release



* Align create-release-branch



---------

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>
mgencur pushed a commit to mgencur/eventing that referenced this pull request Nov 3, 2023
* Apply vendor patches on make generate-release

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

* Align create-release-branch

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

---------

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants