fix: add back the --sync-period flag #1506
Conversation
shaneutt
added
area/maintenance
Codecov Report
@@ Coverage Diff @@
## next #1506 +/- ##
==========================================
+ Coverage 51.46% 51.65% +0.18%
==========================================
Files 91 91
Lines 6299 6300 +1
==========================================
+ Hits 3242 3254 +12
+ Misses 2764 2754 -10
+ Partials 293 292 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Per kubernetes-sigs/controller-runtime#521 we may want to revert to the default. The original was probably arbitrary--no way to know for sure, as it was there from the start and presumably just copied from the NGINX controller settings. However, they've since disabled it entirely kubernetes/ingress-nginx#2634
It looks like this was a workaround for client-go issues that are no longer present, and shorter times may cause issues. However, we also have no known reports of issues, so 🤷
Good find on that issue, I'll change the default. |
This patch adds the --sync-period flag back and delivers the provided time duration to controller runtime, which now in v2 handles the relevant client-go configuration on our behalf (whereas in v1 we used it directly). We now use the default value of 48 hours instead of the value we previously used in v1 of 10 minutes, as per the improvements and context relevant to the setting which make relying on it no longer necessary. See also: kubernetes-sigs/controller-runtime#521
What this PR does / why we need it:
This patch adds the
--sync-periodflag back and deliversthe provided time duration to controller runtime, which
now in v2 handles the relevant client-go configuration on
our behalf (whereas in v1 we used it directly).
This intentionally uses the same settings that V1 established
(instead of the defaults provided by
controller-runtime) to avoidchange where we don't otherwise have any data influencing us
to pick a new setting.
More context can be seen in this relevant comment.
Which issue this PR fixes
Fixes #1309