Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible enhance ambassador customization in seldon deployment #1378

Closed
yufengshan opened this issue Jan 28, 2020 · 6 comments · Fixed by #1383
Closed

Possible enhance ambassador customization in seldon deployment #1378

yufengshan opened this issue Jan 28, 2020 · 6 comments · Fixed by #1383
Assignees
Milestone

Comments

@yufengshan
Copy link

If one wants to customize the ambassador configuration for a certain Seldon deployment, the user needs to add an annotation to the seldondeployment yaml file by following the link at Seldon document page:
https://docs.seldon.io/projects/seldon-core/en/latest/ingress/ambassador.html#custom-amabassador-configuration

For example, if a user wants to set the num_retries of ambassador for a certain backend service, he/she needs to include an annotation in the seldondeployment yaml file (an example is at the bottom of this page). The annotation looks verbose and easy to make errors. Can Seldon team work on an easy way to customize the ambassador? maybe parameterize some params?

One more question here, I checked the envoy document online, the default value for num_retries at envoy is set to 1. At Seldon, it is set to 3 as default. Are there any reason for choosing 3?

https://www.envoyproxy.io/learn/automatic-retries
By default, Envoy will set the number of retries to one with num_retries. There’s little downside to increasing this to three, especially for relatively short requests, as Envoy will limit the total time spent to the overall request timeout, including the initial request and all retries.

======================= seldondeployment yaml file annotation =======

kind: SeldonDeployment
metadata:
  labels:
    app: seldon
  name: monitor-backend-service-amba
  namespace: seldon-monitor
spec:
  annotations:
    seldon.io/headless-svc: "true"
    seldon.io/ambassador-config: "apiVersion: ambassador/v1\n\
      kind: Mapping\n\
      name: seldon_seldon-monitor_monitor-backend-service-amba_main_rest_mapping\n\
      prefix: /seldon/seldon-monitor/monitor-backend-service-amba/\n\
      service: monitor-backend-service-amba-monitor-backend-service-amba-main.seldon-monitor:8000\n\
      timeout_ms: 3000\n\
      weight: 100\n\
      retry_policy:\n\
      \  retry_on: connect-failure\n\
      \  num_retries: 1\n\
      --- \n\
      apiVersion: ambassador/v1\n\
      kind: Mapping\n\
      name: seldon_seldon-monitor_monitor-backend-service-amba_main_grpc_mapping\n\
      grpc: true\n\
      prefix: /seldon.protos.Seldon\n\
      rewrite: /seldon.protos.Seldon\n\
      service: monitor-backend-service-amba-monitor-backend-service-amba-main.seldon-monitor:5001\n\
      timeout_ms: 3000\n\
      weight: 100\n\
      headers:\n\
      \  namespace: seldon-monitor\n\
      \  seldon: monitor-backend-service-amba\n\
      retry_policy:\n\
      \  retry_on: connect-failure\n\
      \  num_retries: 2\n"
  name: monitor-backend-service-amba
@ukclivecox
Copy link
Contributor

Could we extend the annotations available to cover some core basic Ambassador configs : https://docs.seldon.io/projects/seldon-core/en/latest/ingress/ambassador.html ?

Any alternative suggestions?

@ukclivecox ukclivecox added this to the 1.1 milestone Jan 28, 2020
@ukclivecox
Copy link
Contributor

I suggest we change to 1 reties following envoy recommendation?

@ukclivecox ukclivecox self-assigned this Jan 29, 2020
@yufengshan
Copy link
Author

yufengshan commented Jan 29, 2020 via email

@ukclivecox
Copy link
Contributor

@yufengshan can you review the PR #1383 ?

@yufengshan
Copy link
Author

yufengshan commented Jan 30, 2020 via email

@divyadilip91
Copy link

Hi,
Is it possible to customize the seldon ambassador url?
I know that the url prefix can be changed from the default.
By default it is http:///seldon///api/v0.1/predictions.
I have seen cases where we can change the prefix upto api.
Is it possible to completely change the url according to user requirement?That is without the portion /api/v0.1/predictions?
Kindly help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants