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.

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

Calculate vnodes on daprd instead of the placement service #7382

Merged
merged 23 commits into from Jan 17, 2024

Conversation

elena-kolevska
Copy link
Contributor

@elena-kolevska elena-kolevska commented Jan 15, 2024

Description

(Note: Please refer to the issue for the reason behind this PR)

This PR removes the vnodes and their sorted set from the placement table that's disseminated from the placement service to the dapr sidecars, and instead, it calculates them on the dapr sidecars.
It accounts for mismatch of versions in daprd/placement services (older daprd version with placement on 1.13+ and vice versa). In the next Dapr version we should be able to remove these checks and clean up the code.

A few notes that might help during the review:

  • replicationFactor is an argument received on placement service startup, whose default value is 100.
  • We pass the replication factor as part of the placement table message to daprd, so we can calculate the vnodes there
  • We attach the api level of the daprd as metadata to the stream connection. The placement service will look at this data and decide if it needs to send the vnodes based on it (needed for daprd sidecars on older versions). This way we're not relying on the minimum API level of the cluster, but serve data based on what the particular sidecar needs.
  • We still need to keep the vnodes in state on the placement side because of backwards compatibility.

Issue reference

#7360

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Unit tests passing
  • End-to-end tests passing

N/A

@elena-kolevska elena-kolevska changed the title Remove vnodes from placement table Calculate vnodes on daprd instead of the placement service Jan 15, 2024
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
@elena-kolevska elena-kolevska force-pushed the remove-vnodes-from-placement-table branch from 19e070b to f7adb0f Compare January 16, 2024 01:27
@elena-kolevska elena-kolevska marked this pull request as ready for review January 16, 2024 01:36
@elena-kolevska elena-kolevska requested review from a team as code owners January 16, 2024 01:36
Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Attention: 41 lines in your changes are missing coverage. Please review.

Comparison is base (f9a2088) 62.36% compared to head (db1f17c) 62.34%.
Report is 1 commits behind head on master.

Files Patch % Lines
pkg/placement/hashing/consistent_hash.go 48.83% 21 Missing and 1 partial ⚠️
pkg/placement/membership.go 46.42% 9 Missing and 6 partials ⚠️
pkg/placement/placement.go 62.50% 1 Missing and 2 partials ⚠️
pkg/actors/placement/placement.go 92.85% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7382      +/-   ##
==========================================
- Coverage   62.36%   62.34%   -0.03%     
==========================================
  Files         240      240              
  Lines       22075    22154      +79     
==========================================
+ Hits        13767    13811      +44     
- Misses       7156     7186      +30     
- Partials     1152     1157       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@artursouza artursouza left a comment

Choose a reason for hiding this comment

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

This PR warms my heart because it addresses an obvious problem that nobody dared to point out AND fix. Thanks a lot.

pkg/actors/internal/api_level.go Outdated Show resolved Hide resolved
@@ -90,7 +82,33 @@ func NewConsistentHash() *Consistent {
}

// NewFromExisting creates a new consistent hash from existing values.
func NewFromExisting(hosts map[uint64]string, sortedSet []uint64, loadMap map[string]*Host) *Consistent {
func NewFromExisting(loadMap map[string]*Host, replicationFactor int) *Consistent {
Copy link
Member

Choose a reason for hiding this comment

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

Why the old name is used for the new behavior? Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So my thinking was that we rename the old behaviour to NewFromExistingWithVirtNodes and then we remove it in the next version. And we keep the new behaviour with a name that's gonna read more naturally in this version, but also in the next ones.

pkg/placement/placement.go Outdated Show resolved Hide resolved
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
@elena-kolevska
Copy link
Contributor Author

elena-kolevska commented Jan 16, 2024

I added caching for the virtual hosts. I'll be adding a few more tests today and also I'll fix the Test_Integration/placement/apilevel/noMax/run test that I seem to have broken.

pkg/actors/placement/placement.go Outdated Show resolved Hide resolved
pkg/placement/hashing/consistent_hash.go Show resolved Hide resolved
pkg/placement/hashing/consistent_hash.go Outdated Show resolved Hide resolved
pkg/placement/hashing/consistent_hash.go Outdated Show resolved Hide resolved
pkg/placement/membership.go Show resolved Hide resolved
pkg/placement/hashing/consistent_hash.go Outdated Show resolved Hide resolved
elena-kolevska and others added 7 commits January 16, 2024 19:36
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
…n of placement tables

Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
@yaron2 yaron2 merged commit 4bea838 into dapr:master Jan 17, 2024
18 of 21 checks passed
whytem pushed a commit to whytem/dapr that referenced this pull request Jan 22, 2024
* Removes vnodes from placement table message

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds vnodes calculation on the sidecar

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Removes log line

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Backwards compatibility for  placement tables without vnodes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Update

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Fixes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Account for daprd at 1.13 and placement at < 1.13

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Missed var

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds unit tests

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Cleaning up

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Fixes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Small optimisation

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds caching for vnodes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Update pkg/placement/hashing/consistent_hash.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>

* Fixes after review

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds unit test for the Virtual nodes cache

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* lint fix

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Update tests

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Fixes linter errors

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds tests for actor service properly handling the old and new version of placement tables

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* small improvement

Signed-off-by: Elena Kolevska <elena@kolevska.com>

---------

Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
elena-kolevska added a commit to elena-kolevska/dapr that referenced this pull request Jan 23, 2024
…apr#7382)"

This reverts commit 4bea838.

Signed-off-by: Elena Kolevska <elena@kolevska.com>

# Conflicts:
#	pkg/actors/placement/client.go
#	pkg/actors/placement/client_test.go
#	pkg/placement/membership.go
#	tests/integration/suite/placement/apilevel/shared.go
#	tests/integration/suite/placement/authz/mtls.go
#	tests/integration/suite/placement/authz/nomtls.go
#	tests/integration/suite/placement/quorum/insecure.go
#	tests/integration/suite/placement/quorum/jwks.go
#	tests/integration/suite/placement/quorum/notls.go
elena-kolevska added a commit to elena-kolevska/dapr that referenced this pull request Jan 25, 2024
* Removes vnodes from placement table message

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds vnodes calculation on the sidecar

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Removes log line

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Backwards compatibility for  placement tables without vnodes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Update

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Fixes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Account for daprd at 1.13 and placement at < 1.13

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Missed var

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds unit tests

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Cleaning up

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Fixes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Small optimisation

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds caching for vnodes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Update pkg/placement/hashing/consistent_hash.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>

* Fixes after review

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds unit test for the Virtual nodes cache

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* lint fix

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Update tests

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Fixes linter errors

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds tests for actor service properly handling the old and new version of placement tables

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* small improvement

Signed-off-by: Elena Kolevska <elena@kolevska.com>

---------

Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
elena-kolevska added a commit to elena-kolevska/dapr that referenced this pull request Jan 25, 2024
* Removes vnodes from placement table message

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds vnodes calculation on the sidecar

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Removes log line

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Backwards compatibility for  placement tables without vnodes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Update

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Fixes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Account for daprd at 1.13 and placement at < 1.13

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Missed var

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds unit tests

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Cleaning up

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Fixes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Small optimisation

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds caching for vnodes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Update pkg/placement/hashing/consistent_hash.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>

* Fixes after review

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds unit test for the Virtual nodes cache

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* lint fix

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Update tests

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Fixes linter errors

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Adds tests for actor service properly handling the old and new version of placement tables

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* small improvement

Signed-off-by: Elena Kolevska <elena@kolevska.com>

---------

Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
@JoshVanL JoshVanL added this to the v1.13 milestone Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants