-
Notifications
You must be signed in to change notification settings - Fork 835
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
support traffic settings for shadow deployment with istio #3780
support traffic settings for shadow deployment with istio #3780
Conversation
Hi @ChowXu. Thanks for your PR. I'm waiting for a SeldonIO or todo member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository. |
Thanks @ChowXu seldon-core/operator/apis/machinelearning.seldon.io/v1/seldondeployment_webhook.go Lines 126 to 149 in ce9a284
|
0d5206c
to
8424ef3
Compare
|
} | ||
if trafficSum > 0 && trafficSum < 100 && len(spec.Predictors) == 1 { | ||
allErrs = append(allErrs, field.Invalid(fldPath, spec.Predictors[0].Name, "Traffic must sum be 100 for a single predictor when set")) | ||
if shadows == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to do the checks even if shadows are present. We just need to ignore the shadow percentage in the calculation.
Looks good - following up on some of the requests, it would be great if you could also add some docs, and potentially extend some of the istio docs to show an example (i.e. the istio notebook) |
8424ef3
to
306f927
Compare
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
c8f71ee
to
0f7ede8
Compare
thanks @axsaucedo , i have added the shadow deploy doc upder examples.istio , and also with a tip about traffic settings usage. |
@ChowXu There are some errors compiling the operator. |
0f7ede8
to
e835564
Compare
thanks @cliveseldon , it fixed now |
allErrs = append(allErrs, field.Invalid(fldPath, spec.Predictors[i].Name, "shadow traffic is illegal, the traffic number should be between [0, 100]")) | ||
} | ||
} else { | ||
trafficSum = trafficSum + p.Traffic | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a go test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, i have add a UT for shadow traffic validation
e835564
to
02cc652
Compare
/test integration |
/test notebook |
@ChowXu: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository. |
/retest |
@ChowXu: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository. |
Hi @axsaucedo the integration test meet some problems, but i have not authorization to visit pipelinerun page to see detail about what happended and i cannot rerun the test , could u give me a help? |
/test integration |
@ChowXu will run again as failed early so need to see if real issue |
/test integration |
1 similar comment
/test integration |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: axsaucedo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it: support traffic settings for shadow deployment with istio to meet more situations
Special notes for your reviewer:
Which issue(s) this PR fixes: Fixes #3772
Does this PR introduce a user-facing change?: NONE