More robust WaitForServiceLatestRevision function for rolling upgrades #5956

mgencur · 2019-11-06T15:32:34Z

Fixes the following type of issue when running rolling-upgrade tests, this issue happens randomly:

service_postupgrade_test.go:35: Since the Service was updated a new Revision will be created and the Service will be updated\n    service_postupgrade_test.go:35: Service pizzaplanet-upgrade-service was not updated with the Revision for image pizzaplanetv2: service \"pizzaplanet-upgrade-service\" is not in desired state, got: &{TypeMeta:{Kind: APIVersion:} ObjectMeta:{Name:pizzaplanet-upgrade-service GenerateName: Namespace:serving-tests SelfLink:/apis/serving.knative.dev/v1alpha1/namespaces/serving-tests/services/pizzaplanet-upgrade-service UID:e0b1a3ff-007a-11ea-bfa4-02c1cbe14dfb ResourceVersion:27702 Generation:3 CreationTimestamp:2019-11-06 04:50:34 -0500 EST DeletionTimestamp:<nil> DeletionGracePeriodSeconds:<nil> Labels:map[] Annotations:map[serving.knative.dev/creator:kube:admin serving.knative.dev/forceUpgrade:true serving.knative.dev/lastModifier:kube:admin] OwnerReferences:[] Initializers:nil Finalizers:[] ClusterName:} Spec:{DeprecatedGeneration:0 DeprecatedRunLatest:<nil> DeprecatedPinned:<nil> DeprecatedManual:<nil> DeprecatedRelease:<nil> ConfigurationSpec:{DeprecatedGeneration:0 DeprecatedBuild:nil DeprecatedRevisionTemplate:<nil> Template:&ObjectMeta{Name:,GenerateName:,Namespace:,SelfLink:,UID:,ResourceVersion:,Generation:0,CreationTimestamp:0001-01-01 00:00:00 +0000 UTC,DeletionTimestamp:<nil>,DeletionGracePeriodSeconds:nil,Labels:map[string]string{},Annotations:map[string]string{autoscaling.knative.dev/minScale: 1,},OwnerReferences:[],Finalizers:[],ClusterName:,Initializers:nil,}} RouteSpec:{DeprecatedGeneration:0 Traffic:[{DeprecatedName: TrafficTarget:{Tag: RevisionName: ConfigurationName: LatestRevision:0xc0007a1048 Percent:0xc0007a1070 URL:}}]}} Status:{Status:{ObservedGeneration:3 Conditions:[{Type:ConfigurationsReady Status:True Severity: LastTransitionTime:{Inner:2019-11-06 04:53:45 -0500 EST} Reason: Message:} {Type:Ready Status:True Severity: LastTransitionTime:{Inner:2019-11-06 04:53:46 -0500 EST} Reason: Message:} {Type:RoutesReady Status:True Severity: LastTransitionTime:{Inner:2019-11-06 04:53:46 -0500 EST} Reason: Message:}]} RouteStatusFields:{URL:http://pizzaplanet-upgrade-service.serving-tests.apps.ocf-rollup-19-ocf-ocp-4.0-aws-rolling-upgrade.openshift-aws.rhocf-dev.com DeprecatedDomain: DeprecatedDomainInternal: Address:0xc000382760 Traffic:[{DeprecatedName: TrafficTarget:{Tag: RevisionName:pizzaplanet-upgrade-service-lqckw ConfigurationName: LatestRevision:0xc0007a109b Percent:0xc0007a10a0 URL:}}]} ConfigurationStatusFields:{LatestReadyRevisionName:pizzaplanet-upgrade-service-lqckw LatestCreatedRevisionName:pizzaplanet-upgrade-service-lqckw}}}: timed out waiting for the condition

knative-prow-robot

@mgencur: 0 warnings.

In response to this:

Fixes the following type of issue when running rolling-upgrade tests, this issue happens randomly:

