Implement CloudEvents for Runs #4663

afrittoli · 2022-03-11T00:20:50Z

Changes

Emit CloudEvents for Runs. This is achieved by:

add a new read-only controller for Runs
emit CloudEvents only (no k8s events) on every reconcile of a Run
use an ephemeral cache to store sent events across reconcile runs.
This is required because since the Runs controller only observes
Runs, it does not have the context to know what was changed in the
Run and though if a new event is required.

The ephemeral cache logic is largely taken from the same
functionality implemented in tektoncd/experimental/cloudevents

Fixes #3862
Depends-on #4659

Signed-off-by: Andrea Frittoli [email protected]

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Tekton Pipelines can now generate CloudEvents for Runs. 
This feature is disabled by default. It can by enable by setting a sink URL and flipping the new feature flag "send-cloudevents-for-runs" to "true"

tekton-robot · 2022-03-11T00:24:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	87.8%	86.0%	-1.8
pkg/reconciler/events/cache/cache.go	Do not exist	84.6%
pkg/reconciler/events/cache/cacheclient.go	Do not exist	7.7%
pkg/reconciler/events/cache/cachefakeclient.go	Do not exist	14.3%
pkg/reconciler/events/cloudevent/cloud_event_controller.go	93.1%	82.9%	-10.2
pkg/reconciler/events/cloudevent/cloudevent.go	91.2%	91.0%	-0.2
pkg/reconciler/events/event.go	95.2%	66.7%	-28.6
pkg/reconciler/run/controller.go	Do not exist	100.0%
pkg/reconciler/run/run.go	Do not exist	100.0%

tekton-robot · 2022-03-11T00:24:43Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	87.8%	86.0%	-1.8
pkg/reconciler/events/cache/cache.go	Do not exist	84.6%
pkg/reconciler/events/cache/cacheclient.go	Do not exist	7.7%
pkg/reconciler/events/cache/cachefakeclient.go	Do not exist	14.3%
pkg/reconciler/events/cloudevent/cloud_event_controller.go	93.1%	82.9%	-10.2
pkg/reconciler/events/cloudevent/cloudevent.go	91.2%	91.0%	-0.2
pkg/reconciler/events/event.go	95.2%	66.7%	-28.6
pkg/reconciler/run/controller.go	Do not exist	100.0%
pkg/reconciler/run/run.go	Do not exist	100.0%

afrittoli · 2022-03-11T00:29:01Z

This PR include one commit from #4659 as it depends on it.
Once #4659 is merge I will remove the extra commit from here.

tekton-robot · 2022-03-11T01:22:35Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	87.8%	86.0%	-1.8
pkg/reconciler/events/cache/cache.go	Do not exist	56.5%
pkg/reconciler/events/cache/cacheclient.go	Do not exist	7.7%
pkg/reconciler/events/cache/cachefakeclient.go	Do not exist	14.3%
pkg/reconciler/events/cloudevent/cloud_event_controller.go	93.1%	82.9%	-10.2
pkg/reconciler/events/cloudevent/cloudevent.go	91.2%	91.0%	-0.2
pkg/reconciler/events/event.go	95.2%	66.7%	-28.6
pkg/reconciler/run/controller.go	Do not exist	100.0%
pkg/reconciler/run/run.go	Do not exist	100.0%

tekton-robot · 2022-03-11T11:54:08Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	87.8%	86.0%	-1.8
pkg/reconciler/events/cache/cache.go	Do not exist	78.3%
pkg/reconciler/events/cache/cacheclient.go	Do not exist	7.7%
pkg/reconciler/events/cache/cachefakeclient.go	Do not exist	14.3%
pkg/reconciler/events/cloudevent/cloud_event_controller.go	93.1%	82.9%	-10.2
pkg/reconciler/events/cloudevent/cloudevent.go	91.2%	91.0%	-0.2
pkg/reconciler/events/event.go	95.2%	66.7%	-28.6
pkg/reconciler/run/controller.go	Do not exist	100.0%
pkg/reconciler/run/run.go	Do not exist	100.0%

tekton-robot · 2022-03-11T12:15:59Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	87.8%	86.0%	-1.8
pkg/reconciler/events/cache/cache.go	Do not exist	78.3%
pkg/reconciler/events/cache/cacheclient.go	Do not exist	7.7%
pkg/reconciler/events/cache/cachefakeclient.go	Do not exist	14.3%
pkg/reconciler/events/cloudevent/cloud_event_controller.go	93.1%	82.9%	-10.2
pkg/reconciler/events/cloudevent/cloudevent.go	91.2%	91.0%	-0.2
pkg/reconciler/events/event.go	95.2%	66.7%	-28.6
pkg/reconciler/run/controller.go	Do not exist	100.0%
pkg/reconciler/run/run.go	Do not exist	100.0%

