move apiserver source to eventing by lionelvillard · Pull Request #1108 · knative/eventing

lionelvillard · 2019-04-26T15:38:08Z

Proposed Changes

Move Apiserver source eventing, in pkg/[adapter,reconciler]

knative-prow-robot · 2019-04-26T15:38:23Z

Hi @lionelvillard. Thanks for your PR.

I'm waiting for a knative 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 to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vaikas

Thanks for doing this! Super useful!!!

vaikas · 2019-04-26T16:41:59Z

+          type: object
+        spec:
+          properties:
+            serviceAccountName:


Can we add a "description" to all these fields? Most of the existing ones don't specify them, but we should so that you get a more descriptive message what these fields are. This can be done as a follow on, just jotting it down as I see them.

vaikas · 2019-04-26T16:44:51Z

+	for {
+		stri := strconv.Itoa(i)
+
+		apiVersion, defined := os.LookupEnv(envAPIVersion + stri)


I'd use this package for env variables.
"github.com/kelseyhightower/envconfig"

You can see a use here:
https://github.com/vaikas-google/twitter/blob/master/cmd/source/main.go#L26

Filing an issue and doing in a follow up is totes fine. If you do this, please tag it as good first issue.

vaikas · 2019-04-26T16:49:08Z

+
+	// OwnerReferences is the list of dependents to watch.
+	// Only APIVersion and Kind are used. The other fields are ignored.
+	OwnerReferences []metav1.OwnerReference `json:"ownerReferences"`


Why is this OwnerReference instead of ObjectReference?

Because of the Controller field in OwnerReference. It's not currently used but eventually it will.

vaikas · 2019-04-26T16:54:49Z

+
+	"github.com/knative/eventing/pkg/adapter/apiserver"
+	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
+	"sigs.k8s.io/controller-runtime/pkg/client/config"


We're getting rid of the controller-runtime, so this should really be using the knative/pkg controller. I'd really prefer to do this before merging so we don't then have to migrate it later. Thoughts?

I thought I could get away with this since this belongs to the receiver only. I'm fine removing the dependency on controller-runtime and migrate to pkg/controller

Out of curiosity, what the rationale behind the decision of getting rid of controller-runtime?

vaikas · 2019-04-26T16:56:38Z

+		return err
+	}
+
+	// Create one controller per watch


Why are we creating controllers for everything and not just create cache informers for each one that we care about? handler is all the same?

the handler state is not the same: the gvk is different. Maybe by using pkg/controller we can use one controller.

I was trying to say that we do not need a controller at all. You can just construct a duck type that watches for duck type that's type/objectmeta and just register handlers for each of them. So, in your main you'd construct a kubeclient (like the k8s watcher does today), then create a dynamic client:

"k8s.io/client-go/dynamic"
dynamicClient, err := dynamic.NewForConfig(cfg)

Then for each type that you need, you'd create one of these:
tif := &duck.TypedInformerFactory{
Client: dynamicClient,
Type: &<DUCK_TYPE_FOR_ALL_OBJECTS>,
ResyncPeriod: resyncPeriod,
StopChannel: stopCh,
}

for each of the watched object gvk {
gvr, _ := meta.UnsafeGuessKindToResource(gvk)
informer, lister, err := psif.Get(gvr)

informer.AddEventHandler(AddFunc: messageSender, UpdateFunc: messageSender, DeleteFunc: messageSender)

}

We still need to create a controller in order to process multiple events concurrently. processorListener runs in one go routine only. Am I missing something?

I think we need at least one deployment per service account. It could be something we optimize later.

yes I agree

Ok I understand what Ville is saying a bit better now. The issue here is there is no need for the controller. This can just be a kube client and a dynamic.Interface that sets up duck.TypedInformerFactory

Also, don't use processorListener, use the informer cache.

I'm using the informer cache directly which under the cover uses processorListener. I also refactored the code to use dynamic.Interface and duck.TypedInformerFactory (not pushed yet). I still think the controller is needed to process events concurrently.

vaikas · 2019-04-26T17:15:25Z

+		apiserversource.NewController(
+			opt,
+			apiserverSourceInformer,
+			deploymentInformer,


Could we just pass the RA Image here instead of buried in the controller?

I guess this is a general issue applying to other sources, like cronjob

vaikas · 2019-04-26T17:16:06Z

+	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"sigs.k8s.io/controller-runtime/pkg/client"


Can we change this to use knative/pkg controller instead of controller runtime?

vaikas · 2019-04-26T17:17:14Z

+	return nil
+}
+
+func (r *Reconciler) getReceiveAdapterImage() string {


I'd prefer that we grab this in the main package and fail there if it's not around. Also I'd prefer we pass it into this method directly instead of panicing here.

I guess this is a general issue applying to other sources, like cronjob

Yeah, seems bad to me :) We should fix them all.

