Envoy Retry Policy Support #1358
Conversation
# Conflicts: # ambassador/ambassador/ir/irambassador.py
|
Hey, thanks for this! I'm afraid I've caused some trouble for you by dropping the old Envoy-V1 stuff and the old end-to-end tests -- can you take care of the merge conflicts (probably be deleting things) and we can talk further about tests? Are you on our Slack? |
|
@containscafeine does this touch the work you're looking at right now? |
|
@richarddli not directly, but this will work well in conjunction to circuit breakers 🎉 |
# Conflicts: # ambassador/templates/envoy.j2 # ambassador/tests/000-default/gold.intermediate.json
…apping test with test service configuration
|
@kflynn Hey, sorry for the late reply, busy days. i've merged the latest changes and removed the old / unnecessary files. i've also looked at the test descriptions and moved the old end to end test to a mapping test. and yes, i'm on the datawire slack too if you want to continue the discussion over there :) |
| @@ -236,6 +237,14 @@ def setup(self, ir: 'IR', aconf: Config) -> bool: | |||
| else: | |||
| return False | |||
|
|
|||
| if amod and ('retry_policy' in amod): | |||
| self.retry_policy = IRRETRYPOLICY(ir=ir, aconf=aconf, location=self.location, **amod.retry_policy) | |||
|
|
|||
There was a problem hiding this comment.
Will be helpful if we can validate the retry policy here. Look at validate_load_balancer for reference 👍
There was a problem hiding this comment.
i've added this to the IRRetryPolicy class and validated retry_on in setup. would that be a correct approach to this?
| from .ir import IR | ||
|
|
||
|
|
||
| class IRRETRYPOLICY (IRResource): |
There was a problem hiding this comment.
nit: change this to IRRetryPolicy
I believe you must for gotten confused with IRCORS being all caps, but that's because of CORS being in caps. The rest of the classes are camel case ;)
There was a problem hiding this comment.
ah yes, i guess the ircors got me there you're right. i've changed this.
| spec: | ||
| containers: | ||
| - name: retry | ||
| image: live/ambassador-retry-service:1.0.0 |
There was a problem hiding this comment.
Do you mind committing the Dockerfile for this image with this PR as well? :)
There was a problem hiding this comment.
of course. initially i've added the code and dockerfile to the now unused end-to-end tests (but thats now excluded via gitignore). where should i put those files?
| """) | ||
|
|
||
| def queries(self): | ||
| yield Query(self.parent.url("%s/retry/" % self.name), expected=200) |
There was a problem hiding this comment.
Can you explain a bit more on how does this test the retry policy behavior? How do we know that it's retrying on 5xx?
There was a problem hiding this comment.
i'm not sure if this test fits any requirements or best practices at all, but i'd need a bit help / guidance on implementing a solid testing logic for this one (i'm lacking a bit of experience here).
when i've added the changes, all i wanted todo was to check if it retries failed requests. so i just wrote a simple service that flips between a 503 and 200 response (first one fails, second one succeeds, and so on). with the retry policy in place, a query should always succeed on this service, therefor expecting the 200. With multiple (parallel) tests the flipping responses, as they are now, would probably cause issues though (maybe running multiple queries would be enough?). but i guess that there is a better solution to this?
# Conflicts: # ambassador/tests/test_ambassador.py
|
Can this be set once globally instead of per mapping? |
|
@flands yes! |
Description
Adds support für envoys retry_policy to ambassador module and http mappings.
Related Issues
#1127
Testing
Todos