Add persistent cache store backed by ConfigMap. Refactor mtping #3451

lionelvillard · 2020-06-30T21:47:53Z

One step towards #3157

Proposed Changes

Add a new PersistentStore utility for persisting informer cache to a ConfigMap
Adapter can now be configured to watch for ConfigMap changes. Disabled by default.
mtping now watches a single (for now) configmap, instead of watching all PingSources.

Release Note

- 🧽 The multi-tenant PingSource adapter consumes less resources.

Docs

lionelvillard · 2020-06-30T21:49:05Z

FYI. @nachocano @pierDipi

lionelvillard · 2020-07-01T14:00:51Z

I'm really not convinced mounting the configmap vs creating a watch is more efficient. Under the cover the kubelet creates a reflector for each pod with a mounted configmap (not verified): https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/util/manager/watch_based_manager.go#L149

Since HA works with watches, I think I'll stick with it (at least for now).

@nachocano @pierDipi I know you have been looking at this. Any insights?

Edit: more info: https://kubernetes.io/docs/concepts/configuration/configmap/#mounted-configmaps-are-updated-automatically. Specifically: improves performance of your cluster by significantly reducing load on kube-apiserver, by closing watches for config maps marked as immutable.

lgtm-com · 2020-07-14T15:59:01Z

This pull request introduces 1 alert when merging 0a0275a into 5c276d0 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2020-07-14T17:54:25Z

This pull request introduces 1 alert when merging 706ae64 into 5c276d0 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

cr22rc

Look at minor issues
/lgm

cmd/mtping/main.go

pkg/reconciler/pingsource/pingsource.go

pkg/adapter/mtping/adapter.go

cr22rc · 2020-07-14T19:55:12Z

pkg/adapter/mtping/runner.go

 			event.SetExtension(key, override)
 		}
 	}

 	ctx := context.Background()
-	ctx = cloudevents.ContextWithTarget(ctx, sink)
+	ctx = cloudevents.ContextWithTarget(ctx, cfg.SinkURI)


check for error returned?

there is no error returned here.

…y reason

lionelvillard · 2020-07-15T15:41:45Z

this is really annoying. Can't reproduce locally :-(

lionelvillard · 2020-07-15T18:53:06Z

seems unrelated
/test pull-knative-eventing-integration-tests

lionelvillard · 2020-07-15T22:37:28Z

/test pull-knative-eventing-integration-tests

vaikas · 2020-07-16T20:15:36Z

Can you share more about the motivation for this? controller is watching for all the CRs already, are you running into issues there?

lionelvillard · 2020-07-16T22:04:20Z

This is for #3157. The gist of it is in this comment

The main motivation is to be able to partition CRs across multiple receive adapters and to make sure they are HA. It seems natural to use a ConfigMap as our resource lock, which happens to also contain the resources the leading receive adapter needs to handle. At the same time, using a ConfigMap makes our receive adapters lighter weight since they don't need to store all CRs anymore.

Note that GCPPubSub is already doing this I believe (except that it mounts the ConfigMap instead of setting a watch).

The HA design follows the Per-Reconciler Leader Election design adapted to support Adapter instead of LeaderAware reconciler.

Does that make sense? @vaikas

cr22rc · 2020-07-17T10:37:23Z

/lgtm

knative-prow-robot · 2020-07-17T10:37:29Z

@cr22rc: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

knative-metrics-robot · 2020-07-17T14:43:23Z

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

File	Old Coverage	New Coverage	Delta
pkg/adapter/mtping/adapter.go	100.0%	90.0%	-10.0
pkg/adapter/mtping/config.go	Do not exist	85.7%
pkg/adapter/mtping/runner.go	92.9%	84.5%	-8.4
pkg/adapter/v2/config_watcher.go	Do not exist	78.9%
pkg/adapter/v2/main.go	60.4%	54.8%	-5.6
pkg/reconciler/pingsource/controller.go	93.1%	93.5%	0.4
pkg/utils/cache/persisted_store.go	Do not exist	78.6%

vaikas · 2020-07-17T15:42:10Z

pkg/adapter/mtping/runner.go

