cilium / cilium-cli

CLI to install, manage & troubleshoot Kubernetes clusters running Cilium
https://cilium.io
Apache License 2.0
417 stars 210 forks source link

sysdump: `--worker-count` seems to have limited effect #1459

Open glibsm opened 1 year ago

glibsm commented 1 year ago

It seems like the workerpool is sized to match the number of tasks, rather than the argument.

I added this small patch to exit the sysdump and print the value of wc which is used to side the workerpool:

diff --git a/sysdump/sysdump.go b/sysdump/sysdump.go
index 4c61a46f..4389d1a3 100644
--- a/sysdump/sysdump.go
+++ b/sysdump/sysdump.go
@@ -7,6 +7,7 @@ import (
        "context"
        "fmt"
        "io"
+       "log"
        "os"
        "path"
        "path/filepath"
@@ -1105,6 +1106,7 @@ func (c *Collector) Run() error {
        if wc < c.Options.WorkerCount {
                wc = c.Options.WorkerCount
        }
+       log.Fatal("wc:", wc)
        c.Pool = workerpool.New(wc)
        c.logDebug("Using %d workers (requested: %d)", wc, c.Options.WorkerCount)

https://github.com/cilium/cilium-cli/blob/ede083039a08fad1e36d1234dc95ab4f58e2fba4/sysdump/sysdump.go#L1108

Running it shows wc 17, and not 5 as expected.

❯ make && ./cilium sysdump --worker-count 5
CGO_ENABLED=0 go build  \
                -ldflags "-w -s \
                -X 'github.com/cilium/cilium-cli/internal/cli/cmd.Version=v0.13.1-26-g21916db6'" \
                -o cilium \
                ./cmd/cilium
🔍 Collecting sysdump with cilium-cli version: v0.13.1-26-g21916db6, args: [sysdump --worker-count 5]
🔮 Detected Cilium installation in namespace "kube-system"
🔮 Detected Cilium operator in namespace "kube-system"
🔍 Collecting Kubernetes nodes
2023/03/17 12:04:58 wc:17

There is logic that caps the min number of workers https://github.com/cilium/cilium-cli/blob/ede083039a08fad1e36d1234dc95ab4f58e2fba4/sysdump/sysdump.go#L1105 but I think it's having an adverse effect on the situation when the number of tasks is quite large (typical use-case).

Unless I'm missing some context, I think the max number of parallel workers should be capped at --worker-count.

github-actions[bot] commented 2 days ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.