GH-84 refactor DNSRecord conflict detection #100
Conversation
|
It has as a base Mike's PR #103 I guess it is a merge race! (that is there will be changes on that PR) If that looks good we can have one PR to rule 'em all |
maksymvavilov
changed the title
maksymvavilov
force-pushed
the
GH-84
branch
2 times, most recently
from
49571b0 to
b2ff34f
Compare
maksymvavilov
force-pushed
the
GH-84
branch
3 times, most recently
from
235ea03 to
84a885e
Compare
|
Also added an option to override validation timeout for the DNS controller. With the "default" 5 seconds some tests could be flaky and would timeout after 15 secs. In this repo the integration suite doesn't take ages to run, but I'd rather make tests faster if I can rather than increasing timeouts |
maksymvavilov
force-pushed
the
GH-84
branch
5 times, most recently
from
a419303 to
0986c42
Compare
| Clock clock.Clock = clock.RealClock{} | ||
| defaultRequeueTime time.Duration | ||
| defaultValidationRequeue time.Duration | ||
| validationRequeueTime time.Duration |
There was a problem hiding this comment.
This var is always the randomized requeue, right? I think the name makes this confusing, it would be better with something like: randomizedRequeueTime. Though I'm not convinced that it's required at this scope... why not just calculate it at the point it is required?
There was a problem hiding this comment.
It is used a few times across the code and if we will calculate it every time we need that we will end up with different values in one instance (right now will not cause any troubles, but might in future) and it seems cleaner to me to randomize once and then just use the var. But I do have a problem with naming 😆
| // implies that they were overridden - bump write counter | ||
| if !generationChanged(current) { | ||
| current.Status.WriteCounter++ | ||
| writeCounter.WithLabelValues(current.Name, current.Namespace).Inc() |
There was a problem hiding this comment.
should this involve the generation as well?
There was a problem hiding this comment.
We could 🤔 but not sure that we should. Considering that we should have a record per listener there will be a lot happening in the metrics. I'm concerned about the dimensions of this metric. Would generation be useful considering that we reset it anyway on a generation change? I reason that whatever tool will be consuming this metric (Grafana most likely) will maintain the history to see write counts of the past gens.
|
This looks basically fine, other than a few small comments |
Currently contains only the logic to change behaviour on conflict detection and conflict resolution.
Also moves all the logic for updating the status of the DNS Record to the
updateStatus()func.Finally, allows you to override default validation timeout using a flag (defaults to the 5 sec)