Repository navigation
Optimize list child actions operation - #8279
Sovietaced wants to merge 3 commits into
Conversation
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
3857ac6 to
31920c6
Compare
There was a problem hiding this comment.
cloud-verifier · ⚠️ 1 finding, not blocking
🟠 parentLabelQueries hides pre-existing unlabeled children from watch snapshot · actions/k8s/client.go:431 · medium · medium likelihood · see inline comment
Reviewed 31920c6973 · turn 1 · re-run with /cloud-verifier review
| } | ||
| } | ||
| for i := range items.Items { | ||
| action := &items.Items[i] |
There was a problem hiding this comment.
🟠 parentLabelQueries hides pre-existing unlabeled children from watch snapshot
With the flag on, the spec-based parent check is skipped, so actions without the new label are dropped from the result. Consider a backfill or fallback for legacy actions.
There was a problem hiding this comment.
That's why the flag defaults to false, it requires operators to be sure that task actions have the new labels. It is documented as such.
|
I think you shoudl not need feature flag i feel. as you always get the parent name in every request and it seems it is just dropped today right |
There is no existing label which can be used to query the k8s with a filter to get only the |
Tracking issue
Closes #8282
Why are the changes needed?
The
ListChildActionsoperations enumerates over every singleTaskActionin a run to determine the child actions for a parent which can explode memory usage for high scale + fanout runs.What changes were proposed in this pull request?
Add a feature flag to filter
TaskActionsbased on a new parent task action label such that filtering is done on the server side instead of the client side in application code. This reduces CPU usage, memory usage, and network I/O.I chose to use a feature flag for this because I don't think there is any truly safe way to understand when to use the optimized query pattern vs the old one because of progressive rollouts, rollbacks, etc. As such, I've left it up to the user to enable the feature but I think at some point we should try and make it the default behavior (ie. the next minor release).
How was this patch tested?
We tested this optimization in our internal for albeit with a Redis based TaskAction store implementation we have. I think performance improvements should be similar.
Example run with total tasks set to 5000.
Before (Notice replicas autoscale to 5. 2-3 total CPU usage, memory usage shoots up > 2G, and network RX is ~ 20-30 MB/s)

After (CPU usage peaks at .3 CPU, memory peaks at 400MB, network RX peaks at ~3 MB/s)

Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Stack
If you do use
git townto manage PR Stacks, the stack relevant to this PRwill show below. Otherwise, you can ignore this section.
Docs link