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.

Sign up for GitHub

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

Jump to bottom

fix(xds): use stable hashes for outbound cluster names #8081

Merged
merged 13 commits into from
Oct 23, 2023
Merged

fix(xds): use stable hashes for outbound cluster names #8081

merged 13 commits into from
Oct 23, 2023

Conversation

michaelbeaumont
Copy link
Contributor

Checklist prior to review

  • Link to relevant issue as well as docs and UI issues --
  • This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as a image registry) and it will work on Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
  • Tests (Unit test, E2E tests, manual test on universal and k8s) --
    • Don't forget ci/ labels to run additional/fewer tests
  • Do you need to update UPGRADE.md? --
  • Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label) --

@michaelbeaumont michaelbeaumont added the ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully) label Oct 20, 2023
@michaelbeaumont
feat(kuma-cp): use stable hashes for outbound cluster names
b862985 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
test: update
ce56d7c 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
feat(Mesh*Route): use hashes for cluster names
e779c9b 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
refactor: simplify DestinationClusterName
f711822 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
test: update
a46c062 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont michaelbeaumont changed the title feat(kuma-cp): use stable hashes for outbound cluster names fix(kuma-cp): use stable hashes for outbound cluster names Oct 20, 2023
@michaelbeaumont michaelbeaumont linked an issue Oct 23, 2023 that may be closed by this pull request
unify gateway and DP cluster naming #5793
Closed
@michaelbeaumont michaelbeaumont force-pushed the feat/cluster_name_hash branch 2 times, most recently from 9c9e6a8 to 24cd5e7 Compare October 23, 2023 09:54
@michaelbeaumont
refactor: use new split cluster name format in policies
97e78d6 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
test(MeshCircuitBreaker): adjust for new cluster names
fda0b66 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
test(e2e): fix cluster name dependent assertions
d9c2844 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
fix(metrics): adjust hijacker
1189000 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
test(metrics): adjust
213eb47 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont michaelbeaumont marked this pull request as ready for review October 23, 2023 11:54
@michaelbeaumont michaelbeaumont requested a review from a team as a code owner October 23, 2023 11:54
@michaelbeaumont michaelbeaumont requested review from jakubdyszkiewicz and Automaat and removed request for a team October 23, 2023 11:54
lahabana
lahabana reviewed Oct 23, 2023
View reviewed changes
Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

The change makes sense to me. Let's make sure the orders are maintained too so that we avoid accidental snapshot invalidation.
I'm wondering if this might solve some of the flakes we've seen in the past

@michaelbeaumont
test(MeshCircuitBreaker): use correct number of hex digits for cluster
58db476 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
lobkovilya
lobkovilya approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@lobkovilya lobkovilya left a comment

Choose a reason for hiding this comment

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

LGTM

app/kuma-dp/pkg/dataplane/metrics/merge.go Outdated Show resolved Hide resolved
app/kuma-dp/pkg/dataplane/metrics/merge.go Outdated Show resolved Hide resolved
pkg/plugins/policies/core/xds/clusters.go Outdated Show resolved Hide resolved
pkg/plugins/policies/core/xds/meshroute/listeners.go Show resolved Hide resolved
@michaelbeaumont
refactor: keep regex and format string private to package
1936405 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
docs: improve comments
0da38f1 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont michaelbeaumont merged commit 7659385 into kumahq:master Oct 23, 2023
5 checks passed
@michaelbeaumont michaelbeaumont deleted the feat/cluster_name_hash branch October 23, 2023 16:29
@lahabana lahabana changed the title fix(kuma-cp): use stable hashes for outbound cluster names fix(xds): use stable hashes for outbound cluster names Nov 3, 2023
@BrewTestBot BrewTestBot mentioned this pull request Nov 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lahabana lahabana lahabana left review comments

@jakubdyszkiewicz jakubdyszkiewicz jakubdyszkiewicz approved these changes

@lobkovilya lobkovilya lobkovilya approved these changes

@Automaat Automaat Awaiting requested review from Automaat Automaat is a code owner automatically assigned from kumahq/kuma-maintainers

Assignees
No one assigned
Labels
ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

unify gateway and DP cluster naming
4 participants
@michaelbeaumont @lahabana @jakubdyszkiewicz @lobkovilya