Introduce config map watcher for feature flags #2637

adshmh · 2020-05-17T20:30:22Z

Changes

Taskrun reconciler now uses a configmap watcher for feature-flags
configmap to remove the need for an extra Kubernetes API call to get the
configmap's value every time a pod is being created. Fixes #2117.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

tekton-robot · 2020-05-17T20:30:37Z

Hi @adshmh. Thanks for your PR.

I'm waiting for a tektoncd 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.

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.

tekton-robot · 2020-05-17T20:30:44Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-05-17T20:30:53Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-05-17T20:31:00Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

vdemeester · 2020-05-18T10:44:56Z

/ok-to-test
/kind feature
/cc @sbwsg

tekton-robot · 2020-05-18T10:47:39Z

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/pod/pod.go	Do not exist	82.8%
pkg/reconciler/taskrun/controller.go	96.2%	96.4%	0.3
pkg/reconciler/taskrun/taskrun.go	75.2%	75.3%	0.1

tekton-robot · 2020-05-18T11:02: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/pod/pod.go	Do not exist	82.8%
pkg/reconciler/taskrun/controller.go	96.2%	96.4%	0.3
pkg/reconciler/taskrun/taskrun.go	75.2%	75.3%	0.1

tekton-robot · 2020-05-18T12:06:55Z

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/pod/pod.go	Do not exist	82.8%
pkg/reconciler/taskrun/controller.go	96.2%	96.4%	0.3
pkg/reconciler/taskrun/taskrun.go	75.2%	75.3%	0.1

tekton-robot · 2020-05-18T13:18:44Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

afrittoli

Thanks a lot for working on this, I've been wondering why we used the configStore for pipelineruns and not for taskruns too.
My main questions on this is whether we really need the extra module, or if we could just use the existing one and add extra flags to it?

The other question I have is about the existing config store. The testing methods for the pipelinerun controller do not support having configmaps as test data to seed. I think this probably means that we had less coverage there. Would it make sense to add the same feature there? (possibly in a different PR)

afrittoli · 2020-05-21T09:37:49Z

pkg/apis/config/feature_flags.go

