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

Closed
houshengbo wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
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
May 15, 2025
0cb7ff8
Added the e2e test verification to make sure pod number reaches minScale
May 16, 2025
1bbcee7
Used the correct name for the revision
May 16, 2025
797b64c
Removed the unused function IsRevisionContainerHealthy
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
Prev Previous commit
Next Next commit
Added the e2e test verification to make sure pod number reaches minScale
  • Loading branch information
Vincent Hou committed May 16, 2025
commit 0cb7ff84eeecbafae2446942e398574aaef68cf9
30 changes: 30 additions & 0 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,6 +85,21 @@
revName := latestRevisionName(t, clients, names.Config, "")
serviceName := PrivateServiceName(t, clients, revName)

t.Log("Waiting for new revision's container to become healthy with the minScale")
if err := v1test.WaitForRevisionState(
clients.ServingClient, newRevName, v1test.IsRevisionContainerHealthy, "ContainerIsHealthy",

Check failure on line 90 in test/e2e/minscale_readiness_test.go

View workflow job for this annotation

GitHub Actions / build / Build 

undefined: newRevName

Check failure on line 90 in test/e2e/minscale_readiness_test.go

View workflow job for this annotation

GitHub Actions / style / Golang / Lint 

undefined: newRevName
); err != nil {
t.Fatalf("The container of the Revision %q did not become healthy: %v", newRevName, err)

Check failure on line 92 in test/e2e/minscale_readiness_test.go

View workflow job for this annotation

GitHub Actions / build / Build 

undefined: newRevName

Check failure on line 92 in test/e2e/minscale_readiness_test.go

View workflow job for this annotation

GitHub Actions / style / Golang / Lint 

undefined: newRevName
}

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

@dprotaso dprotaso May 22, 2025

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

@dprotaso dprotaso May 22, 2025

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)
Expand All @@ -102,7 +117,7 @@
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{})

Check failure on line 120 in test/e2e/minscale_readiness_test.go

View workflow job for this annotation

GitHub Actions / build / Build 

no new variables on left side of :=

Check failure on line 120 in test/e2e/minscale_readiness_test.go

View workflow job for this annotation

GitHub Actions / style / Golang / Lint 

no new variables on left side of :=
if err != nil {
t.Fatalf("An error occurred getting revision %v, %v", revName, err)
}
Expand All @@ -118,6 +133,21 @@
newRevName := latestRevisionName(t, clients, names.Config, revName)
newServiceName := PrivateServiceName(t, clients, newRevName)

t.Log("Waiting for new revision's container to become healthy with the minScale")
if err := v1test.WaitForRevisionState(
clients.ServingClient, newRevName, v1test.IsRevisionContainerHealthy, "ContainerIsHealthy",
); err != nil {
t.Fatalf("The container of the Revision %q did not become healthy: %v", newRevName, err)
}

revision, err := clients.ServingClient.Revisions.Get(context.Background(), revName, metav1.GetOptions{})

Check failure on line 143 in test/e2e/minscale_readiness_test.go

View workflow job for this annotation

GitHub Actions / build / Build 

no new variables on left side of :=

Check failure on line 143 in test/e2e/minscale_readiness_test.go

View workflow job for this annotation

GitHub Actions / style / Golang / Lint 

no new variables on left side of := (typecheck)
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)
}

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)
Expand Down
5 changes: 5 additions & 0 deletions test/v1/revision.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@ func IsRevisionReady(r *v1.Revision) (bool, error) {
return r.IsReady(), nil
}

// IsRevisionContainerHealthy will indicate whether the revision has marked the container healthy.
func IsRevisionContainerHealthy(r *v1.Revision) (bool, error) {
return r.Status.GetCondition(v1.RevisionConditionContainerHealthy).IsTrue(), nil
}

// IsRevisionFailed will check the status condition sof the revision and return true if the revision is
// marked as failed, otherwise it will return false.
func IsRevisionFailed(r *v1.Revision) (bool, error) {
Expand Down
Loading