bugfix for annotation updater. #490
Conversation
pkg/plugin/aggregation/run.go
Outdated
| return | ||
| } | ||
| wait.JitterUntil(func() { | ||
| pluginsdone := aggr.isComplete() |
There was a problem hiding this comment.
I could probably make this a var outside and used on final check in defer.
| @@ -113,21 +116,31 @@ func Run(client kubernetes.Interface, plugins []plugin.Interface, cfg plugin.Agg | |||
| }() | |||
|
|
|||
| updater := newUpdater(expectedResults, namespace, client) | |||
| ticker := time.NewTicker(annotationUpdateFreq) | |||
| ctx, cancel := context.WithCancel(context.TODO()) | |||
There was a problem hiding this comment.
should this be .Background()?
There was a problem hiding this comment.
For this case, nope.
pkg/plugin/aggregation/run.go
Outdated
| logrus.Info("All plugins have completed, status has been updated") | ||
| cancel() | ||
| } | ||
| }, annotationUpdateFreq, 1.2, true, ctx.Done()) |
There was a problem hiding this comment.
can you const these magic numbers?
pkg/plugin/aggregation/run.go
Outdated
| cancel() | ||
| // 2. Try one last time to get an update out on exit | ||
| updater.ReceiveAll(aggr.Results) | ||
| if err := updater.Annotate(); err != nil { |
There was a problem hiding this comment.
could ReceiveAll() and Annotate() be combined? looks like they're duplicated.
There was a problem hiding this comment.
probably, I'll check the call sites and update if I can reduce.
pkg/plugin/aggregation/run.go
Outdated
| } | ||
| }() | ||
| if pluginsdone { |
There was a problem hiding this comment.
any particular reason we emit this on the JitterUntil case but not on the termination guard case?
There was a problem hiding this comment.
I have two separate log statements to try and bisect in case we somehow see an issue in the wild.
Signed-off-by: Timothy St. Clair <timothysc@gmail.com>
Signed-off-by: Timothy St. Clair <timothysc@gmail.com>
Signed-off-by: Timothy St. Clair <timothysc@gmail.com>
What this PR does / why we need it:
Fixes annotation updater issue(s).
Which issue(s) this PR fixes
Special notes for your reviewer:
I still need to run some tests on this to verify.
Release note:
/assign @liztio