service_postupgrade_test.go:35: Since the Service was updated a new Revision will be created and the Service will be updated\n    service_postupgrade_test.go:35: Service pizzaplanet-upgrade-service was not updated with the Revision for image pizzaplanetv2: service \"pizzaplanet-upgrade-service\" is not in desired state, got: &{TypeMeta:{Kind: APIVersion:} ObjectMeta:{Name:pizzaplanet-upgrade-service GenerateName: Namespace:serving-tests SelfLink:/apis/serving.knative.dev/v1alpha1/namespaces/serving-tests/services/pizzaplanet-upgrade-service UID:e0b1a3ff-007a-11ea-bfa4-02c1cbe14dfb ResourceVersion:27702 Generation:3 CreationTimestamp:2019-11-06 04:50:34 -0500 EST DeletionTimestamp:<nil> DeletionGracePeriodSeconds:<nil> Labels:map[] Annotations:map[serving.knative.dev/creator:kube:admin serving.knative.dev/forceUpgrade:true serving.knative.dev/lastModifier:kube:admin] OwnerReferences:[] Initializers:nil Finalizers:[] ClusterName:} Spec:{DeprecatedGeneration:0 DeprecatedRunLatest:<nil> DeprecatedPinned:<nil> DeprecatedManual:<nil> DeprecatedRelease:<nil> ConfigurationSpec:{DeprecatedGeneration:0 DeprecatedBuild:nil DeprecatedRevisionTemplate:<nil> Template:&ObjectMeta{Name:,GenerateName:,Namespace:,SelfLink:,UID:,ResourceVersion:,Generation:0,CreationTimestamp:0001-01-01 00:00:00 +0000 UTC,DeletionTimestamp:<nil>,DeletionGracePeriodSeconds:nil,Labels:map[string]string{},Annotations:map[string]string{autoscaling.knative.dev/minScale: 1,},OwnerReferences:[],Finalizers:[],ClusterName:,Initializers:nil,}} RouteSpec:{DeprecatedGeneration:0 Traffic:[{DeprecatedName: TrafficTarget:{Tag: RevisionName: ConfigurationName: LatestRevision:0xc0007a1048 Percent:0xc0007a1070 URL:}}]}} Status:{Status:{ObservedGeneration:3 Conditions:[{Type:ConfigurationsReady Status:True Severity: LastTransitionTime:{Inner:2019-11-06 04:53:45 -0500 EST} Reason: Message:} {Type:Ready Status:True Severity: LastTransitionTime:{Inner:2019-11-06 04:53:46 -0500 EST} Reason: Message:} {Type:RoutesReady Status:True Severity: LastTransitionTime:{Inner:2019-11-06 04:53:46 -0500 EST} Reason: Message:}]} RouteStatusFields:{URL:http://pizzaplanet-upgrade-service.serving-tests.apps.ocf-rollup-19-ocf-ocp-4.0-aws-rolling-upgrade.openshift-aws.rhocf-dev.com DeprecatedDomain: DeprecatedDomainInternal: Address:0xc000382760 Traffic:[{DeprecatedName: TrafficTarget:{Tag: RevisionName:pizzaplanet-upgrade-service-lqckw ConfigurationName: LatestRevision:0xc0007a109b Percent:0xc0007a10a0 URL:}}]} ConfigurationStatusFields:{LatestReadyRevisionName:pizzaplanet-upgrade-service-lqckw LatestCreatedRevisionName:pizzaplanet-upgrade-service-lqckw}}}: timed out waiting for the condition

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.

vagababov · 2019-11-06T22:03:11Z

test/v1/revision.go

@@ -75,6 +75,14 @@ func IsRevisionReady(r *v1.Revision) (bool, error) {
 	return r.Generation == r.Status.ObservedGeneration && r.Status.IsReady(), nil
 }

