FRR configuration retries #1463
Conversation
If the reload fails, most likely due to the reloader container not ready yet, we wait and try to reload the configuration again. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
If we detect a failure in the configuration processing, we ask the debouncer to retry using the latest configuration. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
fedepaol
requested review from
gclawes,
johananl,
oribon,
rata and
russellb
as code owners
| if newCfg.useOld && config == nil { | ||
| continue // just ignore the event | ||
| } |
There was a problem hiding this comment.
not sure I understand this :/ why sending something to the chan in the first place only to do nothing with it?
There was a problem hiding this comment.
This should never happen, because it means that we received a "reload old config" event but the old config is nil (meaning, we already tried a reload that failed).
Nonethless, I'd prefer not having the speaker crash but ignore the event in such corner case.
There was a problem hiding this comment.
sorry, I misread it as newCfg.config == nil 😅
| } | ||
| updated = <-result | ||
| if updated.Hostname != "1" { | ||
| t.Fatal("Config was not updated") |
There was a problem hiding this comment.
shouldn't this be "Config was updated"?
There was a problem hiding this comment.
Nope, because what writes to result is the action to update the config. So in this case, if we don't get the value, it means that the retry mechanism did not trigger and that the update function was not called
| err := body(config) | ||
| if err != nil { | ||
| timeOut = time.After(failureRetryInterval) | ||
| timerSet = true |
There was a problem hiding this comment.
does this mean that after a fail the next config update won't happen until the failureretryinterval finishes? shouldn't we let new updates override this timeout?
There was a problem hiding this comment.
That is correct, and I thought that it makes sense to go slow after a failure happened (think about the case of the slow container startup, after a failure we want to give it time to recover). 5 seconds is still a bearable timeout for applying configuration changes.
|
Tested also manually by putting a sleep in the reloader script first, and in the frr entry point after, and the speaker recovered. |
Adding a note about metallb#1463 Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
Adding a note about #1463 Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
Adding a note about metallb#1463 Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
Adding a note about metallb/metallb#1463 Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
2 participants
Making the interaction with FRR more robust introducing retries after failures in both the call of the reloader (if the pid is not there yet) and if we find that the interaction between the reloader and the container fails.
Fixes #1462