Revision return replicas by leetcope · Pull Request #11038 · knative/serving

leetcope · 2021-03-26T03:58:36Z

Proposed Changes

API change to include ActualReplicas and DesiredReplicas in Revision status
Assert and use the ActualReplicas and DesiredReplicas in e2e tests

codecov · 2021-03-26T04:04:00Z

Codecov Report

Merging #11038 (7abff9f) into main (813aa65) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #11038      +/-   ##
==========================================
- Coverage   87.74%   87.72%   -0.02%     
==========================================
  Files         190      190              
  Lines        9162     9164       +2     
==========================================
  Hits         8039     8039              
- Misses        870      871       +1     
- Partials      253      254       +1

Impacted Files	Coverage Δ
pkg/apis/serving/v1/revision_lifecycle.go	`92.98% <100.00%> (+0.25%)`	⬆️
pkg/autoscaler/statforwarder/leases.go	`76.97% <0.00%> (-1.44%)`	⬇️

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 813aa65...7abff9f. Read the comment docs.

leetcope · 2021-03-26T12:21:14Z

/retest

julz · 2021-03-26T13:33:51Z

pkg/apis/serving/v1/revision_lifecycle.go

 	// Reflect the PA status in our own.
 	cond := ps.GetCondition(autoscalingv1alpha1.PodAutoscalerConditionReady)
