Skip to content

Experimental flag guard infrastructure#1426

Closed
nikkithurmond wants to merge 1 commit into
knative:masterfrom
nikkithurmond:experiment
Closed

Experimental flag guard infrastructure#1426
nikkithurmond wants to merge 1 commit into
knative:masterfrom
nikkithurmond:experiment

Conversation

@nikkithurmond
Copy link
Copy Markdown
Contributor

@nikkithurmond nikkithurmond commented Jun 29, 2018

Adds dedicated experimental flag guarding. This will pave the way for
making an e2e test dashboard that can continuously test our experiments
without the need to turn them on at HEAD.

To use:

Create a flag in config/experiments/config-experiments.yaml. For
instance,

data:
  autoscaler.enable-my-experiment: "true"

Then, in the binary you would like to flag guard:

	var (
         experimentalFlagSet = k8sflag.NewFlagSet("/etc/config-experiments")

	 enableMyExperiment =
	 experimentalFlagSet.Bool("autoscaler.enable-my-experiment", false)
	 ...
	 )
	 ...

	 # If my experiment is enabled, run my new code path
	 if enableMyExperiment.Get() {
		# my new feature
	 }
	 ...

That's it. To enable experiments, simply run
ko apply -f config/experiments/config-experiments.yaml before running ko apply -f config/

Proposed Changes

  • Adding new configMap, config-experiments
  • Mounting a new optional volume for the new configMap.

Release Note

NONE

@google-prow-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nikkithurmond
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: vaikas-google

Assign the PR to them by writing /assign @vaikas-google in a comment when ready.

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:

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

@google-prow-robot google-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 29, 2018
@nikkithurmond
Copy link
Copy Markdown
Contributor Author

/assign @vaikas-google
/assign @josephburnett
/assign @adrcunha

apiVersion: v1
kind: Namespace
metadata:
name: knative-serving
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Delete this? Why redeclare the namespace?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You can't create the config-map if the namespace doesn't exist. I can delete it, but I was opting for as few steps as possible to use the experiments. This way, you just run ko apply -f config/experiments/

Without it, you will need to manually create the namespace yourself before doing so.

Comment thread config/controller.yaml
- name: config-experiments
configMap:
name: config-experiments
optional: true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why optional? Why not just empty as needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If it isn't optional, the configmap needs to exist. That means I need to declare an empty configMap in /config. If I call ko apply -f config/experiments/ then ko apply -f config, the experiments will be overwritten by the empty map.

I experimented with using kubectl patch, but it doesn't work very elegantly for configmaps.

Adds dedicated experimental flag guarding. This will pave the way for
making an e2e test dashboard that can continuously test our experiments
without the need to turn them on at HEAD.

To use:

* Create a flag in config/experiments/config-experiments.yaml. For
instance,

	data:
	   autoscaler.enable-my-experiment: "true"

Then, in the binary you would like to flag guard:
	var (
         experimentalFlagSet = k8sflag.NewFlagSet("/etc/config-experiments")

	 enableMyExperiment =
	 experimentalFlagSet.Bool("enable-my-experiment", false)

	 ...
	 )
	 ...

	 <existing code path>

	 # If my experiment is enabled, run my new code path
	 if enableMyExperiment.Get() {
		<new code path>
	 }
	 ...

That's it. To enable experiments, simply run ko apply -f
config/experiments/config-experiments.yaml before running ko apply -f
config/

 * Adding new configMap, config-experiments
 * Mounting a new optional volume for the new configMap.
@nikkithurmond
Copy link
Copy Markdown
Contributor Author

/retest

@google-prow-robot
Copy link
Copy Markdown

google-prow-robot commented Jun 29, 2018

@nikkithurmond: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-knative-serving-unit-tests d0b2770 link /test pull-knative-serving-unit-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@mattmoor
Copy link
Copy Markdown
Member

tl;dr Please bias towards ./pkg/configmap.

If you follow the pattern I've been using as I refactor our existing Config types then we can statically check our ConfigMaps schemas (#1421), or leverage configmap.Watch to have our controllers pick up config changes nearly instantly (#1417).

k8sflag is nice, but it scatters the schema across the various places that use parts of it, and its reliance on the Kubelet refreshing ConfigMaps on disk is subject to various TTLs that make its latency very unreliable in practice.

./pkg/configmap's Watch is based on a K8s informer watching knative-serving, which in my experience means that changes are picked up sub-second. 🎉

@nikkithurmond
Copy link
Copy Markdown
Contributor Author

@mattmoor I knew we were moving away from k8sflag, I just wasn't sure if it was done yet. I don't believe anything would change about this PR, though, correct? The only thing that would be different is my PR description?

@nikkithurmond
Copy link
Copy Markdown
Contributor Author

Looking over your change, it looks like it would be different. That's fair, I'll scrap this work.

@mattmoor
Copy link
Copy Markdown
Member

I think the main thing is that we'd want a typed Config and functions that parse map[string]string to Config with some limited validation. I don't know that much else would need to change (besides the description).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants