osm icon indicating copy to clipboard operation
osm copied to clipboard

Retries: consolidate retries into UpstreamTrafficSetting API

Open shashankram opened this issue 2 years ago • 2 comments

Please describe the Improvement and/or Feature Request

The UpstreamTrafficSetting API is used to configure settings for traffic directed to upstream services. Currently, circuit breaking and rate limiting are a part of the UpstreamTrafficSetting spec.

A long term goal has been to consolidate the retry policy into the same UpstreamTrafficSetting API, as it allows more fine-grained control over applying HTTP policies at the virtual_host or route level. Moreover, it simplifies various code paths as custom policies pertaining to upstream services would be readily available on the UpstreamTrafficSetting object, thereby removing the need for multiple informers, caches, events, internal APIs, etc.

As a proposal, I would like to have the existing RetryPolicy (alpha feature) merged into the UpstreamTrafficSetting API as follows:

apiVersion: policy.openservicemesh.io/v1alpha1
kind: UpstreamTrafficSetting
metadata:
  name: foo
  namespace: test
spec:
  host: foo.test.svc.cluster.local
  retries: # applied on virtual_host
    retryOn: "5xx"
    perTryTimeout: 1s
    numRetries: 5
    retryBackoffBaseInterval: 1s
    sources:
    - kind: ServiceAccount
      namespace: curl
      name: curl
  httpRoutes: # applied on routes within virtual_host
    - path: /foo
      retries: 
        retryOn: "5xx"
        perTryTimeout: 1s
        numRetries: 5
        retryBackoffBaseInterval: 1s
        sources:
        - kind: ServiceAccount
          namespace: curl
          name: curl

Scope (please mark with X where applicable)

  • Retries [X]

shashankram avatar Jun 08 '22 20:06 shashankram

@shalier As a part of #2018, I am going to add support to plumb the policy in UpstreamTrafficSetting at the inbound route and virtual_host level. Since retries and rate limiting policies are at the same level, it would be easier for you to update the retries code once the initial part of #2018 is done. However, you may independently start updating UpstreamTrafficSetting API schema as I don't intend to modify it further at the moment.

shashankram avatar Jun 10 '22 18:06 shashankram

@shalier I wanted to also note that before proceeding with an implementation, it would make sense to verify if merging the Retries policy with the UpstreamTrafficSetting API is feasible. I have outlined a sketch in the description, but it does not imply the design is vetted.

shashankram avatar Jun 16 '22 16:06 shashankram

This issue will be closed due to a long period of inactivity. If you would like this issue to remain open then please comment or update.

github-actions[bot] avatar Jan 14 '23 00:01 github-actions[bot]

Issue closed due to inactivity.

github-actions[bot] avatar Jan 21 '23 00:01 github-actions[bot]