Skip to content
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.

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

Old revision does not scale down when TargetBurstCapacity set to 0 #13487

Closed
yuzisun opened this issue Nov 19, 2022 · 8 comments · Fixed by #13501
Closed

Old revision does not scale down when TargetBurstCapacity set to 0 #13487

yuzisun opened this issue Nov 19, 2022 · 8 comments · Fixed by #13501
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Issues which should be fixed (post-triage)
Milestone

Comments

@yuzisun
Copy link

yuzisun commented Nov 19, 2022

What version of Knative?

1.7.x

Expected Behavior

After updating the knative service, the previous revision should be scaled down once the new revision is in ready state no matter targetBurstCapacity is set or not.

Actual Behavior

Both old and new revisions are running when targetBurstCapacity is set to 0 to remove the activator from the path. It works as expected when we do not set targetBurstCapacity explicitly on the annotations.

Note: This wasn't an issue in 1.0 version

Steps to Reproduce the Problem

  1. Create a Knative Service with annotation autoscaling.knative.dev/targetBurstCapacity: "0"
apiVersion: serving.knative.dev/v1
kind: Service
metadata:
  name: helloworld-go
spec:
  template:
    metadata:
      annotations:
        autoscaling.knative.dev/targetBurstCapacity: "0"
    spec:
      containers:
        - image: docker.io/yuzisun/helloworld-python
          env:
            - name: TARGET
              value: "Go Sample v1"
kubectl get revisions

NAME                                   CONFIG NAME                      K8S SERVICE NAME   GENERATION   READY   REASON             ACTUAL REPLICAS   DESIRED REPLICAS
helloworld-go-00002                    helloworld-go                                       2            True                       1                 1
  1. Update the Knative service by changing the env value, the old revision is not scaled down.
kubectl get revisions

NAME                                   CONFIG NAME                      K8S SERVICE NAME   GENERATION   READY   REASON   ACTUAL REPLICAS   DESIRED REPLICAS
helloworld-go-00002                    helloworld-go                                       2            True                       1                 1
helloworld-go-00003                    helloworld-go                                       3            True                       1                 1
kubectl get pods
NAME                                                              READY   STATUS    RESTARTS   AGE
helloworld-go-00002-deployment-7f995798dd-54jq2                   2/2     Running   0          4m27s
helloworld-go-00003-deployment-9f6997666-sfcpt                    2/2     Running   0          3m
@yuzisun yuzisun added the kind/bug Categorizes issue or PR as related to a bug. label Nov 19, 2022
@dprotaso dprotaso added this to the v1.9.0 milestone Nov 19, 2022
@dprotaso
Copy link
Member

/triage accepted

@knative-prow knative-prow bot added the triage/accepted Issues which should be fixed (post-triage) label Nov 19, 2022
@psschwei
Copy link
Contributor

Another interesting thing is that the new pod is still running too... in general, we'd expect a pod not getting requests to scale down much sooner than that (under two minutes).

@dprotaso
Copy link
Member

dprotaso commented Nov 21, 2022

Bisected it to this PR - #12033

Looks like the autoscaler tries to probe that the activator is in the path but the request is not being directed to the 'probing logic'. So it's being forwarded to the user container and being counted as regular request.

So effectively in low-TBC (to be confirmed if this affects TBC>0) this prevents scaling to 0.

Thankfully it's a simple fix but we'll probably want to cherry pick this back to earlier releases.

/assign @dprotaso

@yuzisun
Copy link
Author

yuzisun commented Nov 21, 2022

Thanks @dprotaso ! Would appreciate if you can help patch this to 1.7 release.

@dprotaso
Copy link
Member

Yeah - I'll be pulling this fix back to older releases (probably back to 1.5)

@dprotaso
Copy link
Member

@yuzisun Point releases should be out next Tuesday when the automation kicks off.

I'll post a nightly link when that becomes available if you care to verify the fix beforehand.

@yuzisun
Copy link
Author

yuzisun commented Nov 28, 2022

@yuzisun Point releases should be out next Tuesday when the automation kicks off.

I'll post a nightly link when that becomes available if you care to verify the fix beforehand.

Thanks, will check tomorrow and verify the fix.

@dprotaso
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Issues which should be fixed (post-triage)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants