Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

feat(routesv2) : Cut over OSM to build routes using routes v2 #2605

Merged
merged 1 commit into from
Feb 24, 2021
Merged

feat(routesv2) : Cut over OSM to build routes using routes v2 #2605

merged 1 commit into from
Feb 24, 2021

Conversation

snehachhabria
Copy link
Contributor

@snehachhabria snehachhabria commented Feb 24, 2021 •
edited
Loading

Description:

This PR makes routes v2 the default route building mechanism in OSM.

Signed-off-by: Sneha Chhabria snchh@microsoft.com

Affected area:

  • New Functionality [ ]
  • Documentation [ ]
  • Install [ ]
  • Control Plane [X]
  • CLI Tool [ ]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [ ]
  • Security [ ]
  • Tests [ ]
  • CI System [ ]
  • Performance [ ]
  • Other [ ]

Please answer the following questions with yes/no.

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution? no

@snehachhabria snehachhabria added wip Work-in-Progress do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 24, 2021
@snehachhabria snehachhabria mentioned this pull request Feb 24, 2021
Closed
@snehachhabria snehachhabria force-pushed the Routesv2Cutover branch 2 times, most recently from 24c4b49 to 6a5d974 Compare February 24, 2021 19:10
@snehachhabria snehachhabria changed the title make routes v2 default fest(routesv2) : cut over OSM to build routes using routes v2 Feb 24, 2021
snehachhabria
snehachhabria commented Feb 24, 2021
View reviewed changes
demo/deploy-bookstore.sh
@@ -23,6 +23,8 @@ spec:
ports:
- port: 80
name: bookstore-port
selector:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the selector on apex service back @draychev, @shashankram

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this isn't needed but putting it here doesn't cause any harm for this demo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I added it back since we had it previously

@snehachhabria snehachhabria removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. wip Work-in-Progress labels Feb 24, 2021
@snehachhabria snehachhabria marked this pull request as ready for review February 24, 2021 19:21
@snehachhabria snehachhabria requested a review from a team as a code owner February 24, 2021 19:21
@snehachhabria snehachhabria changed the title fest(routesv2) : cut over OSM to build routes using routes v2 fest(routesv2) : Cut over OSM to build routes using routes v2 Feb 24, 2021
shashankram
shashankram approved these changes Feb 24, 2021
View reviewed changes
demo/deploy-bookstore.sh
@@ -23,6 +23,8 @@ spec:
ports:
- port: 80
name: bookstore-port
selector:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this isn't needed but putting it here doesn't cause any harm for this demo.

tests/e2e/e2e_http_ingress_test.go Outdated Show resolved Hide resolved
@shashankram shashankram changed the title fest(routesv2) : Cut over OSM to build routes using routes v2 feat(routesv2) : Cut over OSM to build routes using routes v2 Feb 24, 2021
@snehachhabria
feat(routesv2): Cut over OSM to build routes using routes v2
bcf2f90 
This PR makes routes v2 the default route building mechanism in OSM.

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
draychev
draychev approved these changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@draychev draychev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @snehachhabria

michelleN
michelleN approved these changes Feb 24, 2021
View reviewed changes
tests/scenarios/traffic_split_with_apex_service_test.go
@@ -129,9 +135,9 @@ var _ = Describe(``+

expectedWeightedCluster := &xds_route.WeightedCluster{
Clusters: []*xds_route.WeightedCluster_ClusterWeight{
weightedCluster("bookstore-v2", 10),
weightedCluster("bookstore-v2", 100),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious to know why we changed these weights

Copy link
Contributor Author

@snehachhabria snehachhabria Feb 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because in routes v2 we build the outbound policies with default weights :

osm/pkg/catalog/routes.go

Line 695 in 8a19278

weightedCluster := getDefaultWeightedClusterForService(destService)

The weights will only get updated for outbound virtual hosts that are referenced in traffic splits, otherwise it always remains 100. This is different from routes v1 and hence the change in weights

@snehachhabria snehachhabria merged commit 95ed095 into openservicemesh:main Feb 24, 2021
@snehachhabria snehachhabria deleted the Routesv2Cutover branch February 24, 2021 20:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@draychev draychev draychev approved these changes

@shashankram shashankram shashankram approved these changes

@michelleN michelleN michelleN approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@snehachhabria @michelleN @shashankram @draychev