@@ -0,0 +1,78 @@
+/*
+Copyright 2019 The Tekton Authors


NIT: this should probably be 2020

Thank you for the review. Fixed.

The other question I have is about the existing config store. The testing methods for the pipelinerun controller do not support having configmaps as test data to seed. I think this probably means that we had less coverage there. Would it make sense to add the same feature there? (possibly in a different PR)

Sure. I will review the pipelinerun controller code and submit a PR to cover this.

afrittoli · 2020-05-21T09:38:19Z

pkg/apis/config/feature_flags_test.go

@@ -0,0 +1,99 @@
+/*
+Copyright 2019 The Tekton Authors


Thank you for the review. Fixed.

afrittoli · 2020-05-21T09:41:18Z

pkg/apis/config/testdata/feature-flags-all-flags-set.yaml

@@ -0,0 +1,22 @@
+# Copyright 2019 The Tekton Authors


Thank you for the review. Fixed.

afrittoli · 2020-05-21T09:41:26Z

pkg/apis/config/testdata/feature-flags-empty.yaml

@@ -0,0 +1,26 @@
+# Copyright 2019 The Tekton Authors


Thank you for the review. Fixed.

afrittoli · 2020-05-21T09:41:33Z

pkg/apis/config/testdata/feature-flags.yaml

@@ -0,0 +1,22 @@
+# Copyright 2019 The Tekton Authors


Thank you for the review. Fixed.

afrittoli · 2020-05-21T11:12:17Z

pkg/pod/pod.go

-		return false
-	}
-	return true
+func ShouldOverrideHomeEnv(ctx context.Context) bool {


afrittoli · 2020-05-21T11:48:04Z

pkg/reconciler/taskrun/taskrun_test.go

 		Clients:    c,
-	}, cancel
+	}, cancel, stopCh


We already return a cancel function to tests, so it doesn't feel ok to pass them an extra channel to stop.

I wonder if it would be possible to change SetupFakeContext to handle the configMapWatcher instead, but at least we should be able to return something like:

, func() { close(stopCh) cancel() }

Thank you for the review. Fixed. I will take a look and submit a PR if there is a way to avoid dealing with the channel altogether, i.e. letting SetupFakeContext handle it.

afrittoli · 2020-05-21T11:53:53Z

pkg/reconciler/taskrun/taskrun_test.go

@@ -479,6 +519,184 @@ func TestReconcile_ExplicitDefaultSA(t *testing.T) {
 	}
 }

+func TestReconcile_FeatureFlags(t *testing.T) {
+	taskWithEnvVar := tb.Task("test-task-with-env-var",


It would be great if you could add a comment that describes what the test aims to verify!

Thank you for the review. Fixed.

afrittoli · 2020-05-21T14:29:57Z

pkg/apis/config/store.go

 	return &Config{
-		Defaults: defaults,
+		Defaults:     defaults,
+		FeatureFlags: featureFlags,


Could you clarify why we need FeatureFlags to be separated from Defaults?
AFAIU feature flags are a special type of defaults, that take a boolean value

Thanks a lot for working on this, I've been wondering why we used the configStore for pipelineruns and not for taskruns too.
My main questions on this is whether we really need the extra module, or if we could just use the existing one and add extra flags to it?

Thank you for the detailed review. Please let me know if the following sounds correct. I'd be happy to update the PR if needed to merge the two sets of options.

I have implemented the draft this way mainly because the feature-flags and config-defaults are different config maps. Modules using the config store are only minimally affected by this: they use config store the same way, i.e. by passing the context, just specify the flags under FeatureFlags "option group" as Defaults and FeatureFlags are both structs under the same config module.

If combining the two sets is preferred, please let me know and I will update the PR.

I tend to like having it seperate 🤔
/cc @sbwsg

Indeed, feature-flags is already a separate ConfigMap. If we wanted to merge it with config-defaults then I'd prefer that be a discussion/PR unto itself rather than mix it in with a behavioral change such as this. @afrittoli if you prefer to go that route do you want to pursue it at an upcoming WG?

tekton-robot · 2020-05-22T15:57:14Z

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/pod/pod.go	Do not exist	82.8%
pkg/reconciler/taskrun/controller.go	96.2%	96.4%	0.3
pkg/reconciler/taskrun/taskrun.go	75.2%	75.3%	0.1

vdemeester

/lgtm

vdemeester · 2020-06-02T09:12:58Z

pkg/apis/config/store.go

 	return &Config{
-		Defaults: defaults,
+		Defaults:     defaults,
+		FeatureFlags: featureFlags,


I tend to like having it seperate 🤔
/cc @sbwsg

afrittoli

Thanks for the clarification.
My intention was not to merge the configmaps necessarily, I was only trying to understand if we had to duplicate the logic that manages the configmaps.

It's fine for me to go ahead as it is, we can still factor things up later if possible.

ghost · 2020-06-02T13:00:51Z

I was only trying to understand if we had to duplicate the logic that manages the configmaps.

Oh I see, apologies! This makes perfect sense to me. Agree on it being a possible improvement!

ghost · 2020-06-02T14:19:21Z

vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/fake/fake.go

@@ -0,0 +1,40 @@
+/*


Where did these vendor changes come from? I see vendor/modules.txt updated but I have to confess I don't really understand how that plays with go.mod / go.sum. Did you manually update that file? Or was it somehow automatically updated?

Thank you for the review. The changes in vendor/modules.txt were made by the hack/update-deps.sh script.

ghost

I don't really understand why vendor/modules.txt would be updated but not go.mod or go.sum but 🤷.

tekton-robot · 2020-06-04T13:33:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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

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

jlpettersson · 2020-06-04T14:13:04Z

There are two new feature-flags added to this configMap in #2158 and #2630

Feature flags configuration is now tracked by the config store. This allows watching for the corresponding config map changes, removing the need to call kubernetes API to get the feature flags config map every time a pod is created. Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>

Taskrun reconciler now uses a configmap watcher for `feature-flags` configmap to remove the need for an extra Kubernetes API call to get the configmap's value every time a pod is being created. Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>

adshmh · 2020-06-06T17:09:42Z

There are two new feature-flags added to this configMap in #2158 and #2630

Thank you for the reminder. Fixed.

tekton-robot · 2020-06-06T17:12: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/pod/pod.go	Do not exist	84.5%
pkg/reconciler/pipelinerun/affinity_assistant.go	91.8%	91.5%	-0.3
pkg/reconciler/taskrun/controller.go	96.2%	96.4%	0.3
pkg/reconciler/taskrun/taskrun.go	76.0%	76.1%	0.1

adshmh · 2020-06-06T18:26:34Z

/retest

vdemeester · 2020-06-08T08:51:19Z

/lgtm

tekton-robot requested review from imjasonh and a user May 17, 2020 20:30

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 17, 2020

tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. 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 May 18, 2020

adshmh force-pushed the 2117-config-map-watcher-for-feature-flags branch from 210c5eb to 19e2cf0 Compare May 18, 2020 11:00

adshmh force-pushed the 2117-config-map-watcher-for-feature-flags branch from 19e2cf0 to dd6ff21 Compare May 18, 2020 12:04

vdemeester added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/feature Categorizes issue or PR as related to a new feature. labels May 18, 2020

jlpettersson mentioned this pull request May 21, 2020

Avoid unecessary work within reconciliation loop - LimitRanges #2666

Closed

afrittoli reviewed May 21, 2020

View reviewed changes

adshmh force-pushed the 2117-config-map-watcher-for-feature-flags branch from dd6ff21 to d0fa286 Compare May 22, 2020 15:54

dibyom mentioned this pull request May 26, 2020

Cache trigger secrets for the duration of request tektoncd/triggers#585

Closed

3 tasks

vdemeester reviewed Jun 2, 2020

View reviewed changes

tekton-robot assigned vdemeester Jun 2, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2020

afrittoli reviewed Jun 2, 2020

View reviewed changes

ghost reviewed Jun 2, 2020

View reviewed changes

ghost approved these changes Jun 4, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2020

adshmh added 2 commits June 6, 2020 11:07

adshmh force-pushed the 2117-config-map-watcher-for-feature-flags branch from d0fa286 to 64da2b5 Compare June 6, 2020 17:09

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 6, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2020

tekton-robot merged commit 6658b82 into tektoncd:master Jun 8, 2020

jlpettersson mentioned this pull request Jul 24, 2020

The affinity-assistant pods are still spawning after disabling the associated feature-flag: disable-affinity-assistant #2979

Closed

bobcatfish mentioned this pull request Nov 24, 2020

Nightly unit test failure due to race condition when initializing config stores #2815

Closed

bobcatfish mentioned this pull request Jan 26, 2021

Race condition on startup when using configuring controller via configmaps #3720

Closed

Introduce config map watcher for feature flags #2637

Introduce config map watcher for feature flags #2637

Conversation

adshmh commented May 17, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented May 17, 2020

tekton-robot commented May 17, 2020

tekton-robot commented May 17, 2020

tekton-robot commented May 17, 2020

vdemeester commented May 18, 2020

tekton-robot commented May 18, 2020

tekton-robot commented May 18, 2020

tekton-robot commented May 18, 2020

tekton-robot commented May 18, 2020

afrittoli 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

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

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

tekton-robot commented May 22, 2020

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

ghost commented Jun 2, 2020

Choose a reason for hiding this comment

adshmh Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 4, 2020

jlpettersson commented Jun 4, 2020

adshmh commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

adshmh commented Jun 6, 2020

vdemeester commented Jun 8, 2020

adshmh commented May 17, 2020 •

edited

Loading

adshmh Jun 3, 2020 •

edited

Loading