Verify ready replicas in e2e test TestMinScale by MIBc · Pull Request #6783 · knative/serving

MIBc · 2020-02-08T09:04:57Z

knative-prow-robot · 2020-02-08T09:05:12Z

Hi @MIBc. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 · 2020-02-08T18:58:40Z

/ok-to-test

markusthoemmes · 2020-02-10T07:58:49Z

@MIBc thanks for doing this. The original issue contains a note that says:

we also need to verify that before the revision is marked ready the number of ready endpoints is also at least N.

Have you taken that into account? Seems like we're only checking ReadyReplicas from the deployment but all of our components actually key-off of the Ready addresses of the revision's private service.

markusthoemmes · 2020-02-10T07:59:04Z

/assign @vagababov @tanzeeb

tanzeeb · 2020-02-10T13:08:27Z

/lgtm

The test may have occasional flakes (#5255), but they'll hopefully be resolved by the fix for #6733.

MIBc · 2020-02-10T15:09:31Z

I have read the note carefully. May be we can check the endponit number of revision before it is ready. @markusthoemmes

vagababov · 2020-02-10T16:44:51Z

Yes, please check endpoints, since that what is drives internal logic, rather than deployment state.

markusthoemmes

Thanks, I think this is almost there! Some small touchups from my end.

markusthoemmes · 2020-02-11T16:32:03Z

+	}
+
 	// Revision becomes ready
+	t.Log("Waiting for revision becomes ready")


Suggested change

t.Log("Waiting for revision becomes ready")

t.Log("Waiting for revision to become ready")

markusthoemmes · 2020-02-11T16:32:12Z

 	}

 	// Route becomes ready
+	t.Log("Waiting for route becomes ready")


Suggested change

t.Log("Waiting for route becomes ready")

t.Log("Waiting for route to become ready")

markusthoemmes · 2020-02-11T16:36:14Z


+	// Before becoming ready, observe revision endpoints
+	endpointName := revName + "-private"
+	t.Log("Waiting for ready endponits to scale to minScale before revison becoming ready")


Suggested change

t.Log("Waiting for ready endponits to scale to minScale before revison becoming ready")

t.Log("Waiting for ready endpoints to scale to minScale before the revision becomes ready")

markusthoemmes · 2020-02-11T16:37:13Z

 	})
 }
+
+func waitForDesiredEndpoints(t *testing.T, clients *test.Clients, endpointName string, cond func(int32) bool) error {


I think we can even go as far as using this as waitForDesiredScale instead of adding these calls in addition.

markusthoemmes · 2020-02-11T16:37:39Z

+func waitForDesiredEndpoints(t *testing.T, clients *test.Clients, endpointName string, cond func(int32) bool) error {
+	endpoints := clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace)
+
+	return wait.PollImmediate(time.Second, 10*time.Minute, func() (bool, error) {


Suggested change

return wait.PollImmediate(time.Second, 10*time.Minute, func() (bool, error) {

return wait.PollImmediate(time.Second, 1*time.Minute, func() (bool, error) {

To stay the same as above?

markusthoemmes · 2020-02-11T16:39:07Z

 	}

+	// Before becoming ready, observe revision endpoints
+	endpointName := revName + "-private"


Can we fetch this via API? See

serving/test/e2e/autoscale_test.go

Lines 283 to 302 in afbd55a

func numberOfReadyPods(ctx *testContext) (float64, error) {

// SKS name matches that of revision.

n := ctx.resources.Revision.Name

sks, err := ctx.clients.NetworkingClient.ServerlessServices.Get(n, metav1.GetOptions{})

if err != nil {

ctx.t.Logf("Error getting SKS %q: %v", n, err)

return 0, fmt.Errorf("error retrieving sks %q: %w", n, err)

}

if sks.Status.PrivateServiceName == "" {

ctx.t.Logf("SKS %s has not yet reconciled", n)

// Not an error, but no pods either.

return 0, nil

}

eps, err := ctx.clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace).Get(

sks.Status.PrivateServiceName, metav1.GetOptions{})

if err != nil {

return 0, fmt.Errorf("failed to get endpoints %s: %w", sks.Status.PrivateServiceName, err)

}

return float64(resources.ReadyAddressCount(eps)), nil

}

for an example of how to do this.

markusthoemmes · 2020-02-11T16:39:52Z

+		if len(endpoint.Subsets) == 0 || len(endpoint.Subsets[0].Addresses) == 0 {
+			return false, nil
+		}
+		return cond(int32(len(endpoint.Subsets[0].Addresses))), nil


I think this should use the resources.ReadyAddressCount helper. See

serving/test/e2e/autoscale_test.go

Lines 283 to 302 in afbd55a

func numberOfReadyPods(ctx *testContext) (float64, error) {

// SKS name matches that of revision.

n := ctx.resources.Revision.Name

sks, err := ctx.clients.NetworkingClient.ServerlessServices.Get(n, metav1.GetOptions{})

if err != nil {

ctx.t.Logf("Error getting SKS %q: %v", n, err)

return 0, fmt.Errorf("error retrieving sks %q: %w", n, err)

}

if sks.Status.PrivateServiceName == "" {

ctx.t.Logf("SKS %s has not yet reconciled", n)

// Not an error, but no pods either.

return 0, nil

}

eps, err := ctx.clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace).Get(

sks.Status.PrivateServiceName, metav1.GetOptions{})

if err != nil {

return 0, fmt.Errorf("failed to get endpoints %s: %w", sks.Status.PrivateServiceName, err)

}

return float64(resources.ReadyAddressCount(eps)), nil

}

for an example.

Thanks for reviewing. I will improve it.

MIBc · 2020-02-12T04:31:21Z

/retest

vagababov · 2020-02-12T06:32:15Z


+	sks, err := clients.NetworkingClient.ServerlessServices.Get(revName, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Error retrieving sks %q: %w", revName, err)


%w works only with fmt.Errorf.

Suggested change

t.Fatalf("Error retrieving sks %q: %w", revName, err)

t.Fatalf("Error retrieving sks %q: %v", revName, err)

markusthoemmes

/lgtm
/approve

Thanks for the changes!

/hold

for the outstanding nits.

markusthoemmes · 2020-02-12T07:16:44Z

+	endpoints := clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace)

 	return wait.PollImmediate(time.Second, 1*time.Minute, func() (bool, error) {
 		deployment, err := deployments.Get(deploymentName, metav1.GetOptions{})


I don't think fetching the deployment is actually required anymore. Shall we remove all mentions of it and redo the error messages above to not include it as well?

Yes. If endpoints satisfy the scale, the deploy satisfies too.

markusthoemmes · 2020-02-12T07:18:22Z

 	revName := latestRevisionName(t, clients, names.Config)
 	deploymentName := revName + "-deployment"

+	sks, err := clients.NetworkingClient.ServerlessServices.Get(revName, metav1.GetOptions{})


Oops, per the test failure this likely needs a loop to wait for the SKS to actually contain a PrivateServiceName and be there.

Yes. I have find that.

knative-test-reporter-robot · 2020-02-12T12:48:42Z

The following jobs failed:

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

Automatically retrying due to test flakiness...
/test pull-knative-serving-integration-tests

MIBc · 2020-02-12T13:34:06Z

/retest

markusthoemmes

I wanted to let the int64 nit pass and merge, but the SKS poll runs at a danger of returning a wrong result so... sorry but here are my last two comments hopefully 😂 . Thanks for being patient!

markusthoemmes · 2020-02-12T13:41:15Z

 		}
-
-		return cond(*deployment.Spec.Replicas), nil
+		return cond(int32(resources.ReadyAddressCount(endpoint))), nil


Nit: Let's change the cond funcs to just take int64 to avoid unnecessary casts (doesn't matter performance wise, but will pollute the code eventually)

markusthoemmes · 2020-02-12T13:45:48Z

+	if err != nil {
+		t.Fatalf("Failed to get sks after it was seen to be live: %v", err)
+	}
+	return sks.Status.PrivateServiceName


Is this guaranteed to be set? 🤔 AFAIK we need to go through at least one reconcilation cycle to set this. I'd propose to cache it from the above like so:

func serverlessServicesName(t *testing.T, clients *test.Clients, revisionName string) string { var privateServiceName string if err := wait.PollImmediate(time.Second, 1*time.Minute, func() (bool, error) { _, err := clients.NetworkingClient.ServerlessServices.Get(revisionName, metav1.GetOptions{}) if err != nil { return false, nil } privateServiceName = sks.Status.PrivateServiceName if privateServiceName == "" { return false, nil } return true, nil }); err != nil { t.Fatalf("Error retrieving sks %q: %v", revisionName, err) } return privateServiceName }

That should make this safe against potential inconsistencies. I'd also bump the poll timeout to 1 minute here probably.

Fixex knative#6716 * Waiting for ready endponits to scale to minScale * Move `teardown` function in front of the `Fatalf`. Otherwise the k8s resources may not delete.

markusthoemmes

/lgtm
/approve

Noice! 🎉

knative-prow-robot · 2020-02-12T15:26:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes, MIBc

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:

~~test/OWNERS~~ [markusthoemmes]

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

markusthoemmes · 2020-02-12T15:39:14Z

/hold cancel

knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 8, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 8, 2020

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 8, 2020

knative-prow-robot requested review from chaodaiG and coryrc February 8, 2020 09:05

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Feb 8, 2020

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 8, 2020

knative-prow-robot assigned tanzeeb and vagababov Feb 10, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 10, 2020

MIBc force-pushed the e2e branch from cb6c4a0 to a3f9cca Compare February 11, 2020 08:50

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 11, 2020

markusthoemmes reviewed Feb 11, 2020

View reviewed changes

MIBc force-pushed the e2e branch from a3f9cca to 54a7d57 Compare February 12, 2020 03:36

vagababov reviewed Feb 12, 2020

View reviewed changes

markusthoemmes approved these changes Feb 12, 2020

View reviewed changes

knative-prow-robot assigned markusthoemmes Feb 12, 2020

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Feb 12, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2020

markusthoemmes reviewed Feb 12, 2020

View reviewed changes

MIBc force-pushed the e2e branch from 54a7d57 to 6a5554a Compare February 12, 2020 12:08

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2020

markusthoemmes reviewed Feb 12, 2020

View reviewed changes

Verify ready replicas in e2e test TestMinScale

52379bf

Fixex knative#6716 * Waiting for ready endponits to scale to minScale * Move `teardown` function in front of the `Fatalf`. Otherwise the k8s resources may not delete.

MIBc force-pushed the e2e branch from 6a5554a to 52379bf Compare February 12, 2020 15:21

markusthoemmes approved these changes Feb 12, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2020

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

knative-prow-robot merged commit d466565 into knative:master Feb 12, 2020

MIBc deleted the e2e branch February 13, 2020 07:14

MIBc mentioned this pull request Aug 28, 2020

REQUEST: New membership for MIBc knative/community#243

Closed

	t.Log("Waiting for revision becomes ready")
	t.Log("Waiting for revision to become ready")

	t.Log("Waiting for route becomes ready")
	t.Log("Waiting for route to become ready")

	t.Log("Waiting for ready endponits to scale to minScale before revison becoming ready")
	t.Log("Waiting for ready endpoints to scale to minScale before the revision becomes ready")

	return wait.PollImmediate(time.Second, 10*time.Minute, func() (bool, error) {
	return wait.PollImmediate(time.Second, 1*time.Minute, func() (bool, error) {

	func numberOfReadyPods(ctx *testContext) (float64, error) {
	// SKS name matches that of revision.
	n := ctx.resources.Revision.Name
	sks, err := ctx.clients.NetworkingClient.ServerlessServices.Get(n, metav1.GetOptions{})
	if err != nil {
	ctx.t.Logf("Error getting SKS %q: %v", n, err)
	return 0, fmt.Errorf("error retrieving sks %q: %w", n, err)
	}
	if sks.Status.PrivateServiceName == "" {
	ctx.t.Logf("SKS %s has not yet reconciled", n)
	// Not an error, but no pods either.
	return 0, nil
	}
	eps, err := ctx.clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace).Get(
	sks.Status.PrivateServiceName, metav1.GetOptions{})
	if err != nil {
	return 0, fmt.Errorf("failed to get endpoints %s: %w", sks.Status.PrivateServiceName, err)
	}
	return float64(resources.ReadyAddressCount(eps)), nil
	}

	t.Fatalf("Error retrieving sks %q: %w", revName, err)
	t.Fatalf("Error retrieving sks %q: %v", revName, err)

Conversation

MIBc commented Feb 8, 2020

Uh oh!

knative-prow-robot commented Feb 8, 2020

Uh oh!

vagababov commented Feb 8, 2020

Uh oh!

markusthoemmes commented Feb 10, 2020

Uh oh!

markusthoemmes commented Feb 10, 2020

Uh oh!

tanzeeb commented Feb 10, 2020

Uh oh!

MIBc commented Feb 10, 2020

Uh oh!

vagababov commented Feb 10, 2020

Uh oh!

markusthoemmes left a comment

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!

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!

MIBc commented Feb 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markusthoemmes left a comment

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!

knative-test-reporter-robot commented Feb 12, 2020

Uh oh!

MIBc commented Feb 12, 2020

Uh oh!

markusthoemmes left a comment

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!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Feb 12, 2020

Uh oh!

markusthoemmes commented Feb 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants