csiaddonsnode: Add retry with exponential backoff for connections #924
Conversation
mergify
bot
requested review from
Madhu-1,
Nikhil-Ladha,
Rakshith-R,
iPraveenParihar,
nixpanic and
yati1998
black-dragon74
force-pushed
the
fix-extra-addonsnodeconn
branch
from
b007a75 to
b5acabf
Compare
|
|
||
| const ( | ||
| CSIAddonsNodeConnectionMaxRetries = 3 | ||
| CSIAddonsNodeConnectionSleepInterval = 2 * time.Second |
There was a problem hiding this comment.
This could be a little short, maybe? If there is a temporary network issue, the object may get deleted too early?
Consider doing an exponential backoff like other reties in Kubernetes do.
| var connErr error | ||
| for i := range CSIAddonsNodeConnectionMaxRetries { | ||
| logger.Info("Connecting to sidecar", "attempt", i) | ||
| newConn, connErr = connection.NewConnection(ctx, endPoint, nodeID, driverName, csiAddonsNode.Namespace, csiAddonsNode.Name, r.EnableAuth) |
There was a problem hiding this comment.
Have you considered doing the retry in the connection itself? That would make the controller a little simpler, it should not need to care about reconnecting too much. Only if there is a particular error returned, the CSIAddonsNode CR could be deleted by the controller.
There was a problem hiding this comment.
This sounds great. Components other than this reconciler would also benefit from the said change. Thank you!
There was a problem hiding this comment.
Upon further consideration, retries inside connection package will be blocking and might starve the work queue.
I implemented the backoff using RequeueAfter and am tracking state in annotation and status.
black-dragon74
force-pushed
the
fix-extra-addonsnodeconn
branch
from
b5acabf to
714a22d
Compare
This patch adds the functionality to retry for a maximum of `maxRetries` to connect to the sidecar. If the connection attempt is not successful, the object is considered obsolete and is deleted. The retry is tracked inside an annotation(`connRetryAnnotation`) and also reflected in object's status. These transient artifacts are cleaned up once a connection is established. Signed-off-by: Niraj Yadav <niryadav@redhat.com>
black-dragon74
force-pushed
the
fix-extra-addonsnodeconn
branch
from
714a22d to
31bdf4c
Compare
black-dragon74
changed the title
| backoff := baseRetryDelay * time.Duration(math.Pow(2, float64(currentRetries))) | ||
| logger.Info("Requeuing request for attempting the connection again", "backoff", backoff) | ||
|
|
||
| return ctrl.Result{RequeueAfter: backoff}, nil |
There was a problem hiding this comment.
The 1st retry is after 0 seconds? That probably isn't what you want. Should getRetryCountFromAnnotation() really return 0 if the annotation is not set?
There was a problem hiding this comment.
The first retry is baseRetryDelay * 2 ^ valFromAnnotation.
Hence, 3 * 2^0 leads to 3 * 1, as intended.
|
What is the process to get a deleted CSIAddonsNode back in case of a longer network interruption? Can that be automated too? |
2 participants
This patch adds the functionality to retry for a maximum of
maxRetriesto connect to the sidecar.If the connection attempt is not successful, the object is considered
obsolete and is deleted.
The retry is tracked inside an annotation(
connRetryAnnotation) and alsoreflected in object's status.
These transient artifacts are cleaned up once a connection is
established.