e2e-k8s.sh: support alpha features #3023

pohly · 2022-12-14T14:34:39Z

FEATURE_GATES and RUNTIME_CONFIG can be set to populate the corresponding kind config and thus modify the cluster.

pohly · 2022-12-14T14:41:59Z

Let's test the modified script first in kubernetes/test-infra#28248 before merging this PR here.

aojea · 2022-12-14T17:07:39Z

/assign @BenTheElder
too much bash for me 😄

BenTheElder · 2023-01-10T00:35:06Z

Discussed more with aojea. I think we should support this. We won't make the go commands drop in replacements for this script and will migrate some core workflows to something else later while leaving this alone.

BenTheElder · 2023-01-10T00:36:18Z

hack/ci/e2e-k8s.sh

@@ -24,7 +24,12 @@ set -o errexit -o nounset -o xtrace
 # FOCUS: ginkgo focus regex
 # GA_ONLY: true  - limit to GA APIs/features as much as possible
 #          false - (default) APIs and features left at defaults
-# 
+# FEATURE_GATES:


I think maybe we should just require a valid yaml map string in the env and not try to do any bash parsing. Same for runtime_config

like FEATURE_GATES='{foo: true}'

Makes sense, will change it.

I keep forgetting that YAML supports JSON and thus everything in a single string with no line breaks 😅

There's one small (and perfectly acceptable) loss of functionality when removing the parsing:

For the sake of simplicity, not attempt is made to merge the settings from
GA_ONLY with these new env variables. If GA_ONLY=true is set, the new env
variables are ignored.

Revised commit pushed.

/hold

I still need to test it.

I tested:

no vars set

GA_ONLY=true

FEATURE_GATES/RUNTIME_CONFIG as in sig-instrumentation: additional log output testing kubernetes/test-infra#28248

all variables

That all works as expected now.

/hold cancel

The script is also being used in https://testgrid.k8s.io/sig-instrumentation-tests#kind-json-logging-master through https://github.com/pohly/test-infra/blob/cc00ee53e0bee4a32e3171d86ddfc67438c81219/config/jobs/kubernetes/sig-instrumentation/sig-instrumentation-kind-periodics.yaml#L36-L43

That also works.

aojea · 2023-01-10T11:32:01Z

+1

pohly · 2023-01-26T19:23:17Z

/retest

liggitt · 2023-01-31T22:06:12Z

hack/ci/e2e-k8s.sh

+  # JSON or YAML map injected into featureGates config
+  feature_gates="${FEATURE_GATES:-{\}}"
+  # --runtime-config argument value passed to the API server, again as a map
+  runtime_config="${RUNTIME_CONFIG:-{\}}"



should GA_ONLY and FEATURE_GATES/RUNTIME_CONFIG be mutually exclusive?

A prior version of this PR allowed both to be set and used shell code to parse and merge all settings. This was deemed too complex and now the options are mutually exclusive: setting GA_ONLY overrides FEATURE_GATES/RUNTIME_CONFIG.

FWIW, I think that's sufficient: someone who has a need to set features individually can also add the "api/alpha":"false", "api/beta":"false" there. The special case for "certificates.k8s.io/v1beta1":"true" seems irrelevant now.

in the GA_ONLY=true case below, can we detect and error if $feature_gates or $runtime_config are anything other than {}, rather than silently ignoring the envvar-set values

pohly · 2023-04-12T08:35:59Z

@BenTheElder, @liggitt: have your questions regarding this PR been addressed? Please take another look.

serathius · 2023-04-14T12:22:46Z

/retest

pohly · 2023-04-14T12:25:44Z

I force-pushed without any changes to trigger a retest.

liggitt · 2023-04-14T12:26:49Z

one comment to explicitly make GA_ONLY=true and FEATURE_GATES/RUNTIME_CONFIG use mutually exclusive, then lgtm

liggitt · 2023-04-14T12:27:06Z

you'll need to rebase as well, the GA_ONLY=true path got cleaned up a couple months ago

pohly · 2023-04-14T12:35:26Z

... and rebased.

liggitt · 2023-04-14T12:36:30Z

hack/ci/e2e-k8s.sh

+# FEATURE_GATES:
+#          JSON or YAML encoding of a string/bool map: {"FeatureGateA": true, "FeatureGateB": false}
+#          Enables or disables feature gates in the entire cluster.
+#          Ignored when GA_ONLY=true.


Suggested change

# Ignored when GA_ONLY=true.

# Cannot be used when GA_ONLY=true.

liggitt · 2023-04-14T12:36:37Z

hack/ci/e2e-k8s.sh

+# RUNTIME_CONFIG:
+#          JSON or YAML encoding of a string/string (!) map: {"apia.example.com/v1alpha1": "true", "apib.example.com/v1beta1": "false"}
+#          Enables API groups in the apiserver via --runtime-config.
+#          Ignored when GA_ONLY=true.


Suggested change

# Ignored when GA_ONLY=true.

# Cannot be used when GA_ONLY=true.

