fix: disable runner #64
Conversation
bavarianbidi
force-pushed
the
toggle_runner_reconciliation
branch
from
a8dd8d4
to
231ce1e
Compare
|
Idk about my impl... On the one hand i like that it cleans up runner crs if the runner-sync flag is set to false and than disables itself, on the otherhand this might be confusing as there is still sth going on for the enduser even if the feat is disabled. I also thought about starting the runner controller not with the manager, but separate so one could disable to entire reconciler at runtime... |
| cache := make(map[string]struct{}) | ||
| var diff []string | ||
|
|
||
| for _, runner := range garmRunners { | ||
| cache[runner] = struct{}{} | ||
| } | ||
|
|
||
| for _, runnerCR := range runnerCRs { | ||
| if !slices.Contains(garmRunners, runnerCR) { | ||
| if _, found := cache[runnerCR]; !found { | ||
| diff = append(diff, runnerCR) | ||
| } | ||
| } |
There was a problem hiding this comment.
why is the new cache var needed?
doesn't do the old if !slices.Contains(garmRunners, runnerCR) { implementation exactly the same as the new if _, found := cache[runnerCR]; !found {?
I personally would stick to the funcs of the official slices package.
There was a problem hiding this comment.
this is just about optimization. Before that we had a O(n^2) runtime, as there were basically two nested for loops. Did some benchmarking on that with the benchmark package, and this solution is much more efficient because of the maps constant lookup times but of course has a bit higher memory footprint
There was a problem hiding this comment.
Idk about my impl... On the one hand i like that it cleans up runner crs if the runner-sync flag is set to false and than disables itself, on the otherhand this might be confusing as there is still sth going on for the enduser even if the feat is disabled. I also thought about starting the runner controller not with the manager, but separate so one could disable to entire reconciler at runtime...
i get your idea for cleaning up old/reflected runners but tbh, it feels wrong doing it even if the flag is set to don't do it.
IMHO it's fine to have orphaned runners in the cluster if the flag got disabled. Once the flag got enabled, they will gone - and in the meantime it's still possible to delete them by hand.
rafalgalaw
force-pushed
the
toggle_runner_reconciliation
branch
from
3f7b174
to
231ce1e
Compare
rafalgalaw
force-pushed
the
toggle_runner_reconciliation
branch
from
1e7019d
to
d19cb8b
Compare
No description provided.