Bugfix: Turing API should process experiment engine passkey only if client selection enabled #196
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 4 commits
April 27, 2022 06:28
krithika369
changed the title
krithika369
force-pushed
the
bugfix_std_exp_engine
branch
from
6a91024 to
5bf6e00
Compare
krithika369
commented
Apr 27, 2022
| @@ -41,7 +41,7 @@ func getRouter( | |||
|
|
|||
| err = json.Unmarshal(respBytes, &router) | |||
| if err != nil { | |||
| return nil, err | |||
| return nil, fmt.Errorf("Could not unmarshal: %v\n %s", err, string(respBytes)) | |||
There was a problem hiding this comment.
Added this for debugging e2e test failure, but I think it's ok to keep it.
krithika369
changed the title
terryyylim
approved these changes
Apr 27, 2022
| Name: "standard-engine-1", | ||
| Type: manager.StandardExperimentManagerType, | ||
| StandardExperimentManagerConfig: &manager.StandardExperimentManagerConfig{ | ||
| ClientSelectionEnabled: false, |
There was a problem hiding this comment.
nit: Any value in adding these testcases too?
&manager.StandardExperimentManagerConfig{}forengineInfo.StandardExperimentManagerConfig.ClientSelectionEnabledconditionStandardExperimentManagerConfig: nilforengineInfo.StandardExperimentManagerConfig != nilcondition
There was a problem hiding this comment.
&manager.StandardExperimentManagerConfig{}is the same as&manager.StandardExperimentManagerConfig{ClientSelectionEnabled: false}- so I don't see any use for this.StandardExperimentManagerConfig: nil- I initially decided not to bother about this because it's only precautionary (NewExperimentsServicewill ensure that anilvalue is not possible for this field if the type if determined to be standard). But I guess it's good to cover nonetheless - I'll add it in. Thanks for the feedback!
krithika369
added a commit
that referenced
this pull request
Apr 28, 2022
…lient selection enabled (#196) * Bugfix: Passkey should not be processed if client selection disabled * Update hardcoded sample plugin to use experiment variables, consistent with the runner * Update RPC plugin example and docs * Correct numbering in doc and plugin name change * Add debug message * Update Deployment controller to consider if client selection enabled * Add another unit test case for TestIsClientSelectionEnabled Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> (cherry picked from commit 83a0c6c)
krithika369
added a commit
that referenced
this pull request
Jun 6, 2022
* Update workflows to use Python 3.7 for ensembler engines and sdk (#190) * Update workflows to use Python 3.7 for ensembler engines and sdk * Add none return option for config in Router SDK class * Update text display settings to display entire image name * Set container image name to overflow instead of being truncated * Include response headers in logs (#191) * Update proto and classes * Add response headers and refactor naming of response bodies * Refactor logging methods * Fix tests * Fix kafka tests * Fix kafka tests * Fix line breaks * Fix line breaks * Fix line breaks * Fix typo in error message * Refactor response fields * Refactor response headers as map of strings * Refactor tests to use map of strings to represent headers * Fix non deterministic serialisation of hashmap in tests * Refactor log handler * Remove debug statement * Refactor HTTP header formatting into a helper function * Fix kafka protobuf test to use JSON as means of comparison * Rename body in proto to response to avoid breaking changes * Refactor all tests to use response instead of body to refer to request body * Fix lint import suggestion * Remove debug statements * Rename variables in http header formatting helper function * Fix BQ marshalling issues * Fix lint import suggestion * Fix lint comments * Minor fixes for experiment engine configs in the helm chart (#193) Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Nop Ensembler Config (#192) * UI changes for nop ensembler config * Make handling of router / version status consistent * SDK changes for default route * Correct the default route id in unit tests * Add tests for the nop ensembler config * Update sample code and doc * Add PR comments Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * [BUG] Fix undeploy router error (#194) * Fix bugs in sample SDK script * Refactor UndeployRouterVersion to take in cleanup flag * Update DeploymentService mock class * Fix bug in deployment controller test * Refactor if else blocks * Add tests for IsKnativeServiceInNamespace and IsSecretinNamespace * Rename test method to make it consistent with the method tested * Refactor k8s deletion methods into separate methods * Fix bug in deleting deployments and services * Fix typo in router name * Rename default route from nothing to control * Make undeploying a pending router status a cleanup job * Refactor code to use ignoreNotFound flag * Fix go mod file * Bugfix: Turing API should process experiment engine passkey only if client selection enabled (#196) * Bugfix: Passkey should not be processed if client selection disabled * Update hardcoded sample plugin to use experiment variables, consistent with the runner * Update RPC plugin example and docs * Correct numbering in doc and plugin name change * Add debug message * Update Deployment controller to consider if client selection enabled * Add another unit test case for TestIsClientSelectionEnabled Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Add Fallback Response Route Config for Standard Ensemblers (#197) * UI changes for standard ensembler Fallback response * Add validation for fallback resonse route id * Update docs for the Standard Ensembler config * Routing stragey changes for default route handling * SDK changes for fallback response route id * Amend user docs Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Remove the default route configuration (#198) * Remove unused default route property * Router: Make the default_route_id only required for DefaultTuringRoutingStrategy * Make Default Route ID optional for the Turing Router creater / update API * Update e2e tests * UI: Update router view / edit to stop handling default route explicitly * UI: Exclude routes with traffic rules in the final/fallback response options * SDK: deprecate the default_route_id config * SDK: Remove default route id from samples * Update user docs * Update OpenAPI bundle * Address PR comments Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Adding SDK support for Python 3.8 and 3.9 (#199) * Update SDK / engines to support multiple Python versions * Pin cloudpickle at 2.0.0 * Introduce Python Version on the Pyfunc ensembler config * Update SDK unit tests * Update Github workflows * Update chart values * Update docs, unit tests * Update SDK CI workflows to test on all Python versions * Pin protobuf version at 3.20.1 * Address PR comments Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Bugfix: Clear default route Id from custom ensembler configs (#200) * Miscellaneous bug fixes * Add unit tests for the SDK changes, address PR comments * Bugfix: Regression in display of container configs for Pyfunc * Add type annotation to class methods Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Update chart version for app release (#201) Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Add dynamic loading of Experiment Engine config (#202) * Add dynamic loading of exp engine config * Address PR comments * Add useEffect rerender * Address PR comments * Simplify conditional logic * Attempt to fix yarn install error Co-authored-by: Ewe Zi Yi <36802364+deadlycoconuts@users.noreply.github.com> Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> Co-authored-by: Terence Lim <terencelimxp@gmail.com>
krithika369
added a commit
that referenced
this pull request
Jun 9, 2022
* Upgrade Go version/Knative/Spark Operator/Kubernetes Client/Docker Compose for dev environment. (#183) * Bump versions for all k8s based libs * Use proper context for each scenario. * Upgrade virtual service version. * Update k3d version to kubernetes 1.22. * Upgrade Go to 1.16. * Fix linting errors. * Update k3d flags. * Upgrade knative/istio/spark-operator versions in cluster init. * Update default versions in cluster-init, change e2e test to new api * Fail fast if default environment is wrong. Extra logging. * Fix unit tests. * Addressed PR comments. * Pull requests to be run on any target branch. * Upgrade to go 1.18, upgrade linter. * Upgrade experiment and router to go 1.18. * Update PR comments. * Parameterise Go and Go Linter Versions. * Update documentation with new versions and ports. * Merge from main -> knative-upgrade branch (#203) * Update workflows to use Python 3.7 for ensembler engines and sdk (#190) * Update workflows to use Python 3.7 for ensembler engines and sdk * Add none return option for config in Router SDK class * Update text display settings to display entire image name * Set container image name to overflow instead of being truncated * Include response headers in logs (#191) * Update proto and classes * Add response headers and refactor naming of response bodies * Refactor logging methods * Fix tests * Fix kafka tests * Fix kafka tests * Fix line breaks * Fix line breaks * Fix line breaks * Fix typo in error message * Refactor response fields * Refactor response headers as map of strings * Refactor tests to use map of strings to represent headers * Fix non deterministic serialisation of hashmap in tests * Refactor log handler * Remove debug statement * Refactor HTTP header formatting into a helper function * Fix kafka protobuf test to use JSON as means of comparison * Rename body in proto to response to avoid breaking changes * Refactor all tests to use response instead of body to refer to request body * Fix lint import suggestion * Remove debug statements * Rename variables in http header formatting helper function * Fix BQ marshalling issues * Fix lint import suggestion * Fix lint comments * Minor fixes for experiment engine configs in the helm chart (#193) Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Nop Ensembler Config (#192) * UI changes for nop ensembler config * Make handling of router / version status consistent * SDK changes for default route * Correct the default route id in unit tests * Add tests for the nop ensembler config * Update sample code and doc * Add PR comments Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * [BUG] Fix undeploy router error (#194) * Fix bugs in sample SDK script * Refactor UndeployRouterVersion to take in cleanup flag * Update DeploymentService mock class * Fix bug in deployment controller test * Refactor if else blocks * Add tests for IsKnativeServiceInNamespace and IsSecretinNamespace * Rename test method to make it consistent with the method tested * Refactor k8s deletion methods into separate methods * Fix bug in deleting deployments and services * Fix typo in router name * Rename default route from nothing to control * Make undeploying a pending router status a cleanup job * Refactor code to use ignoreNotFound flag * Fix go mod file * Bugfix: Turing API should process experiment engine passkey only if client selection enabled (#196) * Bugfix: Passkey should not be processed if client selection disabled * Update hardcoded sample plugin to use experiment variables, consistent with the runner * Update RPC plugin example and docs * Correct numbering in doc and plugin name change * Add debug message * Update Deployment controller to consider if client selection enabled * Add another unit test case for TestIsClientSelectionEnabled Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Add Fallback Response Route Config for Standard Ensemblers (#197) * UI changes for standard ensembler Fallback response * Add validation for fallback resonse route id * Update docs for the Standard Ensembler config * Routing stragey changes for default route handling * SDK changes for fallback response route id * Amend user docs Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Remove the default route configuration (#198) * Remove unused default route property * Router: Make the default_route_id only required for DefaultTuringRoutingStrategy * Make Default Route ID optional for the Turing Router creater / update API * Update e2e tests * UI: Update router view / edit to stop handling default route explicitly * UI: Exclude routes with traffic rules in the final/fallback response options * SDK: deprecate the default_route_id config * SDK: Remove default route id from samples * Update user docs * Update OpenAPI bundle * Address PR comments Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Adding SDK support for Python 3.8 and 3.9 (#199) * Update SDK / engines to support multiple Python versions * Pin cloudpickle at 2.0.0 * Introduce Python Version on the Pyfunc ensembler config * Update SDK unit tests * Update Github workflows * Update chart values * Update docs, unit tests * Update SDK CI workflows to test on all Python versions * Pin protobuf version at 3.20.1 * Address PR comments Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Bugfix: Clear default route Id from custom ensembler configs (#200) * Miscellaneous bug fixes * Add unit tests for the SDK changes, address PR comments * Bugfix: Regression in display of container configs for Pyfunc * Add type annotation to class methods Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Update chart version for app release (#201) Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> * Add dynamic loading of Experiment Engine config (#202) * Add dynamic loading of exp engine config * Address PR comments * Add useEffect rerender * Address PR comments * Simplify conditional logic * Attempt to fix yarn install error Co-authored-by: Ewe Zi Yi <36802364+deadlycoconuts@users.noreply.github.com> Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> Co-authored-by: Terence Lim <terencelimxp@gmail.com> * Update CI specs * Revert UI changes during merge * Update CI specs * Update e2e deployment timeout * Remove WIP inline comment Co-authored-by: Ashwin <ashwinath@hotmail.com> Co-authored-by: Ewe Zi Yi <36802364+deadlycoconuts@users.noreply.github.com> Co-authored-by: Krithika Sundararajan <krithika.sundararajan@go-jek.com> Co-authored-by: Terence Lim <terencelimxp@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Turing API does special handling of the experiment engine passkey (namely, copying existing passkey if unset and encrypting it) when a router is created / edited. While the concept of the passkey is only applicable to those experiment managers that allow client selection, the code was previously attempting to process the passkey for all standard experiment engines. This would lead to a 500 error:
Passkey must be configuredwhen attempting to make a create / update router request.The fix corrects the check:
api/turing/service/experiment_service.go- AddIsClientSelectionEnabledas a convenience method, for the request module to useapi/turing/api/request/request.go- Call experiment service'sIsClientSelectionEnabledinstead ofIsStandardExperimentManagerapi/turing/api/deployment_controller.go- Call decrypt on the passkey based onIsClientSelectionEnabledinstead ofIsStandardExperimentManagerIn addition, some small changes are made to the hardcoded experiment engine RPC plugin example, to complete its configuration from the API.
engines/experiment/examples/plugins/hardcoded/manager.go- ImplementListVariablesForExperimentsso the variables expected by the runner (here and here) can be configured on the router.engines/experiment/examples/plugins/hardcoded/configs/plugin_config_example.yaml- Update the example plugin config to include the unit variable.engines/experiment/docs/rpc_plugins.md- Update doc to show the example plugin being configured from the UI