+	if ps.ActualScale != nil {
+		rs.ActualReplicas = *ps.ActualScale


Could we use the GetActualScale helper here and avoid the nil check?

julz · 2021-03-26T13:36:40Z

pkg/apis/serving/v1/revision_lifecycle.go

+	if ps.ActualScale != nil {
+		rs.ActualReplicas = *ps.ActualScale
+	} else {
+		rs.ActualReplicas = 0


I think this else may be redundant since ActualReplicas defaults to 0 anyway. Might be clearer to drop it.

I think it's fine to guard against the PodAutoscalerStatus and leave this else clause

ie. if in the future PodAutoscalerStatus scale could cycle through the values -1 => some number > 0 => -1 then we'd be fine.

Or alternatively it might be better to have

rs.ActualReplicas := math.Max(ps.GetActualScale(), 0)

Doesn't work, since Math.Max works with float64 only (thanks golang). But we can write our own max (we have a few in the code already).

julz · 2021-03-26T13:37:18Z

pkg/apis/serving/v1/revision_lifecycle.go

+	}
+
+	if ps.DesiredScale != nil && *ps.DesiredScale != -1 {
+		rs.DesiredReplicas = *ps.DesiredScale


can use GetDesiredScale() helper here and avoid nil check

julz · 2021-03-26T13:38:38Z

pkg/apis/serving/v1/revision_lifecycle_test.go

+		ps: autoscalingv1alpha1.PodAutoscalerStatus{
+			DesiredScale: ptr.Int32(-1),
+		},
+		// actualReplicas: 0, desiredReplicas: 0


might as well just set the want fields I think (even if theyre technically redundant) if we're going to have the comment here anyway

julz · 2021-03-26T13:39:48Z

pkg/apis/serving/v1/revision_types.go

 	// +optional
 	ContainerStatuses []ContainerStatus `json:"containerStatuses,omitempty"`
+
+	// Reflects the PodAutoScalerStatus.ActualScale value in RevisionStatus.


Suggested change

// Reflects the PodAutoScalerStatus.ActualScale value in RevisionStatus.

// ActualReplicas reflects the PodAutoScalerStatus.ActualScale value in RevisionStatus.

julz · 2021-03-26T13:40:01Z

pkg/apis/serving/v1/revision_types.go

+
+	// Reflects the PodAutoScalerStatus.ActualScale value in RevisionStatus.
+	ActualReplicas int32 `json:"actualReplicas,omitempty"`
+	// Reflects the PodAutoScalerStatus.DesiredScale value in RevisionStatus.


Suggested change

// Reflects the PodAutoScalerStatus.DesiredScale value in RevisionStatus.

// DesiredReplicas reflects the PodAutoScalerStatus.DesiredScale value in RevisionStatus.

julz · 2021-03-26T13:41:08Z

test/e2e/helloworld_test.go

 	} else {
 		t.Fatalf("Failed to get Service name from Revision label")
 	}
+	if val := revision.Status.ActualReplicas; val != 1 {


Im wondering why we added this check to this test in particular (seems not particularly related?).

julz · 2021-03-26T13:47:17Z

test/e2e/minscale_readiness_test.go

 		t.Fatalf("The revision %q scaled to %d > %d after not being routable anymore: %v", newRevName, lr, minScale, err)
 	}
+
+	revision, err = clients.ServingClient.Revisions.Get(context.Background(), newRevName, metav1.GetOptions{})


Could this be flaky if sometimes the revision hasnt quite finished reconciling the PA status? (ie. since I think we're not waiting for that reconcile to complete in waitForDesiredScale might it be possible for waitForDesiredScale to return before ActualScale has bubbled up?)

markusthoemmes · 2021-03-26T14:07:21Z

test/e2e/autoscale.go

@@ -291,22 +290,12 @@ func assertScaleDown(ctx *TestContext) {
 func numberOfReadyPods(ctx *TestContext) (float64, error) {
 	// SKS name matches that of revision.


Suggested change

// SKS name matches that of revision.

vagababov

Do you want to change revision.yaml CRD as well to report the values on k get revision?

vagababov

stylistic issues only.

test/e2e/minscale_readiness_test.go

markusthoemmes · 2021-03-29T06:33:07Z

pkg/apis/serving/v1/revision_lifecycle_test.go

+		wantActualReplicas:  -1,
+		wantDesiredReplicas: -1,


Do we want these to surface -1 as long as the pointers are nil? Have we considered making the respective Status on the Revision pointers to ints as well?

markusthoemmes · 2021-03-29T13:29:05Z

/assign @dprotaso

For an API review.

leetcope · 2021-03-29T17:48:29Z

/retest

dprotaso · 2021-03-29T19:35:51Z

So the upgrade tests are failing due to downgrading detecting unrecognizable fields

"Failed to migrate: unable to patch resource serving-tests/pizzaplanet-post-upgrade-service-00001 (gvr: serving.knative.dev/v1, Resource=revisions) - admission webhook "webhook.serving.knative.dev" denied the request: mutation failed: cannot decode incoming new object: json: unknown field "actualReplicas""

We should discuss the options - I've made an issue here: #11065

dprotaso · 2021-03-31T15:08:08Z

/hold

API change is pulled out here: #11068

We'll want to land that and wait till after the release before continuing this PR

leetcope · 2021-04-06T03:23:40Z

/test pull-knative-serving-istio-stable-no-mesh

dprotaso · 2021-04-06T14:09:08Z

/retest

pkg/apis/serving/v1/revision_lifecycle.go

leetcope · 2021-04-06T20:11:35Z

/retest

dprotaso · 2021-04-07T14:32:30Z

/retest
/lgtm
/approve

knative-prow-robot · 2021-04-07T14:32:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso

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:

~~OWNERS~~ [dprotaso]

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

dprotaso · 2021-04-07T14:32:48Z

/test pull-knative-serving-istio-stable-no-mesh

dprotaso · 2021-04-07T15:05:58Z

/hold cancel

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Mar 26, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 26, 2021

leetcope force-pushed the revision-return-replicas branch from fc4f41b to 0a1e6af Compare March 26, 2021 06:14

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

julz reviewed Mar 26, 2021

View reviewed changes

leetcope force-pushed the revision-return-replicas branch from 0a1e6af to a09ae43 Compare March 26, 2021 14:03

markusthoemmes reviewed Mar 26, 2021

View reviewed changes

vagababov reviewed Mar 26, 2021

View reviewed changes

vagababov reviewed Mar 27, 2021

View reviewed changes

test/e2e/minscale_readiness_test.go Show resolved Hide resolved

markusthoemmes reviewed Mar 29, 2021

View reviewed changes

knative-prow-robot assigned dprotaso Mar 29, 2021

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 Mar 29, 2021

dprotaso mentioned this pull request Mar 29, 2021

consider allowing unknown fields in the webhooks #11065

Closed

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 31, 2021

leetcope added 5 commits April 5, 2021 11:53

add PodAutoScaler information to revision status

cbe8426

handle PodAutoscaler inactive for Revision status

40928ab

refactor Revision PodAutoScaler replicas status test

64668fa

refactor Revision status test to subtests

39e0514

assert new replica values for Revision in helloworld_test

048a910

leetcope added 7 commits April 5, 2021 11:53

assert new replica values for Revision in minscale_readiness_test

8235d0b

remove unnecessary output

a5ea564

remove desired replicas assertion

10c5cc1

address PR feedback

b37b9d8

reflect revision API change in CRD

5a4cc32

handle error and replica assertion in distinct checks

4c1cdc5

fix misspelled "occurred"

b2493b8

leetcope force-pushed the revision-return-replicas branch from de9916b to b2493b8 Compare April 5, 2021 17:35

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 5, 2021

dprotaso reviewed Apr 6, 2021

View reviewed changes

pkg/apis/serving/v1/revision_lifecycle.go Outdated Show resolved Hide resolved

leetcope force-pushed the revision-return-replicas branch from d2d28fa to 1aa690f Compare April 6, 2021 18:35

tidy up if blocks

7abff9f

leetcope force-pushed the revision-return-replicas branch from 1aa690f to 7abff9f Compare April 6, 2021 19:17

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 Apr 6, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2021

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

knative-prow-robot merged commit 983a307 into knative:main Apr 7, 2021

dprotaso mentioned this pull request Apr 7, 2021

kn service describe should show # of instances knative/client#842

Closed

julz mentioned this pull request Apr 15, 2021

Add julz to API Approvers knative/community#573

Merged

	// Reflects the PodAutoScalerStatus.ActualScale value in RevisionStatus.
	// ActualReplicas reflects the PodAutoScalerStatus.ActualScale value in RevisionStatus.

	// Reflects the PodAutoScalerStatus.DesiredScale value in RevisionStatus.
	// DesiredReplicas reflects the PodAutoScalerStatus.DesiredScale value in RevisionStatus.

		@@ -291,22 +290,12 @@ func assertScaleDown(ctx *TestContext) {
		func numberOfReadyPods(ctx *TestContext) (float64, error) {
		// SKS name matches that of revision.

Conversation

leetcope commented Mar 26, 2021

Proposed Changes

Uh oh!

codecov bot commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

leetcope commented Mar 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julz Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markusthoemmes commented Mar 29, 2021

Uh oh!

leetcope commented Mar 29, 2021

Uh oh!

dprotaso commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dprotaso commented Mar 31, 2021

Uh oh!

leetcope commented Apr 6, 2021

Uh oh!

dprotaso commented Apr 6, 2021

Uh oh!

Uh oh!

leetcope commented Apr 6, 2021

Uh oh!

dprotaso commented Apr 7, 2021

Uh oh!

knative-prow-robot commented Apr 7, 2021

Uh oh!

dprotaso commented Apr 7, 2021

Uh oh!

dprotaso commented Apr 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Mar 26, 2021 •

edited

Loading

dprotaso Apr 6, 2021 •

edited

Loading

julz Mar 26, 2021 •

edited

Loading

dprotaso commented Mar 29, 2021 •

edited

Loading