Implement Tiered Hashing with small pool sizes, kick out nodes for correctness, talk more to fast nodes and reduce pool churn #86

aarshkshah1992 · 2023-04-12T07:39:01Z

TODO

Run in staging → feat: experimental caboose pool management ipfs-inactive/bifrost-gateway#94
Unit Tests
Green CI
Investigate and fix low cache hit Saturn pcts

Subsequent PRs:

Traffic Mirroring to probe the unknown set instead of the current probabilistic mechanism ? (subsequent PR)
Use exponential decay over the entire history of the node rather than sliding window percentiles to rank nodes

willscott · 2023-04-12T07:42:00Z

caboose.go

+const (
+	BifrostProd    = "bifrost-prod"
+	BifrostStaging = "bifrost-staging"
+)
+


if this is coming in as a tag from the environment, why are we making this bifrost specific?

isn't the intention to allow caboose to be re-used for other clients, not just bifrost?

@willscott Yeah, had this for local testing. Will move to using env vars once we set them on Bifrost.

willscott

This seems fine for a next iteration

The "speed" calculation I think needs to look at bytes received versus wall clock time, rather than the throughput of individual requests

aarshkshah1992 · 2023-04-12T14:32:56Z

@willscott Not sure what you mean by that speed comment. Speed is calculated as (bytes recieved)/(end - start time).

What change do we need to make here ?

willscott · 2023-04-12T14:34:54Z

That the speed we care about is how many bytes we got over the last minute - we'll get more accuracy by adding receiver bytes into a bucket per peer and looking at it overall, rather than sampling each individual request as individual throughput measurements

aarshkshah1992 · 2023-04-12T14:36:42Z

@willscott But we add the speed of the individual request to a bucket for that peer and then use the P25 of that bucket to rank. Isn't that what you are saying here ?

willscott · 2023-04-12T14:38:32Z

You wants be directly summing Bytes received. - by dividing by time of individual request you aren't going to measure the overall bandwidth well

wip: filecoin-saturn/caboose#86

fetcher.go

willscott · 2023-05-01T13:57:40Z

caboose_test.go

+
+type HarnessOption func(config *caboose.Config)
+
+func WithTieredHashingOpts(opts []tieredhashing.Option) func(config *caboose.Config) {


Suggested change

func WithTieredHashingOpts(opts []tieredhashing.Option) func(config *caboose.Config) {

func WithTieredHashingOpts(opts []tieredhashing.Option) HarnessOption {

use the type above for these options

willscott · 2023-05-01T14:00:11Z

failure_test.go

@@ -260,6 +188,7 @@ func (e *ep) Setup() {
 	e.valid = true
 	e.resp = testBlock
 	e.server = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		time.Sleep(time.Millisecond * 20)


why is this needed? / why are responses delayed?

Just want to introduce some form of delay in the tests as this is the "L1 peer that sends us blocks".

doing so masks race conditions / makes tests artificially slow - we ideally shouldn't need it - add TODO to further clean up tests

willscott · 2023-05-01T14:01:28Z

fetcher.go

@@ -74,14 +75,17 @@ func (p *pool) doFetch(ctx context.Context, from string, c cid.Cid, attempt int)
 }

 // TODO Refactor to use a metrics collector that separates the collection of metrics from the actual fetching
-func (p *pool) fetchResource(ctx context.Context, from string, resource string, mime string, attempt int, cb DataCallback) (err error) {
+// rm will be nil only for context cancellation errors


this is confusing - it looks like it isn't nil even in the context cancellation case - it's just an empty ResponseMetrics objects?

Yeah this is a remanant of a previous iteration. Fixed.

willscott · 2023-05-01T14:02:22Z

fetcher.go

@@ -232,22 +227,35 @@ func (p *pool) fetchResource(ctx context.Context, from string, resource string,
 			}
 		}
 	}
+	req.Header.Add("User-Agent", "bifrost-"+os.Getenv(EnvironmentKey))


don't over-ride the provided user agent - there's already a more specific user agent set on the client that bifrost passes in and this will over-write it.

@willscott Can we append this to the existing UserAgent that the bifrost client has on it ? I believe L1s need this to be able to bifurcate metrics etc on their side based on environment.

appending is okay, sure

or you could set environment as another header, right?

Appended to existing

willscott · 2023-05-01T14:04:11Z

out.car

this shouldn't be committed / in history

willscott · 2023-05-01T14:09:52Z

pool.go

+	poolSizeMetric.WithLabelValues("unknown").Set(float64(mt.Unknown))
+	poolSizeMetric.WithLabelValues("main").Set(float64(mt.Main))


"unknown" / "main" also as consts

willscott · 2023-05-01T14:10:50Z

pool.go

+		aff = cidToKey(c)
+	}
+
+	p.lk.Lock()


why is this an exclusive lock?

willscott · 2023-05-01T14:11:09Z

pool.go

+		aff = path
+	}
+
+	p.lk.Lock()


likewise: why is this an exclusive lock?

Yeah agreed this can be just a read lock. Fixed.

willscott · 2023-05-01T14:16:42Z

tieredhashing/sorting.go

+	var nodes []nodeWithLatency
+
+	for n, perf := range t.nodes {
+		perf := perf


willscott · 2023-05-01T14:17:39Z

tieredhashing/tiered_hashing.go

+	"github.com/serialx/hashring"
+)
+
+// TODO Make env vars for tuning


file an issue for this if it's not in the PR

wip: filecoin-saturn/caboose#86

aarshkshah1992 added 5 commits April 12, 2023 08:59

metrics collection

5b3d567

verified cid cool down logic

