Remove the default route configuration #198

krithika369 · 2022-05-17T05:02:52Z

This is a follow up from #192 and #197 and this PR removes the concept of the default route from the Turing UI and deprecates the default_route_id property in the SDK. The API and Router now treat the default_route_id property as optional (the API will require that the value is set when using Standard / Nop ensemblers only). The changes are described in detail below.

API

api/api/specs/routers.yaml - Make default_route_id in router config optional
api/e2e/** - Update e2e to stop setting the default_route_id in the test cases as they are no longer required for docker ensemblers.
api/turing/cluster/servicebuilder/router.go - Apply default route id to the router only if it is set in the DB
api/turing/validation/validator.go - Check that default route is set only if ensembler type is Standard or Nop and not set otherwise.

Router

engines/router/missionctl/fiberapi/routing_policies.go - The routeSelectionPolicy is used in the Custom ensembler's EnsemblingFanIn implementation as well as the DefaultTuringRoutingStrategy used in the Nop / Standard ensembler. Now that the Default Route config is not always required, it is checked in the parent struct.
engines/router/missionctl/fiberapi/routing_strategy.go - Check that default route property is set

SDK

sdk/turing/router/config/router_config.py - Add deprecation warning for the default_route_id property. Process it only if set.
Update unit tests and samples.

UI

The main changes in the workflow here are:

There is no "default route" selection in the routes config anymore
Traffic rules can be configured on all routes
Final Response route / Fallback response route cannot be a route that has any traffic rule on it (so, in essence, the check is flipped from "default route cannot have traffic rule on it" to "route with traffic rule cannot be a final/fallback route"). This in line with the sequence of steps (routes and traffic rules are configured first).

Other details on the UI changes are captured inline.

…ingStrategy

… API

…options

krithika369 · 2022-05-17T06:08:04Z

api/api/specs/routers.yaml

@@ -512,8 +512,6 @@ components:
          type: "array"
          items:
            $ref: "#/components/schemas/Route"
-        default_route:


I don't think this was ever meant to be there, it should have only been default_route_id. Removing it.

krithika369 · 2022-05-17T06:18:26Z

api/turing/validation/validator_test.go

 	}
 }

 func TestValidateTrafficRules(t *testing.T) {
+	// Common variables used by suite tests
+	routeAID, routeBID := "route-a", "route-b"
+	routeA, routeB := &models.Route{


Minor clean up of the hardcoded route info in the tests, to use the variables here.

krithika369 · 2022-05-17T06:19:53Z

engines/router/configs/custom_ensembling_router.yaml

@@ -3,7 +3,6 @@ id: combiner
 fan_in:
  type: fiber.EnsemblingFanIn
  properties:
-    default_route_id: route_id_1


No harm having this in the router example but this value will not be set by the API anymore, for custom ensemblers.

krithika369 · 2022-05-17T06:33:22Z

ui/src/router/components/configuration/components/router_config_section/RoutesConfigTable.js

@@ -59,6 +59,7 @@ export const RoutesConfigTable = ({ routes, rules = [], defaultRouteId }) => {
      return acc;
    }, {});

+    // Keep the default route (used in Nop / Std ensemblers), if any, on top


The previous blue cell highlight for the default route (shown below) is now removed, but I left the default route to continue to be on sorted on the top.

krithika369 · 2022-05-17T06:36:16Z

ui/src/router/components/form/components/RouteDropDownOption.js

  EuiToolTip,
 } from "@elastic/eui";

-export const RuleCardRouteDropDownOption = ({ id, endpoint, isDefault }) => {
+export const RouteDropDownOption = ({


This has been refactored to also let the Fallback Route / Final Response Route panel use the same format for the options (the badge + url) as shown in the PR description.

krithika369 · 2022-05-17T06:43:12Z

ui/src/router/components/form/components/ensembler_config/RouteSelectionPanel.js


 const SelectRouteIdCombobox = ({ value, routeOptions, onChange, ...props }) => {
-  // Note: routeNames changes on every render, which we need to capture name changes to routes
-  const routeNames = routeOptions.map((e) => (!!e.value ? e.value : e.label));
+  const noneValue = "_none_";


As captured in the inline code comments below, this special value (can be anything) is needed as a default, to select a placeholder-like option since the EuiSuperSelect doesn't have a placeholder property yet. The same mechanism is used in the traffic rules panel.

krithika369 · 2022-05-17T06:43:46Z

ui/src/router/components/form/components/ensembler_config/RouteSelectionPanel.js


  return (
-    <EuiComboBoxSelect
+    <EuiSuperSelect


Changing to EuiSuperSelect to have a richer dropdown, similar to the traffic rules panel.

krithika369 · 2022-05-17T06:45:04Z

ui/src/router/components/form/components/router_config/RoutesPanel.js

              endpointOptions={endpoints}
              onChange={onChange(`routes.${idx}`)}
-              onSelect={() => setDefaultRouteIndex(idx)}
-              onDelete={onDeleteRoute(idx)}
+              onDelete={routes.length > 1 ? onDeleteRoute(idx) : null}


Previously, we weren't allowing the default route to be deleted. Now that this concept is removed, we only try to ensure there is always at least one route.

krithika369 · 2022-05-17T06:45:54Z

ui/src/router/components/form/components/router_config/RulesPanel.js

@@ -34,7 +28,7 @@ export const RulesPanel = ({
  };

  const addRuleButton = (
-    <EuiButton fullWidth onClick={onAddRule} isDisabled={routes.length < 2}>


Same change as above - previously, we always had a default route and no traffic rule could be created on it.

krithika369 · 2022-05-17T06:48:20Z

ui/src/router/components/form/validation/schema.js

+const validRouteSchema = yup
+  .mixed()
+  .test("valid-route", "Valid route is required", function (value) {
+    const configSchema = this.from.slice(-1).pop();


The last item in the validation context has the entire router config (hierarchically), so using that to define this schema. This way, the schema can be used from multiple steps (fallback/final response validation and traffic rules).

Thanks for refactoring this!

deadlycoconuts

Thanks a lot for the changes to the UI/API/SDK! Looks good to me except for one tiny comment!

deadlycoconuts · 2022-05-19T06:24:22Z

ui/src/router/components/form/components/router_config/RulesPanel.js

@@ -34,7 +28,7 @@ export const RulesPanel = ({
  };

  const addRuleButton = (
-    <EuiButton fullWidth onClick={onAddRule} isDisabled={routes.length < 2}>
+    <EuiButton fullWidth onClick={onAddRule} isDisabled={!routes.length}>


Tiny nit: since routes.length == 0 can never happen (due to the changes you've made here), should we just remove isDisabled since there no way the 'Add Rule' button will ever be disabled?

Thanks for catching - at first, I didn't enforce the presence of at least one route from the routes panel and I had modified this condition (since all routes could be deleted). I introduced that check later but failed to update this. But thinking about it now, it makes sense to keep the old check here (have at least 2 routes to be able to create a rule) - so I put it back.

deadlycoconuts · 2022-05-19T06:27:32Z

ui/src/router/components/form/validation/schema.js

+const validRouteSchema = yup
+  .mixed()
+  .test("valid-route", "Valid route is required", function (value) {
+    const configSchema = this.from.slice(-1).pop();


Thanks for refactoring this!

terryyylim

Overall LGTM, very neat PR, thank you @krithika369 !

terryyylim · 2022-05-19T07:00:22Z

api/turing/validation/validator_test.go

+	// Common variables used by suite tests
+	routeAID, routeBID := "route-a", "route-b"
+	routeA, routeB := &models.Route{
+		ID:       "route-a",


nit: We could use the common variables routeAID and routeBID here.

krithika369 · 2022-05-20T02:39:43Z

Thanks for the reviews! I've addressed the comments. I'll merge this in once the CI succeeds.

* 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>

* 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>

Krithika Sundararajan added 8 commits May 8, 2022 19:44

Remove unused default route property

1567281

Router: Make the default_route_id only required for DefaultTuringRout…

06a3317

…ingStrategy

Make Default Route ID optional for the Turing Router creater / update…

f67f39c

… API

Update e2e tests

56dbcac

UI: Update router view / edit to stop handling default route explicitly

951e5ae

UI: Exclude routes with traffic rules in the final/fallback response …

aab3263

…options

SDK: deprecate the default_route_id config

d216599

SDK: Remove default route id from samples

b0a08c2

krithika369 marked this pull request as draft May 17, 2022 05:03

Krithika Sundararajan added 2 commits May 17, 2022 13:17

Update user docs

c947108

Update OpenAPI bundle

e3c8438

krithika369 force-pushed the remove_default_route branch from db2783d to e3c8438 Compare May 17, 2022 06:03

krithika369 commented May 17, 2022

View reviewed changes

krithika369 changed the title ~~Draft: Remove default route~~ Remove the default route configuration May 17, 2022

krithika369 marked this pull request as ready for review May 17, 2022 06:51

krithika369 requested a review from a team May 17, 2022 06:51

deadlycoconuts approved these changes May 19, 2022

View reviewed changes

terryyylim approved these changes May 19, 2022

View reviewed changes

Address PR comments

02b7198

krithika369 merged commit 3e26e40 into caraml-dev:main May 20, 2022

krithika369 deleted the remove_default_route branch May 20, 2022 02:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the default route configuration #198

Remove the default route configuration #198

krithika369 commented May 17, 2022 •

edited

Loading

krithika369 May 17, 2022

krithika369 May 17, 2022

krithika369 May 17, 2022

krithika369 May 17, 2022

krithika369 May 17, 2022

krithika369 May 17, 2022

krithika369 May 17, 2022

krithika369 May 17, 2022

krithika369 May 17, 2022

krithika369 May 17, 2022

deadlycoconuts May 19, 2022

deadlycoconuts left a comment

deadlycoconuts May 19, 2022

krithika369 May 20, 2022

deadlycoconuts May 19, 2022

terryyylim left a comment

terryyylim May 19, 2022

krithika369 commented May 20, 2022

Remove the default route configuration #198

Remove the default route configuration #198

Conversation

krithika369 commented May 17, 2022 • edited Loading

API

Router

SDK

UI

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadlycoconuts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryyylim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krithika369 commented May 20, 2022

krithika369 commented May 17, 2022 •

edited

Loading