Guard PodSpec dry run with a feature flag #8276

whaught · 2020-06-09T18:32:44Z

Fixes #8077

Proposed Changes

Guard the PodSpec dry run feature with a feature flag
Integration with the feature gates implemented here: Implement Feature Flags #7897

Release Note

knative-prow-robot

@whaught: 0 warnings.

In response to this:

Fixes #8077

Proposed Changes

Guard the PodSpec dry run feature with a feature flag

Release Note

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.

whaught · 2020-06-09T19:07:07Z

/assign @JRBANCEL

pkg/apis/config/features.go

JRBANCEL · 2020-06-09T19:20:06Z

Apart from the flag naming
/lgtm

whaught · 2020-06-09T20:23:38Z

/hold while I sync with @JRBANCEL on how we want to name these

whaught · 2020-06-09T22:21:13Z

/assign @dprotaso

shashwathi

Does it make sense to have a flag for enabled and make the default behaviour for dryrun as disabled in this case? It would eliminate the need for enabled and disabled flags.

JRBANCEL · 2020-06-10T01:07:28Z

Does it make sense to have a flag for enabled and make the default behaviour for dryrun as disabled in this case? It would eliminate the need for enabled and disabled flags.

We want to provide control to the operator. A cluster operator should be able to disable features entirely, or at least require the developers to manually enable it (that's what Allowed is).
Also, what is enabled by default changes overtime. Flags go through a lifecycle similar to https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#using-a-feature. We need to formalize this.

knative-test-reporter-robot · 2020-06-11T01:00:08Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-unit-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

cmd/activator.[build failed]
cmd/autoscaler.[build failed]
cmd/queue.[build failed]
pkg/activator/handler.[build failed]
pkg/activator/net.[build failed]
pkg/activator/util.[build failed]
pkg/apis/autoscaling/v1alpha1.[build failed]
pkg/apis/config.[build failed]

and 37 more.

JRBANCEL · 2020-06-11T01:36:14Z

/approve
/lgtm

pkg/webhook/validate_service_test.go

pkg/webhook/validate_service.go

dprotaso

/lgtm
/approve

knative-prow-robot · 2020-06-11T19:56:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, JRBANCEL, whaught

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:

~~config/OWNERS~~ [dprotaso]
~~pkg/apis/OWNERS~~ [dprotaso]
~~pkg/webhook/OWNERS~~ [dprotaso]

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

knative-metrics-robot · 2020-06-11T20:00:21Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/features.go	92.3%	86.7%	-5.6
pkg/webhook/validate_service.go	91.7%	93.1%	1.4

vagababov · 2020-06-11T20:17:05Z

pkg/webhook/validate_service.go

+	case config.Disabled:
+		return nil
+	default:
+		mode = DryRunMode(uns.GetAnnotations()[PodSpecDryRunAnnotation])


should this be same as disabled if annotation is not set?

The feature is only client controlled by the annotation in "allowed" mode. Vendors may disable the feature completely or always run it with disable/allowed feature flags.

vagababov · 2020-06-11T20:19:36Z

pkg/webhook/validate_service.go

+	case config.Enabled:
+		mode = DryRunEnabled
+	case config.Disabled:
+		return nil


Is this right though? This basically skips the whole validation?
I'd expect disable means we don't use dry run to validate the template.

At the moment dryrun is the only validation in this codepath, but we'd want to rearrange this if we had other validations here

whaught · 2020-06-11T20:30:09Z

/hold cancel

add feature flag to dryrun

b2a89b8

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

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 9, 2020

knative-prow-robot requested review from dprotaso and vagababov June 9, 2020 18:32

knative-prow-robot reviewed Jun 9, 2020

View reviewed changes

knative-prow-robot added the area/API API objects and controllers label Jun 9, 2020

Add new value to unit test

2808f50

knative-prow-robot assigned JRBANCEL Jun 9, 2020

whaught added 2 commits June 9, 2020 12:14

Update default value to allowed

345cb09

update comment

bf60f7f

JRBANCEL reviewed Jun 9, 2020

View reviewed changes

pkg/apis/config/features.go Outdated Show resolved Hide resolved

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

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2020

include example in the .yaml

c2998d1

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2020

mattmoor reviewed Jun 9, 2020

View reviewed changes

whaught added 2 commits June 9, 2020 13:35

remove whitespace

8a3439c

update checksum

4db8ae7

knative-prow-robot assigned dprotaso Jun 9, 2020

shashwathi reviewed Jun 9, 2020

View reviewed changes

scope flag to kubernetes namespace

d4efed7

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 11, 2020

Merge remote-tracking branch 'upstream/master' into feature-flag

8cb1f46

typo

e8e2521

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

Merge remote-tracking branch 'upstream/master' into feature-flag

df1f4dc

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2020

dprotaso reviewed Jun 11, 2020

View reviewed changes

pkg/webhook/validate_service_test.go Outdated Show resolved Hide resolved

pkg/webhook/validate_service.go Outdated Show resolved Hide resolved

whaught added 2 commits June 11, 2020 12:52

fix nit

3bd2051

fix nit

97201eb

dprotaso approved these changes Jun 11, 2020

View reviewed changes

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

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

vagababov reviewed Jun 11, 2020

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2020

knative-prow-robot merged commit ab1a0fb into knative:master Jun 11, 2020

whaught deleted the feature-flag branch June 19, 2020 04:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guard PodSpec dry run with a feature flag #8276

Guard PodSpec dry run with a feature flag #8276

whaught commented Jun 9, 2020 •

edited

Loading

knative-prow-robot left a comment

whaught commented Jun 9, 2020

JRBANCEL commented Jun 9, 2020

whaught commented Jun 9, 2020

whaught commented Jun 9, 2020

shashwathi left a comment

JRBANCEL commented Jun 10, 2020

knative-test-reporter-robot commented Jun 11, 2020

JRBANCEL commented Jun 11, 2020

dprotaso left a comment

knative-prow-robot commented Jun 11, 2020

knative-metrics-robot commented Jun 11, 2020

vagababov Jun 11, 2020

whaught Jun 11, 2020

vagababov Jun 11, 2020

whaught Jun 11, 2020

whaught commented Jun 11, 2020

Guard PodSpec dry run with a feature flag #8276

Guard PodSpec dry run with a feature flag #8276

Conversation

whaught commented Jun 9, 2020 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

whaught commented Jun 9, 2020

JRBANCEL commented Jun 9, 2020

whaught commented Jun 9, 2020

whaught commented Jun 9, 2020

shashwathi left a comment

Choose a reason for hiding this comment

JRBANCEL commented Jun 10, 2020

knative-test-reporter-robot commented Jun 11, 2020

JRBANCEL commented Jun 11, 2020

dprotaso left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jun 11, 2020

knative-metrics-robot commented Jun 11, 2020

vagababov Jun 11, 2020

Choose a reason for hiding this comment

whaught Jun 11, 2020

Choose a reason for hiding this comment

vagababov Jun 11, 2020

Choose a reason for hiding this comment

whaught Jun 11, 2020

Choose a reason for hiding this comment

whaught commented Jun 11, 2020

whaught commented Jun 9, 2020 •

edited

Loading