FEATURE_GATES and RUNTIME_CONFIG can be set to populate the corresponding kind config and thus modify the cluster. For the sake of simplicity, not attempt is made to merge the settings from GA_ONLY with these new env variables. If GA_ONLY=true is set, the new env variables are ignored.

liggitt · 2023-04-14T12:42:32Z

/lgtm

aojea · 2023-04-14T12:44:17Z

hack/ci/e2e-k8s.sh


  case "${GA_ONLY:-false}" in
  false)
-    feature_gates="{}"
-    runtime_config="{}"
+    :


puff, you make me learn things I wish I didn't know they existed 😄 https://stackoverflow.com/a/3224910

Oh no, you incentivised me to click it, now I also wish to forget this.

aojea · 2023-04-14T12:45:39Z

/approve
/hold

Jordan for final approval (unhold)

k8s-ci-robot · 2023-04-14T12:45:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, pohly

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

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

aojea · 2023-04-14T12:47:42Z

oh, he already did
/hold cancel

Thanks patrick

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 14, 2022

k8s-ci-robot requested review from aojea and neolit123 December 14, 2022 14:34

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 14, 2022

k8s-ci-robot assigned BenTheElder Dec 14, 2022

BenTheElder reviewed Jan 10, 2023

View reviewed changes

pohly force-pushed the e2e-feature-gates branch from 9bde011 to f5ece48 Compare January 10, 2023 11:23

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 10, 2023

pohly force-pushed the e2e-feature-gates branch from f5ece48 to b585ec9 Compare January 10, 2023 13:33

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2023

pohly changed the title ~~WIP: e2e-k8s.sh: support alpha features~~ e2e-k8s.sh: support alpha features Jan 10, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2023

mattcary mentioned this pull request Jan 10, 2023

Improve coverage of e2e tests requiring a default storage class kubernetes/kubernetes#114955

Closed

liggitt reviewed Jan 31, 2023

View reviewed changes

pohly mentioned this pull request Apr 12, 2023

sig-instrumentation: add pre-submit jobs for logging kubernetes/test-infra#29266

Merged

pohly force-pushed the e2e-feature-gates branch from b585ec9 to 8cfc170 Compare April 14, 2023 12:25

pohly force-pushed the e2e-feature-gates branch from 8cfc170 to 65e4dff Compare April 14, 2023 12:32

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2023

pohly force-pushed the e2e-feature-gates branch from 65e4dff to c4639bf Compare April 14, 2023 12:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2023

liggitt reviewed Apr 14, 2023

View reviewed changes

pohly force-pushed the e2e-feature-gates branch from c4639bf to 5551b32 Compare April 14, 2023 12:41

k8s-ci-robot assigned liggitt Apr 14, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2023

aojea reviewed Apr 14, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2023

k8s-ci-robot merged commit c861819 into kubernetes-sigs:main Apr 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e-k8s.sh: support alpha features #3023

e2e-k8s.sh: support alpha features #3023

pohly commented Dec 14, 2022

pohly commented Dec 14, 2022

aojea commented Dec 14, 2022

BenTheElder commented Jan 10, 2023

BenTheElder Jan 10, 2023

pohly Jan 10, 2023

pohly Jan 10, 2023

pohly Jan 10, 2023

pohly Jan 10, 2023

pohly Jan 24, 2023

aojea commented Jan 10, 2023

pohly commented Jan 26, 2023

liggitt Jan 31, 2023

pohly Feb 1, 2023

pohly Feb 1, 2023

liggitt Apr 14, 2023

pohly Apr 14, 2023

pohly commented Apr 12, 2023

serathius commented Apr 14, 2023

pohly commented Apr 14, 2023

liggitt commented Apr 14, 2023

liggitt commented Apr 14, 2023

pohly commented Apr 14, 2023

liggitt Apr 14, 2023

pohly Apr 14, 2023

liggitt Apr 14, 2023

pohly Apr 14, 2023

liggitt commented Apr 14, 2023

aojea Apr 14, 2023

serathius Apr 14, 2023

aojea commented Apr 14, 2023

k8s-ci-robot commented Apr 14, 2023

aojea commented Apr 14, 2023

	# Ignored when GA_ONLY=true.
	# Cannot be used when GA_ONLY=true.

e2e-k8s.sh: support alpha features #3023

e2e-k8s.sh: support alpha features #3023

Conversation

pohly commented Dec 14, 2022

pohly commented Dec 14, 2022

aojea commented Dec 14, 2022

BenTheElder commented Jan 10, 2023

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

aojea commented Jan 10, 2023

pohly commented Jan 26, 2023

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

pohly commented Apr 12, 2023

serathius commented Apr 14, 2023

pohly commented Apr 14, 2023

liggitt commented Apr 14, 2023

liggitt commented Apr 14, 2023

pohly commented Apr 14, 2023

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

liggitt commented Apr 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Apr 14, 2023

k8s-ci-robot commented Apr 14, 2023

aojea commented Apr 14, 2023