+// IsRevisionPinned will check if the revision is pinned to a route.
+func IsRevisionPinned(r *v1.Revision) (bool, error) {


It seems that if you just return false that will percolate nicely without error? (return CheckRevisionState)

Yeah. That's right. I just wanted to align the function signature with the other functions of this type so that they can be used similarly. But I don't have a problem changing that if required.

Signature is fine. But returning false rather than error would achieve same benefit, no?

Yes. I meant signature including return parameters/type. Returning false would be simpler in this case indeed.

Ehmm. Actually, the CheckRevisionState function expects the signature I have now.

Yes, don't change the signature, change the logic :)

Oooh, ok. Do you mean something like this?

// IsRevisionPinned will check if the revision is pinned to a route. func IsRevisionPinned(r *v1beta1.Revision) (bool, error) { _, pinned := r.Annotations[serving.RevisionLastPinnedAnnotationKey] return pinned, nil }

And in the call site just
return X(...), rather than if _, err := X(...); err != nil....

Pushed changes. Unfortunately, the last suggestion from you is not possible because I'm not directly calling IsRevisionPinned but I'm passing it to CheckRevisionState which, itself returns only error. But the enclosing function expects again (bool, error). So there's double conversion of return types throughout the method calls.

vagababov · 2019-11-07T08:37:23Z

/lgtm
/approve
I am annoyed that CheckRevisionState does not follow (bool, error) signature but I guess it's a different fix

knative-prow-robot · 2019-11-07T08:37:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mgencur, 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:

~~test/OWNERS~~ [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 · 2019-11-07T08:46:37Z

The following jobs failed:

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

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

test/scale.[build failed]
test/conformance/api/v1.[build failed]
test/conformance/api/v1alpha1.[build failed]
test/conformance/api/v1beta1.[build failed]
test/conformance/runtime.[build failed]
test/e2e.[build failed]
test/e2e/istio.[build failed]

mgencur · 2019-11-07T08:54:09Z

@vagababov thanks for the review, I had to do another fix after rebase. Please review.

dgerd · 2019-11-08T23:55:31Z

/lgtm

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 6, 2019

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Nov 6, 2019

knative-prow-robot reviewed Nov 6, 2019

View reviewed changes

knative-prow-robot requested review from dgerd and markusthoemmes November 6, 2019 15:33

mgencur force-pushed the stabilize_rollups branch from 7c5dfc4 to e853147 Compare November 6, 2019 15:52

vagababov reviewed Nov 6, 2019

View reviewed changes

knative-prow-robot assigned vagababov Nov 7, 2019

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

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

mgencur added 3 commits November 7, 2019 09:49

More robust WaitForServiceLatestRevision function

5ef555a

Do not return error in IsRevisionPinned

6e1a941

Fixes after rebase

68707a1

mgencur force-pushed the stabilize_rollups branch from b0e7ea7 to 68707a1 Compare November 7, 2019 08:53

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2019

knative-prow-robot assigned dgerd Nov 8, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2019

knative-prow-robot merged commit 7b6e76d into knative:master Nov 9, 2019

yanweiguo mentioned this pull request Aug 12, 2021

Conformance tests should not depend on serving.knative.dev/routingState label #11797

Closed

yanweiguo mentioned this pull request Aug 26, 2021

Wait latest ready revision without routingState label check in conformance tests #11823

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More robust WaitForServiceLatestRevision function for rolling upgrades #5956

More robust WaitForServiceLatestRevision function for rolling upgrades #5956

mgencur commented Nov 6, 2019

knative-prow-robot left a comment

vagababov Nov 6, 2019

mgencur Nov 7, 2019 •

edited

Loading

vagababov Nov 7, 2019

mgencur Nov 7, 2019

mgencur Nov 7, 2019

vagababov Nov 7, 2019

mgencur Nov 7, 2019

vagababov Nov 7, 2019

vagababov Nov 7, 2019

mgencur Nov 7, 2019

vagababov commented Nov 7, 2019

knative-prow-robot commented Nov 7, 2019

knative-test-reporter-robot commented Nov 7, 2019

mgencur commented Nov 7, 2019

dgerd commented Nov 8, 2019

More robust WaitForServiceLatestRevision function for rolling upgrades #5956

More robust WaitForServiceLatestRevision function for rolling upgrades #5956

Conversation

mgencur commented Nov 6, 2019

knative-prow-robot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgencur Nov 7, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vagababov commented Nov 7, 2019

knative-prow-robot commented Nov 7, 2019

knative-test-reporter-robot commented Nov 7, 2019

mgencur commented Nov 7, 2019

dgerd commented Nov 8, 2019

mgencur Nov 7, 2019 •

edited

Loading