From 933ff821700a8d79bf6f3d27a20e811bab1874ce Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Wed, 27 Sep 2023 18:24:56 +0200 Subject: [PATCH 01/14] reconciling the revision so the deployment status is propagated when there is something wrong (like low resources request and limits), since it was just beign propagated when the revision status was nil and the deployment existed trying to surface replicaset creation errors --- pkg/apis/serving/v1/revision_helpers.go | 7 +++++++ pkg/reconciler/revision/reconcile_resources.go | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/apis/serving/v1/revision_helpers.go b/pkg/apis/serving/v1/revision_helpers.go index e561c7ae6495..2b6d937bafea 100644 --- a/pkg/apis/serving/v1/revision_helpers.go +++ b/pkg/apis/serving/v1/revision_helpers.go @@ -19,6 +19,7 @@ package v1 import ( "time" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" net "knative.dev/networking/pkg/apis/networking" "knative.dev/pkg/kmeta" @@ -143,3 +144,9 @@ func (rs *RevisionStatus) IsActivationRequired() bool { c := revisionCondSet.Manage(rs).GetCondition(RevisionConditionActive) return c != nil && c.Status != corev1.ConditionTrue } + +// IsReplicaSetFailure returns true if the deployment replicaset is failing to create +func (rs *RevisionStatus) IsReplicaSetFailure(deploymentStatus *appsv1.DeploymentStatus) bool { + ds := serving.TransformDeploymentStatus(deploymentStatus) + return ds != nil && ds.GetCondition(serving.DeploymentConditionReplicaSetReady).IsFalse() +} diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index 4818c35b9c10..6e5e7eacddb4 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -74,7 +74,7 @@ func (c *Reconciler) reconcileDeployment(ctx context.Context, rev *v1.Revision) // The autoscaler mutates the deployment pretty often, which would cause us // to flip back and forth between Ready and Unknown every time we scale up // or down. - if !rev.Status.IsActivationRequired() { + if !rev.Status.IsActivationRequired() || rev.Status.IsReplicaSetFailure(&deployment.Status) { rev.Status.PropagateDeploymentStatus(&deployment.Status) } } From da77117a619aca896abb73a9f01ca3fc8c41f5cf Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Mon, 2 Oct 2023 19:43:55 +0200 Subject: [PATCH 02/14] added revision condition active to revision live condition set so is not nil after the revision is created --- pkg/apis/serving/v1/revision_lifecycle.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/apis/serving/v1/revision_lifecycle.go b/pkg/apis/serving/v1/revision_lifecycle.go index 48d2cbb1fb02..b0e7c2111571 100644 --- a/pkg/apis/serving/v1/revision_lifecycle.go +++ b/pkg/apis/serving/v1/revision_lifecycle.go @@ -54,6 +54,7 @@ const ( var revisionCondSet = apis.NewLivingConditionSet( RevisionConditionResourcesAvailable, RevisionConditionContainerHealthy, + RevisionConditionActive, ) // GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface. From 97a75c2a9e87237af96b95c4a0331a477708dc7a Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Tue, 3 Oct 2023 12:21:16 +0200 Subject: [PATCH 03/14] removing RevisionConditionActive from Revision ConditionSet since we can have Ready Inactive Revisions (scale to zero) * added docs and tests for this and the replicaset failure propagation --- pkg/apis/serving/v1/revision_helpers_test.go | 43 +++++++++++++++++++ pkg/apis/serving/v1/revision_lifecycle.go | 2 +- .../revision/reconcile_resources.go | 1 + 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/pkg/apis/serving/v1/revision_helpers_test.go b/pkg/apis/serving/v1/revision_helpers_test.go index 65feea02ed5f..dd95c11b4b26 100644 --- a/pkg/apis/serving/v1/revision_helpers_test.go +++ b/pkg/apis/serving/v1/revision_helpers_test.go @@ -20,6 +20,7 @@ import ( "testing" "time" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -267,3 +268,45 @@ func TestSetRoutingState(t *testing.T) { t.Error("Expected default value for unparsable annotationm but got:", got) } } + +func TestIsReplicaSetFailure(t *testing.T) { + revisionStatus := RevisionStatus{} + cases := []struct { + name string + status appsv1.DeploymentStatus + IsReplicaSetFailure bool + }{{ + name: "empty deployment status should not be a failure", + status: appsv1.DeploymentStatus{}, + }, { + name: "Ready deployment status should not be a failure", + status: appsv1.DeploymentStatus{ + Conditions: []appsv1.DeploymentCondition{{ + Type: appsv1.DeploymentAvailable, Status: corev1.ConditionTrue, + }}, + }, + }, { + name: "ReplicasetFailure true should be a failure", + status: appsv1.DeploymentStatus{ + Conditions: []appsv1.DeploymentCondition{{ + Type: appsv1.DeploymentReplicaFailure, Status: corev1.ConditionTrue, + }}, + }, + IsReplicaSetFailure: true, + }, { + name: "ReplicasetFailure false should not be a failure", + status: appsv1.DeploymentStatus{ + Conditions: []appsv1.DeploymentCondition{{ + Type: appsv1.DeploymentReplicaFailure, Status: corev1.ConditionFalse, + }}, + }, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got, want := revisionStatus.IsReplicaSetFailure(&tc.status), tc.IsReplicaSetFailure; got != want { + t.Errorf("IsReplicaSetFailure = %v, want: %v", got, want) + } + }) + } +} diff --git a/pkg/apis/serving/v1/revision_lifecycle.go b/pkg/apis/serving/v1/revision_lifecycle.go index b0e7c2111571..c51ec7383435 100644 --- a/pkg/apis/serving/v1/revision_lifecycle.go +++ b/pkg/apis/serving/v1/revision_lifecycle.go @@ -51,10 +51,10 @@ const ( ReasonProgressDeadlineExceeded = "ProgressDeadlineExceeded" ) +// RevisionConditionActive is not part of the Revision Condition set because we can have Inactive Ready Revisions (scale to zero) var revisionCondSet = apis.NewLivingConditionSet( RevisionConditionResourcesAvailable, RevisionConditionContainerHealthy, - RevisionConditionActive, ) // GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface. diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index 6e5e7eacddb4..0e94ea4040bc 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -74,6 +74,7 @@ func (c *Reconciler) reconcileDeployment(ctx context.Context, rev *v1.Revision) // The autoscaler mutates the deployment pretty often, which would cause us // to flip back and forth between Ready and Unknown every time we scale up // or down. + // If the replicaset is failing we asume its an error we have to surface if !rev.Status.IsActivationRequired() || rev.Status.IsReplicaSetFailure(&deployment.Status) { rev.Status.PropagateDeploymentStatus(&deployment.Status) } From f2a37f52dea77e8fe8d94233aa0f98dd2d3e6999 Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Tue, 3 Oct 2023 13:28:14 +0200 Subject: [PATCH 04/14] fixing lint --- pkg/reconciler/revision/reconcile_resources.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index 0e94ea4040bc..89e323e3f175 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -74,7 +74,7 @@ func (c *Reconciler) reconcileDeployment(ctx context.Context, rev *v1.Revision) // The autoscaler mutates the deployment pretty often, which would cause us // to flip back and forth between Ready and Unknown every time we scale up // or down. - // If the replicaset is failing we asume its an error we have to surface + // If the replicaset is failing we assume its an error we have to surface if !rev.Status.IsActivationRequired() || rev.Status.IsReplicaSetFailure(&deployment.Status) { rev.Status.PropagateDeploymentStatus(&deployment.Status) } From 4c39ae2a139b06baadefc0ce4078223899522f88 Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Wed, 4 Oct 2023 15:57:05 +0200 Subject: [PATCH 05/14] adjusted e2e and unit tests for the replica failure erros propagation, improved propagation logic + left todos regarding revision conditionset --- pkg/apis/serving/v1/revision_lifecycle.go | 4 ++-- pkg/reconciler/revision/reconcile_resources.go | 9 +++++++-- test/e2e/resource_quota_error_test.go | 18 +++++++++++------- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/pkg/apis/serving/v1/revision_lifecycle.go b/pkg/apis/serving/v1/revision_lifecycle.go index c51ec7383435..49115780b562 100644 --- a/pkg/apis/serving/v1/revision_lifecycle.go +++ b/pkg/apis/serving/v1/revision_lifecycle.go @@ -51,7 +51,8 @@ const ( ReasonProgressDeadlineExceeded = "ProgressDeadlineExceeded" ) -// RevisionConditionActive is not part of the Revision Condition set because we can have Inactive Ready Revisions (scale to zero) +// RevisionConditionActive is not part of the RevisionConditionSet because we can have Inactive Ready Revisions (scale to zero) +// TODO: add to the revisionCondSet or at least initialize RevisionConditionActive and deal with the edge cases var revisionCondSet = apis.NewLivingConditionSet( RevisionConditionResourcesAvailable, RevisionConditionContainerHealthy, @@ -172,7 +173,6 @@ func (rs *RevisionStatus) PropagateDeploymentStatus(original *appsv1.DeploymentS func (rs *RevisionStatus) PropagateAutoscalerStatus(ps *autoscalingv1alpha1.PodAutoscalerStatus) { // Reflect the PA status in our own. cond := ps.GetCondition(autoscalingv1alpha1.PodAutoscalerConditionReady) - rs.ActualReplicas = nil if ps.ActualScale != nil && *ps.ActualScale >= 0 { rs.ActualReplicas = ps.ActualScale diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index 89e323e3f175..ebfe4abca16d 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -74,12 +74,17 @@ func (c *Reconciler) reconcileDeployment(ctx context.Context, rev *v1.Revision) // The autoscaler mutates the deployment pretty often, which would cause us // to flip back and forth between Ready and Unknown every time we scale up // or down. - // If the replicaset is failing we assume its an error we have to surface - if !rev.Status.IsActivationRequired() || rev.Status.IsReplicaSetFailure(&deployment.Status) { + if !rev.Status.IsActivationRequired() { rev.Status.PropagateDeploymentStatus(&deployment.Status) } } + // If the replicaset is failing we assume its an error we have to surface + if rev.Status.IsReplicaSetFailure(&deployment.Status) { + rev.Status.PropagateDeploymentStatus(&deployment.Status) + return nil + } + // If a container keeps crashing (no active pods in the deployment although we want some) if *deployment.Spec.Replicas > 0 && deployment.Status.AvailableReplicas == 0 { pods, err := c.kubeclient.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{LabelSelector: metav1.FormatLabelSelector(deployment.Spec.Selector)}) diff --git a/test/e2e/resource_quota_error_test.go b/test/e2e/resource_quota_error_test.go index 690de7aaa241..708bfbf8da38 100644 --- a/test/e2e/resource_quota_error_test.go +++ b/test/e2e/resource_quota_error_test.go @@ -40,10 +40,11 @@ func TestResourceQuotaError(t *testing.T) { clients := test.Setup(t, test.Options{Namespace: "rq-test"}) const ( - errorReason = "RevisionFailed" - errorMsgScale = "Initial scale was never achieved" - errorMsgQuota = "forbidden: exceeded quota" - revisionReason = "ProgressDeadlineExceeded" + errorReason = "RevisionFailed" + containerCreating = "ContainerCreating" + errorMsgScale = "Initial scale was never achieved" + errorMsgQuota = "forbidden: exceeded quota" + revisionReason = "ProgressDeadlineExceeded" ) names := test.ResourceNames{ Service: test.ObjectNameForTest(t), @@ -87,9 +88,12 @@ func TestResourceQuotaError(t *testing.T) { if strings.Contains(cond.Message, errorMsgQuota) && cond.IsFalse() { return true, nil } + if cond.Reason == errorReason && cond.IsFalse() { + return true, nil + } t.Logf("Reason: %s ; Message: %s ; Status: %s", cond.Reason, cond.Message, cond.Status) return true, fmt.Errorf("the service %s was not marked with expected error condition (Reason=%q, Message=%q, Status=%q), but with (Reason=%q, Message=%q, Status=%q)", - names.Config, errorReason, errorMsgScale, "False", cond.Reason, cond.Message, cond.Status) + names.Config, errorReason, "", "False", cond.Reason, cond.Message, cond.Status) } return false, nil }, "ContainerUnscheduleable") @@ -113,8 +117,8 @@ func TestResourceQuotaError(t *testing.T) { err = v1test.CheckRevisionState(clients.ServingClient, revisionName, func(r *v1.Revision) (bool, error) { cond := r.Status.GetCondition(v1.RevisionConditionReady) if cond != nil { - // Can fail with either a progress deadline exceeded error or an exceeded resource quota error - if cond.Reason == revisionReason && strings.Contains(cond.Message, errorMsgScale) { + // Can fail with either a progress deadline exceeded error, container creating status, waiting for the revision to fail or an exceeded resource quota error + if (cond.Reason == revisionReason || cond.Reason == containerCreating) && cond.IsFalse() { return true, nil } if strings.Contains(cond.Message, errorMsgQuota) && cond.IsFalse() { From caf0d9e7667b0b8d5d0bf607a04c1257166a81d0 Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Tue, 17 Oct 2023 11:14:22 +0200 Subject: [PATCH 06/14] removed todo from revision lifecycle since the discussion has settled --- pkg/apis/serving/v1/revision_lifecycle.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/apis/serving/v1/revision_lifecycle.go b/pkg/apis/serving/v1/revision_lifecycle.go index 49115780b562..f95c93675aa3 100644 --- a/pkg/apis/serving/v1/revision_lifecycle.go +++ b/pkg/apis/serving/v1/revision_lifecycle.go @@ -52,7 +52,6 @@ const ( ) // RevisionConditionActive is not part of the RevisionConditionSet because we can have Inactive Ready Revisions (scale to zero) -// TODO: add to the revisionCondSet or at least initialize RevisionConditionActive and deal with the edge cases var revisionCondSet = apis.NewLivingConditionSet( RevisionConditionResourcesAvailable, RevisionConditionContainerHealthy, From d9217419e18de06fed0ee0f63415a654f27b0677 Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Tue, 17 Oct 2023 15:56:37 +0200 Subject: [PATCH 07/14] added test case for revision fast failures when it comes to replicaset failing to create --- pkg/apis/serving/v1/revision_helpers.go | 2 +- pkg/reconciler/revision/table_test.go | 18 ++++++++++++++++++ pkg/testing/v1/revision.go | 18 ++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/pkg/apis/serving/v1/revision_helpers.go b/pkg/apis/serving/v1/revision_helpers.go index 2b6d937bafea..4270f94ac518 100644 --- a/pkg/apis/serving/v1/revision_helpers.go +++ b/pkg/apis/serving/v1/revision_helpers.go @@ -145,7 +145,7 @@ func (rs *RevisionStatus) IsActivationRequired() bool { return c != nil && c.Status != corev1.ConditionTrue } -// IsReplicaSetFailure returns true if the deployment replicaset is failing to create +// IsReplicaSetFailure returns true if the deployment replicaset failed to create func (rs *RevisionStatus) IsReplicaSetFailure(deploymentStatus *appsv1.DeploymentStatus) bool { ds := serving.TransformDeploymentStatus(deploymentStatus) return ds != nil && ds.GetCondition(serving.DeploymentConditionReplicaSetReady).IsFalse() diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 498f779f554b..9da739678fa0 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -471,6 +471,24 @@ func TestReconcile(t *testing.T) { Object: pa("foo", "deploy-timeout", WithReachabilityUnreachable), }}, Key: "foo/deploy-timeout", + }, { + Name: "revision failure because replicaset and deployment failed", + // Test just define a Revision so when an error creating the replicaset happens + // this is the state that we want to have, everything been but failing due to Revision ReplicaSetFailure + Objects: []runtime.Object{ + Revision("foo", "deploy-replicaset-failure", + WithLogURL, MarkActivating("Deploying", ""), + WithRoutingState(v1.RoutingStateActive, fc), + withDefaultContainerStatuses(), + WithRevisionObservedGeneration(1), + MarkResourcesUnavailable("FailedCreate", "I ReplicaSet failed!"), + MarkContainerHealthyUnknown("Deploying"), + ), + pa("foo", "deploy-replicaset-failure", WithReachabilityUnreachable), + replicaFailureDeploy(deploy(t, "foo", "deploy-replicaset-failure"), "I ReplicaSet failed!"), + image("foo", "deploy-replicaset-failure"), + }, + Key: "foo/deploy-replicaset-failure", }, { Name: "surface replica failure", // Test the propagation of FailedCreate from Deployment. diff --git a/pkg/testing/v1/revision.go b/pkg/testing/v1/revision.go index 19fa8a2705ea..46e9c337756e 100644 --- a/pkg/testing/v1/revision.go +++ b/pkg/testing/v1/revision.go @@ -152,6 +152,18 @@ func MarkDeploying(reason string) RevisionOption { } } +func MarkContainerHealthyUnknown(reason string) RevisionOption { + return func(r *v1.Revision) { + r.Status.MarkContainerHealthyUnknown(reason, "") + } +} + +func MarkRevisionActiveUnknown(reason string) RevisionOption { + return func(r *v1.Revision) { + r.Status.MarkActiveUnknown(reason, "") + } +} + // MarkProgressDeadlineExceeded calls the method of the same name on the Revision // with the message we expect the Revision Reconciler to pass. func MarkProgressDeadlineExceeded(message string) RevisionOption { @@ -248,6 +260,12 @@ func WithRevisionPVC() RevisionOption { } } +func WithRevisionReplicaSetFailure(reason, message string) RevisionOption { + return func(r *v1.Revision) { + r.Status.MarkResourcesAvailableFalse(reason, message) + } +} + // Revision creates a revision object with given ns/name and options. func Revision(namespace, name string, ro ...RevisionOption) *v1.Revision { r := &v1.Revision{ From 158d35b8b2ca0f114f8911031d92c9e0c7f7636c Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Wed, 18 Oct 2023 10:42:43 +0200 Subject: [PATCH 08/14] fixed resource quota error, now it never waits for progress deadline and it fails fast, so removing the bits where it can go one way or another in the e2e resource_quota_error_test --- test/e2e/resource_quota_error_test.go | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/test/e2e/resource_quota_error_test.go b/test/e2e/resource_quota_error_test.go index 708bfbf8da38..eea21a51a1bc 100644 --- a/test/e2e/resource_quota_error_test.go +++ b/test/e2e/resource_quota_error_test.go @@ -40,11 +40,9 @@ func TestResourceQuotaError(t *testing.T) { clients := test.Setup(t, test.Options{Namespace: "rq-test"}) const ( - errorReason = "RevisionFailed" - containerCreating = "ContainerCreating" - errorMsgScale = "Initial scale was never achieved" - errorMsgQuota = "forbidden: exceeded quota" - revisionReason = "ProgressDeadlineExceeded" + errorReason = "RevisionFailed" + errorMsgQuota = "forbidden: exceeded quota" + revisionReason = "RevisionFailed" ) names := test.ResourceNames{ Service: test.ObjectNameForTest(t), @@ -81,13 +79,6 @@ func TestResourceQuotaError(t *testing.T) { err = v1test.WaitForServiceState(clients.ServingClient, names.Service, func(r *v1.Service) (bool, error) { cond = r.Status.GetCondition(v1.ServiceConditionConfigurationsReady) if cond != nil && !cond.IsUnknown() { - // Can fail with either a progress deadline exceeded error or an exceeded resource quota error - if strings.Contains(cond.Message, errorMsgScale) && cond.IsFalse() { - return true, nil - } - if strings.Contains(cond.Message, errorMsgQuota) && cond.IsFalse() { - return true, nil - } if cond.Reason == errorReason && cond.IsFalse() { return true, nil } @@ -117,15 +108,11 @@ func TestResourceQuotaError(t *testing.T) { err = v1test.CheckRevisionState(clients.ServingClient, revisionName, func(r *v1.Revision) (bool, error) { cond := r.Status.GetCondition(v1.RevisionConditionReady) if cond != nil { - // Can fail with either a progress deadline exceeded error, container creating status, waiting for the revision to fail or an exceeded resource quota error - if (cond.Reason == revisionReason || cond.Reason == containerCreating) && cond.IsFalse() { - return true, nil - } if strings.Contains(cond.Message, errorMsgQuota) && cond.IsFalse() { return true, nil } return true, fmt.Errorf("the revision %s was not marked with expected error condition (Reason=%q, Message=%q), but with (Reason=%q, Message=%q)", - revisionName, revisionReason, errorMsgScale, cond.Reason, cond.Message) + revisionName, revisionReason, errorMsgQuota, cond.Reason, cond.Message) } return false, nil }) From 5ff10f6287ba99297e2884c49ea5129a5806862f Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Wed, 25 Oct 2023 16:51:32 +0200 Subject: [PATCH 09/14] finishing the replicaset deployment status failure bubbling up to the revision table test --- pkg/reconciler/revision/table_test.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 9da739678fa0..385d65e0a16b 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -473,21 +473,31 @@ func TestReconcile(t *testing.T) { Key: "foo/deploy-timeout", }, { Name: "revision failure because replicaset and deployment failed", - // Test just define a Revision so when an error creating the replicaset happens - // this is the state that we want to have, everything been but failing due to Revision ReplicaSetFailure + // This test defines a Revision InProgress with status Deploying but with a + // Deployment with a ReplicaSet failure, so the wanted status update is for + // the Deployment FailedCreate error to bubble up to the Revision Objects: []runtime.Object{ Revision("foo", "deploy-replicaset-failure", WithLogURL, MarkActivating("Deploying", ""), WithRoutingState(v1.RoutingStateActive, fc), withDefaultContainerStatuses(), WithRevisionObservedGeneration(1), - MarkResourcesUnavailable("FailedCreate", "I ReplicaSet failed!"), MarkContainerHealthyUnknown("Deploying"), ), pa("foo", "deploy-replicaset-failure", WithReachabilityUnreachable), replicaFailureDeploy(deploy(t, "foo", "deploy-replicaset-failure"), "I ReplicaSet failed!"), image("foo", "deploy-replicaset-failure"), }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: Revision("foo", "deploy-replicaset-failure", + WithLogURL, MarkResourcesUnavailable("FailedCreate", "I ReplicaSet failed!"), + withDefaultContainerStatuses(), + WithRoutingState(v1.RoutingStateActive, fc), + MarkContainerHealthyUnknown("Deploying"), + WithRevisionObservedGeneration(1), + MarkActivating("Deploying", ""), + ), + }}, Key: "foo/deploy-replicaset-failure", }, { Name: "surface replica failure", From 25714c289d35499052ac4772303ea2cbe8c16c21 Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Thu, 2 Nov 2023 16:19:17 +0100 Subject: [PATCH 10/14] removed unused test methods from revision testing package --- pkg/testing/v1/revision.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/pkg/testing/v1/revision.go b/pkg/testing/v1/revision.go index 46e9c337756e..a3cd452fd9ef 100644 --- a/pkg/testing/v1/revision.go +++ b/pkg/testing/v1/revision.go @@ -158,12 +158,6 @@ func MarkContainerHealthyUnknown(reason string) RevisionOption { } } -func MarkRevisionActiveUnknown(reason string) RevisionOption { - return func(r *v1.Revision) { - r.Status.MarkActiveUnknown(reason, "") - } -} - // MarkProgressDeadlineExceeded calls the method of the same name on the Revision // with the message we expect the Revision Reconciler to pass. func MarkProgressDeadlineExceeded(message string) RevisionOption { @@ -260,12 +254,6 @@ func WithRevisionPVC() RevisionOption { } } -func WithRevisionReplicaSetFailure(reason, message string) RevisionOption { - return func(r *v1.Revision) { - r.Status.MarkResourcesAvailableFalse(reason, message) - } -} - // Revision creates a revision object with given ns/name and options. func Revision(namespace, name string, ro ...RevisionOption) *v1.Revision { r := &v1.Revision{ From 2b999c818fbbf7c3049ad5f4004b38b4547081cf Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Sun, 5 Nov 2023 14:27:39 +0100 Subject: [PATCH 11/14] adding condition to wait for container creation in the resource quota service creation test --- test/e2e/resource_quota_error_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/e2e/resource_quota_error_test.go b/test/e2e/resource_quota_error_test.go index eea21a51a1bc..d2876e9293ed 100644 --- a/test/e2e/resource_quota_error_test.go +++ b/test/e2e/resource_quota_error_test.go @@ -41,6 +41,7 @@ func TestResourceQuotaError(t *testing.T) { clients := test.Setup(t, test.Options{Namespace: "rq-test"}) const ( errorReason = "RevisionFailed" + waitReason = "ContainerCreating" errorMsgQuota = "forbidden: exceeded quota" revisionReason = "RevisionFailed" ) @@ -111,6 +112,10 @@ func TestResourceQuotaError(t *testing.T) { if strings.Contains(cond.Message, errorMsgQuota) && cond.IsFalse() { return true, nil } + // wait for the container creation + if cond.Reason == waitReason { + return false, nil + } return true, fmt.Errorf("the revision %s was not marked with expected error condition (Reason=%q, Message=%q), but with (Reason=%q, Message=%q)", revisionName, revisionReason, errorMsgQuota, cond.Reason, cond.Message) } From b02331f885b8f484bac6b55ee5623e416e733ee2 Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Tue, 7 Nov 2023 23:15:12 +0100 Subject: [PATCH 12/14] with some istio cases this could fail with progress deadline exceeded error so adding that case too --- test/e2e/resource_quota_error_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/e2e/resource_quota_error_test.go b/test/e2e/resource_quota_error_test.go index d2876e9293ed..3aa3dc20cf22 100644 --- a/test/e2e/resource_quota_error_test.go +++ b/test/e2e/resource_quota_error_test.go @@ -41,6 +41,7 @@ func TestResourceQuotaError(t *testing.T) { clients := test.Setup(t, test.Options{Namespace: "rq-test"}) const ( errorReason = "RevisionFailed" + progressDeadlineReason = "ProgressDeadlineExceeded" waitReason = "ContainerCreating" errorMsgQuota = "forbidden: exceeded quota" revisionReason = "RevisionFailed" @@ -83,6 +84,7 @@ func TestResourceQuotaError(t *testing.T) { if cond.Reason == errorReason && cond.IsFalse() { return true, nil } + if cond.Reason == t.Logf("Reason: %s ; Message: %s ; Status: %s", cond.Reason, cond.Message, cond.Status) return true, fmt.Errorf("the service %s was not marked with expected error condition (Reason=%q, Message=%q, Status=%q), but with (Reason=%q, Message=%q, Status=%q)", names.Config, errorReason, "", "False", cond.Reason, cond.Message, cond.Status) @@ -112,6 +114,10 @@ func TestResourceQuotaError(t *testing.T) { if strings.Contains(cond.Message, errorMsgQuota) && cond.IsFalse() { return true, nil } + // Can fail with either a progress deadline exceeded error + if cond.Reason == progressDeadlineReason { + return true, nil + } // wait for the container creation if cond.Reason == waitReason { return false, nil From d061fe72e97b7d5441d5fa63c295a3c1a9050763 Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Tue, 7 Nov 2023 23:25:25 +0100 Subject: [PATCH 13/14] Update resource_quota_error_test.go --- test/e2e/resource_quota_error_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/test/e2e/resource_quota_error_test.go b/test/e2e/resource_quota_error_test.go index 3aa3dc20cf22..e3c502811de8 100644 --- a/test/e2e/resource_quota_error_test.go +++ b/test/e2e/resource_quota_error_test.go @@ -84,7 +84,6 @@ func TestResourceQuotaError(t *testing.T) { if cond.Reason == errorReason && cond.IsFalse() { return true, nil } - if cond.Reason == t.Logf("Reason: %s ; Message: %s ; Status: %s", cond.Reason, cond.Message, cond.Status) return true, fmt.Errorf("the service %s was not marked with expected error condition (Reason=%q, Message=%q, Status=%q), but with (Reason=%q, Message=%q, Status=%q)", names.Config, errorReason, "", "False", cond.Reason, cond.Message, cond.Status) From 7d2d707fb7431e8f594333f8219bfa04b4b25235 Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Wed, 8 Nov 2023 01:26:57 +0100 Subject: [PATCH 14/14] formated the test file --- test/e2e/resource_quota_error_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/e2e/resource_quota_error_test.go b/test/e2e/resource_quota_error_test.go index e3c502811de8..6c433ba02b12 100644 --- a/test/e2e/resource_quota_error_test.go +++ b/test/e2e/resource_quota_error_test.go @@ -40,11 +40,11 @@ func TestResourceQuotaError(t *testing.T) { clients := test.Setup(t, test.Options{Namespace: "rq-test"}) const ( - errorReason = "RevisionFailed" + errorReason = "RevisionFailed" progressDeadlineReason = "ProgressDeadlineExceeded" - waitReason = "ContainerCreating" - errorMsgQuota = "forbidden: exceeded quota" - revisionReason = "RevisionFailed" + waitReason = "ContainerCreating" + errorMsgQuota = "forbidden: exceeded quota" + revisionReason = "RevisionFailed" ) names := test.ResourceNames{ Service: test.ObjectNameForTest(t),