vaikas · 2019-04-26T17:18:08Z

/ok-to-test

n3wscott · 2019-04-26T18:19:58Z

FYI knative/docs#1235

lionelvillard · 2019-04-30T14:55:48Z

@vaikas-google @n3wscott I removed the dependency on controller-runtime. I'm still using controllers for the reasons stated above. I'm happy to revisit the decision if needed.

I didn't implemented any e2e tests yet. I'm not sure yet how to do that.

n3wscott · 2019-04-30T21:37:10Z

+
+	event := cloudevents.Event{
+		Context: cloudevents.EventContextV02{
+			ID:     string(object.GetUID()),


object.GetUID() will always return the same value so the cloudevent will not be unique.

it satisfies https://github.com/cloudevents/spec/blob/v0.2/spec.md#id constraints, in particular MUST be unique within the scope of the producer. @n3wscott what is your concern?

n3wscott · 2019-04-30T22:23:40Z

the error is eventing is out of date. Please run hack/update-codegen.sh

knative-metrics-robot · 2019-05-01T13:43:25Z

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
pkg/adapter/apiserver/adapter.go	Do not exist	44.8%
pkg/apis/sources/v1alpha1/apiserver_lifecycle.go	Do not exist	81.8%
pkg/apis/sources/v1alpha1/register.go	Do not exist	100.0%
pkg/reconciler/apiserversource/apiserversource.go	Do not exist	61.9%
pkg/reconciler/apiserversource/resources/labels.go	Do not exist	0.0%
pkg/reconciler/apiserversource/resources/receive_adapter.go	Do not exist	100.0%

n3wscott · 2019-05-01T14:04:23Z

Thanks a bunch! I think we should start with this and try it out, add some e2e tests...

/lgtm
/approve

knative-prow-robot · 2019-05-01T14:04:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lionelvillard, n3wscott

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~~ [n3wscott]

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

n3wscott · 2019-05-01T14:05:18Z

As a follow-up can you delete the api server source from eventing-sources?

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

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 26, 2019

knative-prow-robot requested review from Harwayne and evankanderson April 26, 2019 15:38

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 26, 2019

vaikas reviewed Apr 26, 2019

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 26, 2019

lionelvillard force-pushed the apiserversource branch from 35a24a4 to e9e9675 Compare April 26, 2019 17:40

n3wscott mentioned this pull request Apr 26, 2019

Heads up: Changes in Knative Eventing Sources knative/docs#1235

Closed

lionelvillard changed the title ~~move apiserver source to eventing~~ WIP: move apiserver source to eventing Apr 30, 2019

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 30, 2019

lionelvillard force-pushed the apiserversource branch 2 times, most recently from 1d70833 to 079f278 Compare April 30, 2019 14:52

lionelvillard force-pushed the apiserversource branch 2 times, most recently from 1cde8ee to 7bd6551 Compare April 30, 2019 21:28

n3wscott reviewed Apr 30, 2019

View reviewed changes

lionelvillard force-pushed the apiserversource branch from 7bd6551 to f077339 Compare May 1, 2019 13:03

move apiserver source to eventing

8dcada8

lionelvillard force-pushed the apiserversource branch from f077339 to 8dcada8 Compare May 1, 2019 13:30

lionelvillard changed the title ~~WIP: move apiserver source to eventing~~ move apiserver source to eventing May 1, 2019

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 May 1, 2019

knative-prow-robot assigned n3wscott May 1, 2019

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

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

knative-prow-robot merged commit c7f1582 into knative:master May 1, 2019

lionelvillard mentioned this pull request Dec 12, 2019

adding lionelvillard to approvers #2314

Merged

lionelvillard deleted the apiserversource branch January 20, 2020 14:58

Conversation

lionelvillard commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot commented Apr 26, 2019

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

lionelvillard Apr 26, 2019 • 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!

vaikas commented Apr 26, 2019

Uh oh!

n3wscott commented Apr 26, 2019

Uh oh!

lionelvillard commented Apr 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Apr 30, 2019

Uh oh!

knative-metrics-robot commented May 1, 2019

Uh oh!

n3wscott commented May 1, 2019

Uh oh!

knative-prow-robot commented May 1, 2019

Uh oh!

n3wscott commented May 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

lionelvillard commented Apr 26, 2019 •

edited

Loading

lionelvillard Apr 26, 2019 •

edited

Loading