tekton-robot · 2022-03-11T12:47:30Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	87.8%	86.0%	-1.8
pkg/reconciler/events/cache/cache.go	Do not exist	78.3%
pkg/reconciler/events/cache/cacheclient.go	Do not exist	7.7%
pkg/reconciler/events/cache/cachefakeclient.go	Do not exist	14.3%
pkg/reconciler/events/cloudevent/cloud_event_controller.go	93.1%	82.9%	-10.2
pkg/reconciler/events/cloudevent/cloudevent.go	91.2%	91.0%	-0.2
pkg/reconciler/events/event.go	95.2%	66.7%	-28.6
pkg/reconciler/run/controller.go	Do not exist	100.0%
pkg/reconciler/run/run.go	Do not exist	100.0%

lbernick

I think this would be easier to review if split up into one PR that adds the run controller and one that adds the cloud events (sorry to create more work!)

lbernick · 2022-03-11T15:41:17Z

pkg/reconciler/events/cache/cacheclient.go

+// cacheKey is a way to associate the Cache from inside the context.Context
+type cacheKey struct{}
+
+func withCacheClient(ctx context.Context, cfg *rest.Config) context.Context {


there's some duplication here with thewithFakeCacheClient func below that could probably be pulled into a helper that accepts a buffer size.

lbernick · 2022-03-11T15:48:17Z

pkg/reconciler/run/run_test.go

+// d, where d represents the state of the system (existing resources) needed for the test.
+func getRunController(t *testing.T, d test.Data) (test.Assets, func()) {
+	t.Helper()
+	names.TestingSeed()


this line can probably be removed from the individual test cases

good catch, done

lbernick · 2022-03-11T15:49:00Z

pkg/reconciler/testing/logger.go

@@ -28,6 +30,8 @@ func SetupFakeContext(t *testing.T) (context.Context, []controller.Informer) {
 		SendSuccessfully: true,
 	}
 	ctx = cloudevent.WithClient(ctx, &cloudEventClientBehaviour)
+	cacheClient, _ := lru.New(128)


nit: could you please add an inline comment: lru.New(/*bufSize=*/ 128)

Oh, in fact this is not needed anymore, it's handled by injection and I forgot to remove it here

afrittoli · 2022-03-14T11:36:37Z

I think this would be easier to review if split up into one PR that adds the run controller and one that adds the cloud events (sorry to create more work!)

Hi @lbernick - I'm not sure how you would like to split this further 🙏
The cloud events are added in #4659, but I had to include that commit in this PR because it depends on it - thanks to GitHub that does not support PRs with dependencies.
Once #4659 is merged, I will rebase this and it will only include adding the controller (with the cache).

The only new part in this PR is 749bdba

tekton-robot · 2022-03-14T11:39:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	87.8%	86.0%	-1.8
pkg/reconciler/events/cache/cache.go	Do not exist	78.3%
pkg/reconciler/events/cache/cacheclient.go	Do not exist	7.1%
pkg/reconciler/events/cache/cachefakeclient.go	Do not exist	50.0%
pkg/reconciler/events/cloudevent/cloud_event_controller.go	93.1%	82.9%	-10.2
pkg/reconciler/events/cloudevent/cloudevent.go	91.2%	91.0%	-0.2
pkg/reconciler/events/event.go	95.2%	66.7%	-28.6
pkg/reconciler/run/controller.go	Do not exist	100.0%
pkg/reconciler/run/run.go	Do not exist	100.0%

afrittoli · 2022-03-14T12:05:23Z

/test pull-tekton-pipeline-integration-tests

vdemeester

One small comment, otherwise LGTM 🐯

vdemeester · 2022-03-14T13:47:40Z

pkg/reconciler/events/cache/cacheclient.go

+	logger := logging.FromContext(ctx)
+
+	cacheClient, err := lru.New(size)
+	logger.Infof("CACHE CLIENT %+v", cacheClient)


Is it required in the logs as info or was it a "debug" ? 😛

Well, since this happens once per controller start-up, I thought it would be useful to have it at info to know that the cache was initialised. If it should up a second time it means something is very wrong!

tekton-robot · 2022-03-14T13:48:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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:

~~OWNERS~~ [vdemeester]

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

lbernick · 2022-03-14T14:14:42Z

Hi @lbernick - I'm not sure how you would like to split this further 🙏

I was thinking moving the contents of controller.go and most of run.go into a separate PR (to merge before adding the emitCloudEvents functionality) however I'd be OK with lgtm-ing as is once you rebase this.

tekton-robot · 2022-03-14T14:43:11Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	87.8%	86.0%	-1.8
pkg/reconciler/events/cache/cache.go	Do not exist	78.3%
pkg/reconciler/events/cache/cacheclient.go	Do not exist	7.1%
pkg/reconciler/events/cache/cachefakeclient.go	Do not exist	50.0%
pkg/reconciler/events/cloudevent/cloud_event_controller.go	93.1%	82.9%	-10.2
pkg/reconciler/events/event.go	95.2%	66.7%	-28.6
pkg/reconciler/run/controller.go	Do not exist	100.0%
pkg/reconciler/run/run.go	Do not exist	100.0%

afrittoli · 2022-03-14T14:47:00Z

Hi @lbernick - I'm not sure how you would like to split this further 🙏

I was thinking moving the contents of controller.go and most of run.go into a separate PR (to merge before adding the emitCloudEvents functionality) however I'd be OK with lgtm-ing as is once you rebase this.

Oh, I see. The reason I put this together in one PR is that the only thing the controller does though is to invoke emitCloudEvents, so if moved that in a separate PR I wouldn't be able to test much about the reconcile behaviour in the initial PR.

lbernick · 2022-03-14T14:57:39Z

Oh, I see. The reason I put this together in one PR is that the only thing the controller does though is to invoke emitCloudEvents, so if moved that in a separate PR I wouldn't be able to test much about the reconcile behaviour in the initial PR.

I think that's ok-- no need to add more functionality for the purpose of being able to test something :) The main reason I suggested this was for ease of review. However again it's up to you.

afrittoli · 2022-03-14T15:11:39Z

/test pull-tekton-pipeline-alpha-integration-tests

afrittoli · 2022-03-14T15:40:46Z

Note that when leader election is enabled, enqueued resources will be split into buckets using an hashing function which tasks namespace/name as input (https://github.com/knative/pkg/blob/ce6f2877cef94d5c91bafe2ce4acba52ee22c1fe/controller/controller.go#L425-L431). This means that the same resource will be consistently re-enqueue to the same controller for every subsequent update, which implies that the local cloudevents cache works fine in the HA multi-master case too.

If leadership for a bucket is transferred to a different controller instance, the local cloudevent cache will be lost, similar to what happens in case of controller restart, hence the caveat in the docs.

afrittoli · 2022-03-14T17:15:55Z

Thanks @lbernick - it's rebased now - only one commit in the PR :)

lbernick · 2022-03-14T18:15:56Z

pkg/reconciler/events/cloudevent/cloud_event_controller.go

@@ -137,11 +139,25 @@ func SendCloudEventWithRetries(ctx context.Context, object runtime.Object) error
 	if err != nil {
 		return err
 	}
+	// Events for Runs require a cache of events that have been sent


would you mind adding some tests in cloud_event_controller_test.go to cover the new functionality?

The way I tested that is in https://github.com/tektoncd/pipeline/pull/4663/files#diff-001bb78547301c75f05f989ea355d11512f0cfa73d378d0f0c75e498cdaa6214R249-R252 by running reconcile twice and checking that events are sent the first time and not the second one.
I don't think unit tests specific to SendCloudEventWithRetries would add much coverage, but I can try and add some if you'd like.

would it be possible to just add a test case similar to this one for sending cloud events for a run?

Test added, I hope this look ok.

Add TestEmitCloudEvents as well now

tekton-robot · 2022-03-14T21:24:50Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	87.8%	86.0%	-1.8
pkg/reconciler/events/cache/cache.go	Do not exist	78.3%
pkg/reconciler/events/cache/cacheclient.go	Do not exist	7.1%
pkg/reconciler/events/cache/cachefakeclient.go	Do not exist	50.0%
pkg/reconciler/events/cloudevent/cloud_event_controller.go	93.1%	88.6%	-4.5
pkg/reconciler/events/event.go	95.2%	66.7%	-28.6
pkg/reconciler/run/controller.go	Do not exist	100.0%
pkg/reconciler/run/run.go	Do not exist	100.0%

tekton-robot · 2022-03-14T21:35:23Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	87.8%	86.0%	-1.8
pkg/reconciler/events/cache/cache.go	Do not exist	78.3%
pkg/reconciler/events/cache/cacheclient.go	Do not exist	7.1%
pkg/reconciler/events/cache/cachefakeclient.go	Do not exist	50.0%
pkg/reconciler/events/cloudevent/cloud_event_controller.go	93.1%	88.6%	-4.5
pkg/reconciler/events/event.go	95.2%	93.3%	-1.9
pkg/reconciler/run/controller.go	Do not exist	100.0%
pkg/reconciler/run/run.go	Do not exist	100.0%

afrittoli · 2022-03-14T21:37:49Z

Uhm, interesting, it looks like a flake in an existing unit test :S

/test pull-tekton-pipeline-unit-tests

afrittoli · 2022-03-14T21:41:43Z

/test pull-tekton-pipeline-unit-tests

Emit CloudEvents for Runs. This is achieved by: - add a new read-only controller for Runs - emit CloudEvents only (no k8s events) on every reconcile of a Run - use an ephemeral cache to store sent events across reconcile runs. This is required because since the Runs controller only observes Runs, it does not have the context to know what was changed in the Run and though if a new event is required. The ephemeral cache logic is largely taken from the same functionality implemented in tektoncd/experimental/cloudevents Fixes tektoncd#3862 Signed-off-by: Andrea Frittoli <[email protected]>

tekton-robot · 2022-03-14T21:44:50Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	87.8%	86.0%	-1.8
pkg/reconciler/events/cache/cache.go	Do not exist	78.3%
pkg/reconciler/events/cache/cacheclient.go	Do not exist	7.1%
pkg/reconciler/events/cache/cachefakeclient.go	Do not exist	50.0%
pkg/reconciler/events/cloudevent/cloud_event_controller.go	93.1%	88.6%	-4.5
pkg/reconciler/events/event.go	95.2%	93.3%	-1.9
pkg/reconciler/run/controller.go	Do not exist	100.0%
pkg/reconciler/run/run.go	Do not exist	100.0%

afrittoli · 2022-03-14T22:11:37Z

/test pull-tekton-pipeline-alpha-integration-tests

afrittoli · 2022-03-14T23:10:44Z

/test pull-tekton-pipeline-alpha-integration-tests

lbernick · 2022-03-15T03:53:47Z

/lgtm

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 11, 2022

tekton-robot requested review from bobcatfish and jerop March 11, 2022 00:21

afrittoli force-pushed the run_events_impl branch from 59d7d52 to ff8ad27 Compare March 11, 2022 00:21

afrittoli added this to the Pipelines v0.34 milestone Mar 11, 2022

afrittoli mentioned this pull request Mar 11, 2022

Cloud Events for Runs #3862

Closed

afrittoli force-pushed the run_events_impl branch from ff8ad27 to c864705 Compare March 11, 2022 01:20

afrittoli force-pushed the run_events_impl branch from c864705 to c7fb98e Compare March 11, 2022 11:51

afrittoli force-pushed the run_events_impl branch from c7fb98e to 4e94abf Compare March 11, 2022 12:12

afrittoli force-pushed the run_events_impl branch from 4e94abf to 749bdba Compare March 11, 2022 12:44

lbernick reviewed Mar 11, 2022

View reviewed changes

afrittoli force-pushed the run_events_impl branch from 749bdba to a7a87a7 Compare March 14, 2022 11:37

vdemeester approved these changes Mar 14, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2022

afrittoli force-pushed the run_events_impl branch from a7a87a7 to 0af7079 Compare March 14, 2022 14:40

tekton-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 14, 2022

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 14, 2022

lbernick reviewed Mar 14, 2022

View reviewed changes

afrittoli force-pushed the run_events_impl branch from 0af7079 to 00a8093 Compare March 14, 2022 21:22

afrittoli force-pushed the run_events_impl branch from 00a8093 to ffac70e Compare March 14, 2022 21:33

afrittoli force-pushed the run_events_impl branch from ffac70e to 2abc68b Compare March 14, 2022 21:42

tekton-robot assigned lbernick Mar 15, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2022

tekton-robot merged commit 45b7c37 into tektoncd:main Mar 15, 2022

Implement CloudEvents for Runs #4663

Implement CloudEvents for Runs #4663

Conversation

afrittoli commented Mar 11, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Mar 11, 2022

tekton-robot commented Mar 11, 2022

afrittoli commented Mar 11, 2022

tekton-robot commented Mar 11, 2022

tekton-robot commented Mar 11, 2022

tekton-robot commented Mar 11, 2022

tekton-robot commented Mar 11, 2022

lbernick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli commented Mar 14, 2022

tekton-robot commented Mar 14, 2022

afrittoli commented Mar 14, 2022

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Mar 14, 2022

lbernick commented Mar 14, 2022

tekton-robot commented Mar 14, 2022

afrittoli commented Mar 14, 2022

lbernick commented Mar 14, 2022

afrittoli commented Mar 14, 2022

afrittoli commented Mar 14, 2022

afrittoli commented Mar 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Mar 14, 2022

tekton-robot commented Mar 14, 2022

afrittoli commented Mar 14, 2022

afrittoli commented Mar 14, 2022

tekton-robot commented Mar 14, 2022

afrittoli commented Mar 14, 2022

afrittoli commented Mar 14, 2022

lbernick commented Mar 15, 2022

afrittoli commented Mar 11, 2022 •

edited

Loading