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

fix: Remove LIST_LIMIT in workflow informer #9700

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

terrytangyuan
Copy link
Member

@terrytangyuan terrytangyuan commented Sep 28, 2022

Reverts part of the changes in #9691 since we need to verify correctness on pagination.

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
@terrytangyuan terrytangyuan changed the title Revert "fix: Avoid controller crashes when running large number of workflows" fix: Remove LIST_LIMIT in workflow informer Sep 28, 2022
@terrytangyuan terrytangyuan enabled auto-merge (squash) September 28, 2022 00:59
@jessesuen
Copy link
Member

jessesuen commented Sep 28, 2022

For posterity: the controller framework already uses a default limit of 500. And so previous change was unnecessarily reducing this. Also, based on log messages, I see the limit still being used 500 in some places, but my own custom value in other places, proving that the LIST_LIMIT was not working as intended:

EDIT: actually logs show it properly being used. I confused cronworkflow logs with workflow logs

$ LIST_LIMIT=10 LEADER_ELECTION_DISABLE=true go run ./cmd/workflow-controller/main.go -n argo --gloglevel 6 2> >(grep limit= | grep "workflows?")
I0927 18:13:47.423296   50187 round_trippers.go:553] GET https://host.docker.internal:51718/apis/argoproj.io/v1alpha1/workflows?labelSelector=%21workflows.argoproj.io%2Fcontroller-instanceid&limit=10&resourceVersion=0 200 OK in 11 milliseconds
I0927 18:13:47.431463   50187 round_trippers.go:553] GET https://host.docker.internal:51718/apis/argoproj.io/v1alpha1/workflows?allowWatchBookmarks=true&labelSelector=%21workflows.argoproj.io%2Fcontroller-instanceid&limit=10&resourceVersion=83227&timeoutSeconds=479&watch=true 200 OK in 2 milliseconds
I0927 18:13:47.534024   50187 round_trippers.go:553] GET https://host.docker.internal:51718/apis/argoproj.io/v1alpha1/cronworkflows?labelSelector=%21workflows.argoproj.io%2Fcontroller-instanceid&limit=500&resourceVersion=0 200 OK in 5 milliseconds
I0927 18:13:47.534087   50187 round_trippers.go:553] GET https://host.docker.internal:51718/apis/argoproj.io/v1alpha1/workflows?labelSelector=%21workflows.argoproj.io%2Fcontroller-instanceid%2Cworkflows.argoproj.io%2Fcron-workflow&limit=500&resourceVersion=0 200 OK in 5 milliseconds

@terrytangyuan terrytangyuan merged commit f0016e0 into master Sep 28, 2022
@terrytangyuan terrytangyuan deleted the revert-9691-controller-crash branch September 28, 2022 01:23
chenyangxueHDU pushed a commit to chenyangxueHDU/argo that referenced this pull request Sep 29, 2022
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: yangxue.chen <chenyangxuehdu@126.com>
chenyangxueHDU pushed a commit to chenyangxueHDU/argo that referenced this pull request Sep 29, 2022
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: yangxue.chen <chenyangxuehdu@126.com>
juchaosong pushed a commit to juchaosong/argo-workflows that referenced this pull request Nov 3, 2022
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: juchao <juchao@coscene.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants