Fix marking activator endpoints populated into sks condition accurately #15279

yenniechen · 2024-05-30T02:48:03Z

marking activator endpoints populated into sks condition accurately
While reconciling SKS, the reconciler will reconcile Public Endpoints and mark Activator Endpoints Populated into Condition at last. If no backends or if we're in the proxy mode, the activator will be considered to back this revision. But in fact, if no activator found, the mode will be forced into "Serve", and not be put in path. Would it be better to exclude this scenario when marking “ActivatorEndpointsPopulated” Condition?

 // Otherwise check how long SKS was in proxy mode.
 // Compute the difference between time we've been proxying with the timeout.
 // If it's positive, that's the time we need to sleep, if negative -- we
 // can scale to zero.
 pf := sks.Status.ProxyFor()
 to := cfgAS.ScaleToZeroGracePeriod - pf
 if to <= 0 {
 logger.Info("Fast path scaling to 0, in proxy mode for: ", pf)
  return desiredScale, true
 }

The Condition with type name "ActivatorEndpointsPopulated" in SKS status is used to determine whether the interval for scaling to 0 is reached. If there are no endpoints, is it best not to scale down to 0?

Release Note

NONE

…ly (knative#15278)

linux-foundation-easycla · 2024-05-30T02:48:06Z

The committers listed above are authorized under a signed CLA.

✅ login: yenniechen / name: yennie (37fe895, ba729e4, 075925f, 8c77066)

knative-prow · 2024-05-30T02:48:12Z

Welcome @yingyueshi! It looks like this is your first PR to knative/serving 🎉

knative-prow · 2024-05-30T02:48:12Z

Hi @yingyueshi. 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.

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-sigs/prow repository.

pkg/reconciler/serverlessservice/serverlessservice.go

dprotaso · 2024-05-30T19:39:35Z

/ok-to-test

skonto · 2024-05-31T11:54:49Z

So that means basically we didn't update the mode with serve mode and used only the initial sks one (proxy) thus the problem. Unit test fails because we never checked the sks status field to be false:

Extra status update for networking.internal.knative.dev/v1alpha1, Kind=ServerlessService/force/serve (-extra, +prevState):
...
          &v1alpha1.ServerlessService{
          	TypeMeta:   {},
          			Conditions: v1.Conditions{
          				{
          					Type:     "ActivatorEndpointsPopulated",
        - 					Status:   "False",
        + 					Status:   "True",
          					Severity: "Info",
          					... // 1 ignored and 2 identical fields
          				},

…activator endpoints and forcing into serve mode (knative#15278)

knative-prow · 2024-05-31T13:30:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yenniechen
Once this PR has been reviewed and has the lgtm label, please assign dsimansk for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

skonto · 2024-05-31T13:35:41Z

I think there is another case not covered:

 {
		Name: "keep in serve mode, no endpoints",
		Key:  "force/proxy",
		Objects: []runtime.Object{
			SKS("force", "proxy", markHappy, WithPubService, WithPrivateService, WithDeployRef("bar")),
			deploy("force", "bar"),
			svcpub("force", "proxy"),
			svcpriv("force", "proxy"),
			endpointspub("force", "proxy", withFilteredPorts(networking.BackendHTTPPort)),
			endpointspriv("force", "proxy" /* revision has no endpoints, force proxy mode */),
			activatorEndpoints(),
		},

		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
			Object: SKS("force", "proxy", WithPubService, WithPrivateService, WithDeployRef("bar"),
				// Changes from above.
				markNoEndpoints),
		}},
	},

This will pass by setting:

    logger.go:146: 2024-05-31T16:22:16.718+0300	DEBUG	serverlessservice/reconciler.go:333	Updating status with:   v1alpha1.ServerlessServiceStatus{
          	Status: v1.Status{
          		ObservedGeneration: 0,
          		Conditions: v1.Conditions{
          			{
          				Type:               "ActivatorEndpointsPopulated",
        - 				Status:             "False",
        + 				Status:             "True",

I see we accept that condition in one similar test: "OnCreate-no-activator-eps-proxy", where activator does not have endpoints. Any reason why is that, what does that mean when we dont have any endpoints?

…native#15278)

yenniechen · 2024-05-31T14:57:38Z

So that means basically we didn't update the mode with serve mode and used only the initial sks one (proxy) thus the problem. Unit test fails because we never checked the sks status field to be false:

Extra status update for networking.internal.knative.dev/v1alpha1, Kind=ServerlessService/force/serve (-extra, +prevState):
...
          &v1alpha1.ServerlessService{
          	TypeMeta:   {},
          			Conditions: v1.Conditions{
          				{
          					Type:     "ActivatorEndpointsPopulated",
        - 					Status:   "False",
        + 					Status:   "True",
          					Severity: "Info",
          					... // 1 ignored and 2 identical fields
          				},

You are right. We can see that above Condition (with type name "ActivatorEndpointsPopulated") is used to determine whether the interval for scaling to 0 is reached. If there are no endpoints, is it best not to scale down to 0?

 // Otherwise check how long SKS was in proxy mode.
 // Compute the difference between time we've been proxying with the timeout.
 // If it's positive, that's the time we need to sleep, if negative -- we
 // can scale to zero.
 pf := sks.Status.ProxyFor()
 to := cfgAS.ScaleToZeroGracePeriod - pf
 if to <= 0 {
 logger.Info("Fast path scaling to 0, in proxy mode for: ", pf)
   return desiredScale, true
 }

 // ProxyFor returns how long it has been since Activator was moved
 // to the request path.
 func (sss *ServerlessServiceStatus) ProxyFor() time.Duration {
 	cond := sss.GetCondition(ActivatorEndpointsPopulated)
 	if cond == nil || cond.Status != corev1.ConditionTrue {
 		return 0
 	}
 	return time.Since(cond.LastTransitionTime.Inner.Time)
  }

If no activator endpoints, but have revisions endpoints, is it better not scale to Zero while no traffic? If we scale to Zero at that time, maybe we will throw some traffic, for neither activator endpoints nor revision endpoints are added to Public Service.

In the case of no activator endpoints and revision endpoints, it is Zero endpoint and no need to scale to Zero any more. Whether marking activator populated or not, it seems no effect. Is it better to mark it or not that just follow the facts that whether activator is really in the path?

In addition, I searched the code globally and found that this SKS condition (with type name "ActivatorEndpointsPopulated") is only used to compute grace period of scaling to Zero.

…as endpoints (knative#15278)

codecov · 2024-06-02T23:38:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.78%. Comparing base (c2d0af1) to head (8c77066).
Report is 146 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15279      +/-   ##
==========================================
+ Coverage   84.11%   84.78%   +0.66%     
==========================================
  Files         213      218       +5     
  Lines       16783    13480    -3303     
==========================================
- Hits        14117    11429    -2688     
+ Misses       2315     1685     -630     
- Partials      351      366      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

skonto · 2024-06-03T11:57:33Z

For the case where TBC != -1 there is logic that will not allow to for the scale down:

		if resolveTBC(ctx, pa) != -1 {
			// if TBC is -1 activator is guaranteed to already be in the path.
			// Otherwise, probe to make sure Activator is in path.
			r, err = ks.activatorProbe(pa, ks.transport)
			logger.Infof("Probing activator = %v, err = %v", r, err)
		}

if r is false then it is not going to call ProxyFor(). For the case where TBC = -1 though I think we will scale down to zero while activator might not be around and endpoints are populated (without this PR). I think we should not scale down if activator is not in the path right (when desiredScale is 0), @dprotaso wdyth?

yenniechen · 2024-06-04T08:10:34Z

I also wonder the answer.
For the case TBC=-1 and no ready activator endpoints, with this PR, the handle of scale to 0 will be requeue until activator becomes ready. Once the activator has ready endpoints, SKS controller will watch the change and inform the SKS to mark activator populated, then the revision pods will be scaled to 0.
How would it be desinged？

skonto · 2024-06-14T07:54:24Z

@dprotaso wdyth?

skonto · 2024-06-19T07:50:20Z

@dprotaso gentle ping.

dprotaso · 2024-06-25T19:17:57Z

I think we should not scale down if activator is not in the path right (when desiredScale is 0), @dprotaso wdyth?

Yeah I agree

dprotaso · 2024-06-25T19:22:13Z

I think we should update the PR so that we are only marking ActivatorEndpointsPopulated when we are actually wiring the activator in the data path with ready endpoints.

dprotaso · 2024-06-25T19:22:48Z

Also thanks for the investigation everyone - it helped me be more informed on the path to take here.

Fix marking activator endpoints populated into sks condition accurate…

ba729e4

…ly (knative#15278)

knative-prow bot requested review from izabelacg and skonto May 30, 2024 02:48

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 30, 2024

yenniechen changed the title ~~Fix marking activator endpoints populated into sks condition accurately (##15278)~~ Fix marking activator endpoints populated into sks condition accurately May 30, 2024

dprotaso reviewed May 30, 2024

View reviewed changes

pkg/reconciler/serverlessservice/serverlessservice.go Show resolved Hide resolved

knative-prow bot 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 May 30, 2024

Fix a unit test of sks, remove activator populated condition when no …

37fe895

…activator endpoints and forcing into serve mode (knative#15278)

yenniechen requested a review from dprotaso May 31, 2024 13:32

Add comment for handling scale to zero while no activator endpoints (k…

075925f

…native#15278)

knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 31, 2024

do not mark activator populated when neither revision nor activator h…

8c77066

…as endpoints (knative#15278)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix marking activator endpoints populated into sks condition accurately #15279

Fix marking activator endpoints populated into sks condition accurately #15279

yenniechen commented May 30, 2024 •

edited by dprotaso

Loading

linux-foundation-easycla bot commented May 30, 2024 •

edited

Loading

knative-prow bot commented May 30, 2024

knative-prow bot commented May 30, 2024

dprotaso commented May 30, 2024

skonto commented May 31, 2024 •

edited

Loading

knative-prow bot commented May 31, 2024

skonto commented May 31, 2024 •

edited

Loading

yenniechen commented May 31, 2024 •

edited

Loading

codecov bot commented Jun 2, 2024

skonto commented Jun 3, 2024 •

edited

Loading

yenniechen commented Jun 4, 2024 •

edited

Loading

skonto commented Jun 14, 2024

skonto commented Jun 19, 2024

dprotaso commented Jun 25, 2024

dprotaso commented Jun 25, 2024

dprotaso commented Jun 25, 2024

Fix marking activator endpoints populated into sks condition accurately #15279

Are you sure you want to change the base?

Fix marking activator endpoints populated into sks condition accurately #15279

Conversation

yenniechen commented May 30, 2024 • edited by dprotaso Loading

linux-foundation-easycla bot commented May 30, 2024 • edited Loading

knative-prow bot commented May 30, 2024

knative-prow bot commented May 30, 2024

dprotaso commented May 30, 2024

skonto commented May 31, 2024 • edited Loading

knative-prow bot commented May 31, 2024

skonto commented May 31, 2024 • edited Loading

yenniechen commented May 31, 2024 • edited Loading

codecov bot commented Jun 2, 2024

Codecov Report

skonto commented Jun 3, 2024 • edited Loading

yenniechen commented Jun 4, 2024 • edited Loading

skonto commented Jun 14, 2024

skonto commented Jun 19, 2024

dprotaso commented Jun 25, 2024

dprotaso commented Jun 25, 2024

dprotaso commented Jun 25, 2024

yenniechen commented May 30, 2024 •

edited by dprotaso

Loading

linux-foundation-easycla bot commented May 30, 2024 •

edited

Loading

skonto commented May 31, 2024 •

edited

Loading

skonto commented May 31, 2024 •

edited

Loading

yenniechen commented May 31, 2024 •

edited

Loading

skonto commented Jun 3, 2024 •

edited

Loading

yenniechen commented Jun 4, 2024 •

edited

Loading