Skip to content

Revision sets to True when deploy has the minimum number #15890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Revision sets to True when deploy has the minimum number #15890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
609adac
Revision sets to True when deploy has the minimum number
houshengbo May 15, 2025
0cb7ff8
Added the e2e test verification to make sure pod number reaches minScale
houshengbo May 16, 2025
1bbcee7
Used the correct name for the revision
houshengbo May 16, 2025
797b64c
Removed the unused function IsRevisionContainerHealthy
houshengbo May 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/reconciler/revision/reconcile_resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ func (c *Reconciler) reconcileDeployment(ctx context.Context, rev *v1.Revision)
}
}

if deployment.Status.ReadyReplicas > 0 {
if deployment.Spec.Replicas != nil && deployment.Status.ReadyReplicas >= *deployment.Spec.Replicas {
rev.Status.MarkContainerHealthyTrue()
}

Expand Down
40 changes: 28 additions & 12 deletions test/e2e/minscale_readiness_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,24 +85,32 @@ func TestMinScale(t *testing.T) {
revName := latestRevisionName(t, clients, names.Config, "")
serviceName := PrivateServiceName(t, clients, revName)

t.Log("Waiting for revision to scale to minScale before becoming ready")
if lr, err := waitForDesiredScale(clients, serviceName, gte(minScale)); err != nil {
t.Fatalf("The revision %q scaled to %d < %d before becoming ready: %v", revName, lr, minScale, err)
}

t.Log("Waiting for revision to become ready")
t.Log("Waiting for new revision to become ready")
if err := v1test.WaitForRevisionState(
clients.ServingClient, revName, v1test.IsRevisionReady, "RevisionIsReady",
); err != nil {
t.Fatalf("The Revision %q did not become ready: %v", revName, err)
}

revision, err := clients.ServingClient.Revisions.Get(context.Background(), revName, metav1.GetOptions{})
if err != nil {
t.Fatalf("An error occurred getting revision %v, %v", revName, err)
}
if replicas := *revision.Status.ActualReplicas; replicas < minScale {
t.Fatalf("Container is indicated as healthy. Expected actual replicas for revision %v to be %v but got %v", revision.Name, minScale, replicas)
}
Comment on lines +99 to +101
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hitting this condition when running locally against a broken serving release. I wouldn't expect that so it makes me think there's a delay when ActualReplicas is updated.

This fails sometimes when scaling up the first revision or second. Because of that I don't think we can reliably use this value in the test.

This is probably a separate issue to the one you're addressing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is also duplicated on line 117


t.Log("Waiting for revision to scale to minScale before becoming ready")
if lr, err := waitForDesiredScale(clients, serviceName, gte(minScale)); err != nil {
t.Fatalf("The revision %q scaled to %d < %d before becoming ready: %v", revName, lr, minScale, err)
}

t.Log("Holding revision at minScale after becoming ready")
if lr, ok := ensureDesiredScale(clients, t, serviceName, gte(minScale)); !ok {
t.Fatalf("The revision %q observed scale %d < %d after becoming ready", revName, lr, minScale)
}

revision, err := clients.ServingClient.Revisions.Get(context.Background(), revName, metav1.GetOptions{})
revision, err = clients.ServingClient.Revisions.Get(context.Background(), revName, metav1.GetOptions{})
if err != nil {
t.Fatalf("An error occurred getting revision %v, %v", revName, err)
}
Expand All @@ -118,18 +126,26 @@ func TestMinScale(t *testing.T) {
newRevName := latestRevisionName(t, clients, names.Config, revName)
newServiceName := PrivateServiceName(t, clients, newRevName)

t.Log("Waiting for new revision to scale to minScale after update")
if lr, err := waitForDesiredScale(clients, newServiceName, gte(minScale)); err != nil {
t.Fatalf("The revision %q scaled to %d < %d after creating route: %v", newRevName, lr, minScale, err)
}

t.Log("Waiting for new revision to become ready")
if err := v1test.WaitForRevisionState(
clients.ServingClient, newRevName, v1test.IsRevisionReady, "RevisionIsReady",
); err != nil {
t.Fatalf("The Revision %q did not become ready: %v", newRevName, err)
}

revision, err = clients.ServingClient.Revisions.Get(context.Background(), newRevName, metav1.GetOptions{})
if err != nil {
t.Fatalf("An error occurred getting revision %v, %v", newRevName, err)
}
if replicas := *revision.Status.ActualReplicas; replicas < minScale {
t.Fatalf("Container is indicated as healthy. Expected actual replicas for revision %v to be %v but got %v", revision.Name, minScale, replicas)
}

t.Log("Waiting for new revision to scale to minScale after update")
if lr, err := waitForDesiredScale(clients, newServiceName, gte(minScale)); err != nil {
t.Fatalf("The revision %q scaled to %d < %d after creating route: %v", newRevName, lr, minScale, err)
}

t.Log("Holding new revision at minScale after becoming ready")
if lr, ok := ensureDesiredScale(clients, t, newServiceName, gte(minScale)); !ok {
t.Fatalf("The revision %q observed scale %d < %d after becoming ready", newRevName, lr, minScale)
Expand Down
Loading