Make agent reconnect retry timeout configurable#6470
Conversation
|
Surge PR preview deployment was removed |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6470 +/- ##
==========================================
+ Coverage 41.25% 41.28% +0.03%
==========================================
Files 431 431
Lines 28792 28805 +13
==========================================
+ Hits 11878 11893 +15
+ Misses 15847 15845 -2
Partials 1067 1067 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Just wondering, we don't use the backoff library there right? |
|
we do use backoff lib: woodpecker/agent/rpc/client_grpc.go Line 130 in 52ed3f1 woodpecker/agent/rpc/client_grpc.go Line 119 in 52ed3f1 woodpecker/agent/rpc/client_grpc.go Line 96 in 52ed3f1 |
|
But shouldn't it be possible to use their internal logic with https://pkg.go.dev/github.com/cenkalti/backoff/v5#WithMaxElapsedTime or https://pkg.go.dev/github.com/cenkalti/backoff/v5#WithMaxTries? |
|
I can look into that but that would be a refactoring followup pull :) |
There was a problem hiding this comment.
Is ok for me, but remind that you then likely have to change the env var behavior which is breaking as the backoff lib doesn't support cumulated timeout. It can only limit the number of retries (which is what we usually do, so I'd go with that) or the duration between two tries.
PR is not tested at my side
|
The lib should support max timeout period ... but even if not, we just have it in our code. Limiting to featureset of an external dependency is not something we should do if easy to make it ourselfe work. anyway, I'll look into using the backoff lib more directly |
|
You're right, the correct function you should use is https://pkg.go.dev/github.com/cenkalti/backoff/v5#WithMaxElapsedTime I guess |
followup #6414 (comment)
also fix https://ci.woodpecker-ci.org/repos/3780/pipeline/33572/39