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.

Sign up for GitHub

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

Jump to bottom

move eventsources out of proc to containers #62

Merged
merged 1 commit into from
May 29, 2018
Merged

move eventsources out of proc to containers #62

merged 1 commit into from
May 29, 2018

Conversation

vaikas
Copy link
Contributor

@vaikas vaikas commented May 29, 2018

Fixes Issue #

Proposed Changes

  • Move Event Sources from running within the controller to their own pods. Make them more 'distribution friendly' so they are self contained and easier to develop and can be done outside the controller.
  • Introduce an event source runner and show how the GCP pubsub handles the long running operations. Same approach should also work for example like legacy source (database / filesystem notifier for example).
  • Remove the receive adapter image hack and introduce parameters to Event Sources.
  • Introduce an event_source_wrapper that makes it easier to write event sources as it fetches the necessary parameters.

@google-prow-robot google-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 29, 2018
@google-prow-robot
Copy link

[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

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

@vaikas vaikas requested a review from grantr May 29, 2018 16:47
@google-prow-robot google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 29, 2018
@vaikas vaikas requested a review from mattmoor May 29, 2018 16:47
grantr
grantr approved these changes May 29, 2018
View reviewed changes
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

/lgtm

TIL about termination message!

pkg/controller/bind/controller.go
Reason: "BindFailed",
Message: msg,
})
glog.Warningf("Couldn't marshal bind context: %+v : %s", bindContext, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't this return err? Should we retry? Seems like it should be Error log level at least. Why was the BindFailed update removed?

pkg/sources/container_event_source.go
glog.Infof("failed to unbind pod: %s", err)
}
// Try to delete the pod anyways
delErr := t.delete(pod)
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about making this a defer?

defer func() {
  if err := t.delete(pod); err != nil {
    glog.Infof("failed to delete the pod after running: %s", err)
  }
}()
if _, err = t.run(pod, false); err != nil {
  glog.Infof("failed to unbind pod: %s", err)
}
return err

Same applies to Bind

pkg/sources/container_event_source.go

// TODO replace with a construct similar to Build by watching for pod
// notifications and use channels for unblocking.
for {
Copy link
Contributor

Choose a reason for hiding this comment

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

https://godoc.org/k8s.io/apimachinery/pkg/util/wait#PollUntil might be useful here

@google-prow-robot google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2018
@google-prow-robot google-prow-robot merged commit e297a9b into knative:master May 29, 2018
@vaikas vaikas deleted the containerize-sources branch June 12, 2018 02:30
@grantr grantr mentioned this pull request Aug 21, 2018
Merged
dprotaso added a commit to dprotaso/eventing that referenced this pull request Sep 12, 2018
@dprotaso
bump knative/pkg to 1e7c2e79b0977d1f6d75e1f8cb75bb7f98ee8e45
aeeb515 
Includes the following changes
1e7c2e7 Update static_watcher.go
d688349 Include note about embedding the ManualWatcher in the InformedWatcher
767d6a5 Few changes to the configmap package
0122abd The test logger will now log the correct caller (knative#63)
e7a4b0d Dont call flag.parse in pkg/test (knative#62)
b213523 Update test-infra dependency (knative#61)
382a2bf Make kube_checks generic so that it can be used in serving (knative#58)
760afb6 cleanup (knative#57)
eedc0a9 Make verify-codegen.sh compatible with OS X (knative#54)
6eff182 Remove docker repo from e2e flags (knative#53)
4be5c07 Vendor the test-infra scripts (knative#52)
8c687df Update WaitForEndpointState to return response (knative#51)
8f6a3be Update knative/pkg/test (knative#50)
3ca4270 Add a logkey for the reconcile key. (knative#49)
62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (knative#43)
f4a77d7 Add a common test clients file (knative#46)
450739d Add common test logging module (knative#45)
dprotaso added a commit to dprotaso/eventing that referenced this pull request Sep 12, 2018
@dprotaso
bump knative/pkg to 8fc80de
7d1b2b6 
Includes the following changes:
8fc80de Few changes to the configmap package (knative#59)
0122abd The test logger will now log the correct caller (knative#63)
e7a4b0d Dont call flag.parse in pkg/test (knative#62)
b213523 Update test-infra dependency (knative#61)
382a2bf Make kube_checks generic so that it can be used in serving (knative#58)
760afb6 cleanup (knative#57)
eedc0a9 Make verify-codegen.sh compatible with OS X (knative#54)
6eff182 Remove docker repo from e2e flags (knative#53)
4be5c07 Vendor the test-infra scripts (knative#52)
8c687df Update WaitForEndpointState to return response (knative#51)
8f6a3be Update knative/pkg/test (knative#50)
3ca4270 Add a logkey for the reconcile key. (knative#49)
62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (knative#43)
f4a77d7 Add a common test clients file (knative#46)
450739d Add common test logging module (knative#45
knative-prow-robot pushed a commit that referenced this pull request Sep 17, 2018
@dprotaso @knative-prow-robot
Bump knative/pkg to 8fc80de (#428)
d2586db 
* bump knative/pkg to 8fc80de

Includes the following changes:
8fc80de Few changes to the configmap package (#59)
0122abd The test logger will now log the correct caller (#63)
e7a4b0d Dont call flag.parse in pkg/test (#62)
b213523 Update test-infra dependency (#61)
382a2bf Make kube_checks generic so that it can be used in serving (#58)
760afb6 cleanup (#57)
eedc0a9 Make verify-codegen.sh compatible with OS X (#54)
6eff182 Remove docker repo from e2e flags (#53)
4be5c07 Vendor the test-infra scripts (#52)
8c687df Update WaitForEndpointState to return response (#51)
8f6a3be Update knative/pkg/test (#50)
3ca4270 Add a logkey for the reconcile key. (#49)
62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (#43)
f4a77d7 Add a common test clients file (#46)
450739d Add common test logging module (#45

* Use updated methods that indicate that the configmap.Watcher is using an informer
matzew pushed a commit to matzew/eventing that referenced this pull request Apr 17, 2019
@openshift-merge-robot
Merge pull request knative#62 from matzew/improve_release_resolver
2e1bd3c 
Improving resolver
matzew added a commit to matzew/eventing that referenced this pull request Feb 9, 2023
@matzew @knative-prow-robot @mgencur
[release-1.7] Properly export trace for duplicated "finished" event (k…
0ce4796 
…native#6540) (knative#62)

* Properly export trace for duplicated "finished" event

Fixes an issue with duplicated finished event in which case the
following error was thrown:
knative.dev/eventing/test/upgrade/prober.(*prober).getStepNoFromMsg
...
panic: runtime error: index out of range [1] with length 0

* Define FinishedEventPrefix and reuse it

Co-authored-by: Martin Gencur <mgencur@redhat.com>

Co-authored-by: Knative Prow Robot <knative-prow-robot@google.com>
Co-authored-by: Martin Gencur <mgencur@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grantr grantr grantr approved these changes

@mattmoor mattmoor Awaiting requested review from mattmoor

Assignees

@grantr grantr

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vaikas @google-prow-robot @grantr