Default the service links flag setting to false in knative #9622

vagababov · 2020-09-29T22:15:38Z

See #8498 for why are we doing this.

/assign mattmoor

See knative#8498 for why are we doing this. Fixes knative#8563

codecov · 2020-09-29T22:20:51Z

Codecov Report

Merging #9622 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #9622   +/-   ##
=======================================
  Coverage   88.16%   88.16%           
=======================================
  Files         176      176           
  Lines        8414     8416    +2     
=======================================
+ Hits         7418     7420    +2     
  Misses        754      754           
  Partials      242      242

Impacted Files	Coverage Δ
pkg/apis/config/defaults.go	`91.66% <100.00%> (+0.14%)`	⬆️
pkg/apis/serving/v1/revision_defaults.go	`100.00% <100.00%> (ø)`
pkg/reconciler/autoscaling/kpa/scaler.go	`88.40% <0.00%> (-1.45%)`	⬇️
pkg/reconciler/configuration/configuration.go	`89.16% <0.00%> (+1.66%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d0ef0d...65b5533. Read the comment docs.

mattmoor · 2020-09-29T22:20:54Z

/lgtm
/approve

knative-prow-robot · 2020-09-29T22:21:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, vagababov

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~~ [mattmoor,vagababov]
~~pkg/apis/OWNERS~~ [mattmoor,vagababov]

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-09-29T22:43:25Z

The following jobs failed:

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

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

test/upgrade.TestProbe
test/upgrade.TestBYORevisionPostUpgrade

vagababov · 2020-09-29T23:00:11Z

/retest

knative-prow-robot · 2020-09-29T23:10:09Z

New changes are detected. LGTM label has been removed.

vagababov · 2020-09-29T23:14:11Z

It seems we were ignoring defaulting only in the revision itself, but completely failing to disable it when coming via config/service.

vagababov · 2020-09-30T14:51:28Z

/retest

mattmoor · 2020-09-30T16:37:51Z

pkg/apis/serving/v1/revision_defaults.go

@@ -46,6 +41,11 @@ func (rts *RevisionTemplateSpec) SetDefaults(ctx context.Context) {

 // SetDefaults implements apis.Defaultable
 func (rs *RevisionSpec) SetDefaults(ctx context.Context) {
+	// SetDefaults may update revision spec which is immutable.
+	// See: https://github.com/knative/serving/issues/8128 for details.
+	if apis.IsInUpdate(ctx) {


We had this in Revision because Revision is immutable. RevisionSpec is not immutable in all contexts.

Putting this here means that pre-existing resources won't have defaulting applied, which seems to me like it'll be breaking.

I'd like to understand what broke a bit better, and if another change is needed, then let's try and do that separately first?

Sure.
BAsically here we did upgrade from nil EnableServiceLinks to a real value.

What do you have in mind that should be defaulted that wasn't before?

Note that also those can be defaulted in reconciler itself.

dprotaso · 2020-10-01T18:31:38Z

Is the intent to migrate existing revisions?

Let me know if I'm missing something but could we do either of the following:

Set the property on the Deployment if it's not set by the user
Only apply the new default to new revisions?

vagababov · 2020-10-01T18:46:07Z

So always resetting is a problem, since upgrade test is unhappy (can be fixed, but 🤷 )
Also we don't want to migrate existing revisions so the options are:

apply this change (either as one or as two prs — one to push down IsInUpdate check — one to enable the change) — I think @mattmoor has some ideas why this is not kosher.
make the IsInUpdate check when setting the field to ensure old revisions are untouched while keeping everything else the same.

vagababov · 2020-10-04T20:10:11Z

/close

knative-prow-robot · 2020-10-04T20:10:18Z

@vagababov: Closed this PR.

In response to this:

/close

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.

Default the service links flag setting to false in knative

14ccc9b

See knative#8498 for why are we doing this. Fixes knative#8563

knative-prow-robot assigned mattmoor Sep 29, 2020

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

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/API API objects and controllers approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 29, 2020

knative-prow-robot requested review from mdemirhan and nak3 September 29, 2020 22:15

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

fix defaulting for spec

616207e

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

remove the thingy

7eb640d

mattmoor reviewed Sep 30, 2020

View reviewed changes

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

vagababov added 3 commits October 1, 2020 14:01

Merge branch 'master' into 20200929-default-svc-links-false

2c2a3b3

Merge branch 'master' into 20200929-default-svc-links-false

477b86a

fixes and what not

ec7fcdd

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

fix v

65b5533

knative-prow-robot closed this Oct 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default the service links flag setting to false in knative #9622

Default the service links flag setting to false in knative #9622

vagababov commented Sep 29, 2020

codecov bot commented Sep 29, 2020 •

edited

Loading

mattmoor commented Sep 29, 2020

knative-prow-robot commented Sep 29, 2020

knative-test-reporter-robot commented Sep 29, 2020

vagababov commented Sep 29, 2020

knative-prow-robot commented Sep 29, 2020

vagababov commented Sep 29, 2020

vagababov commented Sep 30, 2020

mattmoor Sep 30, 2020

vagababov Sep 30, 2020

vagababov Sep 30, 2020

vagababov Sep 30, 2020

dprotaso commented Oct 1, 2020

vagababov commented Oct 1, 2020

vagababov commented Oct 4, 2020

knative-prow-robot commented Oct 4, 2020

Default the service links flag setting to false in knative #9622

Default the service links flag setting to false in knative #9622

Conversation

vagababov commented Sep 29, 2020

codecov bot commented Sep 29, 2020 • edited Loading

Codecov Report

mattmoor commented Sep 29, 2020

knative-prow-robot commented Sep 29, 2020

knative-test-reporter-robot commented Sep 29, 2020

vagababov commented Sep 29, 2020

knative-prow-robot commented Sep 29, 2020

vagababov commented Sep 29, 2020

vagababov commented Sep 30, 2020

mattmoor Sep 30, 2020

Choose a reason for hiding this comment

vagababov Sep 30, 2020

Choose a reason for hiding this comment

vagababov Sep 30, 2020

Choose a reason for hiding this comment

vagababov Sep 30, 2020

Choose a reason for hiding this comment

dprotaso commented Oct 1, 2020

vagababov commented Oct 1, 2020

vagababov commented Oct 4, 2020

knative-prow-robot commented Oct 4, 2020

codecov bot commented Sep 29, 2020 •

edited

Loading