+		if entry, ok := a.entryids[key]; ok {
+			a.Logger.Info("deleting schedule", zap.Any("key", key))
+			a.RemoveSchedule(entry.entryID)
+			delete(a.entryids, key)


Don't you need some synchronization when you're messing with the map and using it in different places?

The map in only used in this function and it is guaranteed to be called by only one go routine (see persistent_store)

vaikas · 2020-07-17T16:02:00Z

/lgtm
/approve

knative-prow-robot · 2020-07-17T16:02:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cr22rc, lionelvillard, vaikas

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~~ [lionelvillard,vaikas]

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

knative-test-reporter-robot · 2020-07-17T16:45:06Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-eventing-integration-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-integration-tests:

test/e2e.TestPingSourceV1Alpha2EventTypes

lionelvillard · 2020-07-17T17:30:37Z

/test pull-knative-eventing-integration-tests

knative-prow-robot requested review from lberk and liu-cong June 30, 2020 21:48

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 30, 2020

lionelvillard changed the title ~~mtping now gets PingSources spec from a mounted configmap~~ WIP: mtping now gets PingSources spec from a mounted configmap Jun 30, 2020

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 Jun 30, 2020

lionelvillard force-pushed the mtping-configmap branch from 52cf8a0 to e6e6534 Compare July 14, 2020 02:07

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 14, 2020

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 14, 2020

lionelvillard changed the title ~~WIP: mtping now gets PingSources spec from a mounted configmap~~ WIP: add persistent cache store backed by ConfigMap Jul 14, 2020

lionelvillard force-pushed the mtping-configmap branch from e6e6534 to 0a0275a Compare July 14, 2020 15:47

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2020

lionelvillard changed the title ~~WIP: add persistent cache store backed by ConfigMap~~ Add persistent cache store backed by ConfigMap. Refactor mtping Jul 14, 2020

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 Jul 14, 2020

lionelvillard force-pushed the mtping-configmap branch from b0dc2d4 to 0513cd3 Compare July 14, 2020 19:25

cr22rc approved these changes Jul 14, 2020

View reviewed changes

lionelvillard added 8 commits July 15, 2020 09:28

add persistent store backed by configmap

1f6d48e

adapter support watching configmap. Disabled by default

3f4ab67

cleanup, add more tests

476ff7c

fix ut, and maybe it

ad73b32

Setting up informers in now an opt-in flag, for backward compatibilit…

7feac1f

…y reason

more tests

cb1c7c4

changes from review and fix configmap label

2249f46

filter resource-scoped pingsources (yeah still there)

3dfc311

lionelvillard force-pushed the mtping-configmap branch from f08ba82 to 3dfc311 Compare July 15, 2020 13:33

really delete schedule

b3b07ca

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Jul 15, 2020

fix crash when configmap does not exist

ce027ef

add more tests

720b70f

fix broken k8s event

57bbb98

vaikas reviewed Jul 17, 2020

View reviewed changes

knative-prow-robot assigned vaikas Jul 17, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2020

knative-prow-robot merged commit 1cbaa24 into knative:master Jul 17, 2020

lionelvillard mentioned this pull request Aug 26, 2020

revert change to get PingSource specs from ConfigMaps #3935

Merged

lionelvillard deleted the mtping-configmap branch August 28, 2020 15:38

lionelvillard restored the mtping-configmap branch September 3, 2020 15:42

lionelvillard deleted the mtping-configmap branch September 3, 2020 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add persistent cache store backed by ConfigMap. Refactor mtping #3451

Add persistent cache store backed by ConfigMap. Refactor mtping #3451

lionelvillard commented Jun 30, 2020 •

edited

Loading

lionelvillard commented Jun 30, 2020

lionelvillard commented Jul 1, 2020 •

edited

Loading

lgtm-com bot commented Jul 14, 2020

lgtm-com bot commented Jul 14, 2020

cr22rc left a comment

cr22rc Jul 14, 2020

lionelvillard Jul 14, 2020

lionelvillard commented Jul 15, 2020

lionelvillard commented Jul 15, 2020

lionelvillard commented Jul 15, 2020

vaikas commented Jul 16, 2020

lionelvillard commented Jul 16, 2020

cr22rc commented Jul 17, 2020

knative-prow-robot commented Jul 17, 2020

knative-metrics-robot commented Jul 17, 2020

vaikas Jul 17, 2020

lionelvillard Jul 17, 2020

vaikas Jul 17, 2020

vaikas commented Jul 17, 2020

knative-prow-robot commented Jul 17, 2020

knative-test-reporter-robot commented Jul 17, 2020

lionelvillard commented Jul 17, 2020

Add persistent cache store backed by ConfigMap. Refactor mtping #3451

Add persistent cache store backed by ConfigMap. Refactor mtping #3451

Conversation

lionelvillard commented Jun 30, 2020 • edited Loading

Proposed Changes

lionelvillard commented Jun 30, 2020

lionelvillard commented Jul 1, 2020 • edited Loading

lgtm-com bot commented Jul 14, 2020

lgtm-com bot commented Jul 14, 2020

cr22rc left a comment

Choose a reason for hiding this comment

cr22rc Jul 14, 2020

Choose a reason for hiding this comment

lionelvillard Jul 14, 2020

Choose a reason for hiding this comment

lionelvillard commented Jul 15, 2020

lionelvillard commented Jul 15, 2020

lionelvillard commented Jul 15, 2020

vaikas commented Jul 16, 2020

lionelvillard commented Jul 16, 2020

cr22rc commented Jul 17, 2020

knative-prow-robot commented Jul 17, 2020

knative-metrics-robot commented Jul 17, 2020

vaikas Jul 17, 2020

Choose a reason for hiding this comment

lionelvillard Jul 17, 2020

Choose a reason for hiding this comment

vaikas Jul 17, 2020

Choose a reason for hiding this comment

vaikas commented Jul 17, 2020

knative-prow-robot commented Jul 17, 2020

knative-test-reporter-robot commented Jul 17, 2020

lionelvillard commented Jul 17, 2020

lionelvillard commented Jun 30, 2020 •

edited

Loading

lionelvillard commented Jul 1, 2020 •

edited

Loading