e66b58d

better instrumentation

637a42c

environment metrics

2053835

fetcher

29b3c5e

willscott reviewed Apr 12, 2023

View reviewed changes

aarshkshah1992 added 6 commits April 12, 2023 13:46

debug correctness

a820872

record cache status

8f0504a

first draft

83ac33b

first draft

51463a9

fix

8ae4ff4

fix rand seed

0ef824e

willscott approved these changes Apr 12, 2023

View reviewed changes

lidel added a commit to ipfs-inactive/bifrost-gateway that referenced this pull request Apr 12, 2023

feat: improved caboose pool management

17ffc64

wip: filecoin-saturn/caboose#86

lidel mentioned this pull request Apr 12, 2023

feat: experimental caboose pool management ipfs-inactive/bifrost-gateway#94

Merged

lidel added a commit to ipfs-inactive/bifrost-gateway that referenced this pull request Apr 12, 2023

feat: improved caboose pool management (#94)

3650eef

wip: filecoin-saturn/caboose#86

aarshkshah1992 added 4 commits April 13, 2023 12:47

better pool algo

3c16c6c

fix correctness metric

e206644

iteration 2

a75a527

review

17ecb1d

lidel mentioned this pull request Apr 18, 2023

chore: update caboose ipfs-inactive/bifrost-gateway#97

Merged

willscott reviewed Apr 25, 2023

View reviewed changes

fetcher.go Outdated Show resolved Hide resolved

aarshkshah1992 added 4 commits April 26, 2023 11:47

QED

6647b9e

Merge branch 'main' into feat/reduce-pool-churn

df55432

go mod

da6bd16

static-check

76e253d

aarshkshah1992 added 10 commits April 26, 2023 14:45

change vars

31336fe

changes

d7c32c5

fix test

7591439

hacky measurement code

5c40c68

record all latencies without looking at cache hit status

69544c9

tiered hashing changes

1ff261d

green CI

6e2f4a0

uncomment test

6630349

QED

7bc03e8

tests

e527427

aarshkshah1992 changed the title ~~[WIP] Aggressive and better pool management~~ Implement Tiered Hashing will small pool sizes May 1, 2023

aarshkshah1992 changed the title ~~Implement Tiered Hashing will small pool sizes~~ Implement Tiered Hashing with small pool sizes, kick out nodes for correctness, talk more to fast nodes and reduce pool churn May 1, 2023

remove idea

0d2c3bf

willscott reviewed May 1, 2023

View reviewed changes

aarshkshah1992 added 3 commits May 2, 2023 10:28

changes as per review

159a615

set header correctly

9123325

rename var

b1bef8e

willscott approved these changes May 2, 2023

View reviewed changes

willscott merged commit 34094f8 into main May 2, 2023

willscott deleted the feat/reduce-pool-churn branch May 2, 2023 07:48

aarshkshah1992 mentioned this pull request May 2, 2023

Update Caboose to use the latest ipfs-inactive/bifrost-gateway#105

Merged

guanzo pushed a commit to guanzo/bifrost-gateway that referenced this pull request May 12, 2023

feat: improved caboose pool management (ipfs-inactive#94)

bb9ff51

wip: filecoin-saturn/caboose#86

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Tiered Hashing with small pool sizes, kick out nodes for correctness, talk more to fast nodes and reduce pool churn #86

Implement Tiered Hashing with small pool sizes, kick out nodes for correctness, talk more to fast nodes and reduce pool churn #86

aarshkshah1992 commented Apr 12, 2023 •

edited

Loading

willscott Apr 12, 2023

aarshkshah1992 Apr 12, 2023

aarshkshah1992 Apr 12, 2023

willscott left a comment

aarshkshah1992 commented Apr 12, 2023

willscott commented Apr 12, 2023

aarshkshah1992 commented Apr 12, 2023

willscott commented Apr 12, 2023

willscott May 1, 2023

aarshkshah1992 May 2, 2023

willscott May 1, 2023

aarshkshah1992 May 2, 2023

willscott May 2, 2023

willscott May 1, 2023

aarshkshah1992 May 2, 2023

willscott May 1, 2023

aarshkshah1992 May 2, 2023

willscott May 2, 2023

willscott May 2, 2023

aarshkshah1992 May 2, 2023

willscott May 1, 2023

aarshkshah1992 May 2, 2023

willscott May 1, 2023

aarshkshah1992 May 2, 2023

willscott May 1, 2023

aarshkshah1992 May 2, 2023

willscott May 1, 2023

aarshkshah1992 May 2, 2023

willscott May 1, 2023

willscott May 1, 2023

aarshkshah1992 May 2, 2023

aarshkshah1992 May 2, 2023


		type HarnessOption func(config *caboose.Config)

		func WithTieredHashingOpts(opts []tieredhashing.Option) func(config *caboose.Config) {

		poolSizeMetric.WithLabelValues("unknown").Set(float64(mt.Unknown))
		poolSizeMetric.WithLabelValues("main").Set(float64(mt.Main))

Implement Tiered Hashing with small pool sizes, kick out nodes for correctness, talk more to fast nodes and reduce pool churn #86

Implement Tiered Hashing with small pool sizes, kick out nodes for correctness, talk more to fast nodes and reduce pool churn #86

Conversation

aarshkshah1992 commented Apr 12, 2023 • edited Loading

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willscott left a comment

Choose a reason for hiding this comment

aarshkshah1992 commented Apr 12, 2023

willscott commented Apr 12, 2023

aarshkshah1992 commented Apr 12, 2023

willscott commented Apr 12, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Apr 12, 2023 •

edited

Loading