Implementing generalized tree queue with node state implemented by tenant-querier assignments #7873

chencs · 2024-04-10T21:14:45Z

What this PR does

This PR heavily edits the TreeQueue data structure -- nodes now contain a DequeueAlgorithm interface, which, when implemented, manages the bulk of logic used to select nodes to dequeue from, and how to update state after the dequeue operation.

Why do we want to do this?

This makes it easy for us to configure the ordering of query components vs. tenant queues in the tree. The previous state is that, if additionalQueueDimensionsEnabled, the tree would be built as such:

root
├─ tenant-1
│  ├─ ingester
│  ├─ store-gateway
│  ├─ ingester-and-store-gateway
...
├─ tenant-n
│  ├─ ingester
│  ├─ store-gateway
│  ├─ ingester-and-store-gateway

The problem with this is that we will always dequeue from a tenant, regardless of the component. Thus, we cannot, for instance, reliably prioritize store-gateway queries when ingesters are experiencing heavy load.

With this change, if additionalQueueDimensionsEnabled and prioritizeQueryComponents, the tree would look like:

root
├─ ingester
│  ├─ tenant-1
│  ├─ ...
│  ├─ tenant-n
├─ store-gateway
│  ├─ tenant-1
│  ├─ ...
│  ├─ tenant-n
├─ ingester-and-store-gateway
│  ├─ tenant-1
│  ├─ ...
│  ├─ tenant-n

With this tree configuration, we can ensure that something is always dequeued from underloaded components if queues exist for those components. Tenant fairness is less guaranteed, since each component may have a different subset of tenant queues, but we mitigate this by having query component nodes share a global tenant order.

Note that this PR does not change the tree prioritization -- prioritizeQueryComponents is currently false everywhere

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

pkg/scheduler/queue/tenant_queues.go

pkg/scheduler/queue/tree_queueing_algorithms.go

pkg/scheduler/queue/tree_queue.go

pkg/scheduler/queue/tree_queueing_algorithms.go

francoposa · 2024-06-11T21:52:50Z

pkg/scheduler/queue/tree_queue.go

+		} else if dequeueNode == nil {
+			// no dequeue-able child found; reset checked children to 0,
+			// as we won't update state before moving on
+			n.childrenChecked = 0
+			return path, v


IMHO childrenChecked should be algorithm state, not tree state - all uses of it are in the algorithm, not the tree and we can just call to update the state here before this early return here.

It's the algo's job rather than the tree's job to a) decide if it's done selecting nodes and b) set itself to the correct state after a successful or failed dequeue.

Also there's some awkwardness with having !checkedAllNodes be a condition on the outerloop, but it never comes into play there because we will never return to the top of the loop with a case where where v is nil and checkedAllNodes is true - because we early return here.
We should do one or the other - don't early return here, or don't check that flag in the outer loop

I considered keeping childrenChecked in the state, but decided against it in the end. With tenantQuerierAssignments, we explicitly take advantage of things in state being shared across all nodes at the same level. Things like childrenChecked (and, e.g., queuePosition) apply to/are maintained by individual nodes, not shared state, so it seemed counterintuitive and possibly error-prone for implementers of DequeueAlgorithm to put them in algorithm state.

I think you're right about not returning early, I'll fix that.

jdbaldry

Docs LGTM

pkg/scheduler/queue/tenant_queues.go

pkg/scheduler/queue/tree_queueing_algorithms.go

francoposa · 2024-06-12T20:55:05Z

(done looking at the smaller stuff for now, now to deep dive on the tests)

pkg/scheduler/queue/tree_queue_test.go

francoposa · 2024-06-17T17:59:49Z

pkg/scheduler/queue/tenant_queues_test.go

+	tenant *queueTenant
+}
+
+func assertExpectedValuesOnDequeue(t *testing.T, qb *queueBroker, lastTenantIndex int, querierID QuerierID, expectedVals []dequeueVal) int {


big fan of this approach

pkg/scheduler/queue/tenant_queues.go

Logiraptor · 2024-06-26T21:29:32Z

pkg/scheduler/queue/tenant_queues.go

+	if useMultiAlgoTreeQueue {
+		tree, err = NewTree(
+			tqas,               // root; QueuingAlgorithm selects tenants
+			&roundRobinState{}, // tenant queues; QueuingAlgorithm selects query component
+			&roundRobinState{}, // query components; QueuingAlgorithm selects query from local queue
+		)
+		// An error building the tree is fatal; we must panic
+		if err != nil {
+			panic(fmt.Sprintf("error creating the tree queue: %v", err))
+		}
+	} else {


I was expecting to see another case for when prioritizeQueryComponents is true. Is that missing?

I think Franco (or maybe you) gave some feedback that this version of the tree should be a complete no-op, and we shouldn't introduce a config option without at least some test coverage. IIRC, the conclusion was, after we verify that the new tree works under production load/migrating to using it, to:

introduce prioritizeQueryComponents as a config option (along with comprehensive test coverage which doesn't currently exist)

eventually deprecate the current TreeQueue data structure

pkg/scheduler/queue/tenant_queues.go

pkg/scheduler/queue/multi_queuing_algorithm_tree_queue.go

tacole02

LGTM :)

CHANGELOG.md

pkg/scheduler/queue/tenant_querier_assignment.go

…ement

pkg/scheduler/queue/tree_queueing_algorithms.go

francoposa

mostly just looking for more comments / clarification on some test cases

I am not sure I have looked quite closely enough to confidently approve considering how big it is, I think maybe just running it locally with and without the flag would get me there, but in any case I am most comfortable with Patrick being the one to give final approval

pkg/scheduler/queue/multi_queuing_algorithm_tree_queue_test.go

pkg/scheduler/queue/tenant_querier_assignment.go

chencs force-pushed the casie/tree-queue-sketch branch 2 times, most recently from a580731 to b2ed920 Compare June 3, 2024 20:56

chencs force-pushed the casie/tree-queue-sketch branch 2 times, most recently from d8691b8 to 64b1ac8 Compare June 10, 2024 22:15

chencs changed the title ~~DRAFT: Implementing tree queue nodes that are aware of tenant-querier relationship~~ Implementing generalized tree queue that with node state implemented by tenant-querier assignments Jun 10, 2024

chencs force-pushed the casie/tree-queue-sketch branch from ccfadcf to 6294be7 Compare June 10, 2024 23:22

chencs marked this pull request as ready for review June 10, 2024 23:29

chencs requested review from a team and jdbaldry as code owners June 10, 2024 23:29

chencs force-pushed the casie/tree-queue-sketch branch 4 times, most recently from 7aaea6f to f5f461a Compare June 11, 2024 19:17

francoposa reviewed Jun 11, 2024

View reviewed changes

pkg/scheduler/queue/tenant_queues.go Outdated Show resolved Hide resolved

francoposa reviewed Jun 11, 2024

View reviewed changes

pkg/scheduler/queue/tree_queueing_algorithms.go Outdated Show resolved Hide resolved

francoposa reviewed Jun 11, 2024

View reviewed changes

pkg/scheduler/queue/tree_queue.go Outdated Show resolved Hide resolved

francoposa reviewed Jun 11, 2024

View reviewed changes

pkg/scheduler/queue/tree_queueing_algorithms.go Outdated Show resolved Hide resolved

francoposa reviewed Jun 11, 2024

View reviewed changes

chencs force-pushed the casie/tree-queue-sketch branch from 388fdb6 to ed1d14f Compare June 11, 2024 22:45

jdbaldry reviewed Jun 12, 2024

View reviewed changes

chencs force-pushed the casie/tree-queue-sketch branch 3 times, most recently from 1175dfd to 6cba727 Compare June 12, 2024 19:35

francoposa reviewed Jun 12, 2024

View reviewed changes

pkg/scheduler/queue/tenant_queues.go Outdated Show resolved Hide resolved

francoposa reviewed Jun 12, 2024

View reviewed changes

pkg/scheduler/queue/tree_queueing_algorithms.go Outdated Show resolved Hide resolved

chencs force-pushed the casie/tree-queue-sketch branch from 15897c1 to c46f9ff Compare June 12, 2024 21:26

francoposa reviewed Jun 12, 2024

View reviewed changes

pkg/scheduler/queue/tree_queue_test.go Outdated Show resolved Hide resolved

chencs force-pushed the casie/tree-queue-sketch branch from c46f9ff to a05f971 Compare June 12, 2024 21:33

francoposa reviewed Jun 17, 2024

View reviewed changes

chencs added 9 commits June 26, 2024 12:32

renaming args and removing config flag re: PR feedback

57fadd6

some test cleanup

9be157a

PR feedback on tests, mostly

2b45713

Patrick PR feedback: Comment and naming updates

872a80d

Re-include original tree queue with a config switch

6d7b9a6

Implement state update fn and bring back TreeQueue-specific tests

44a393d

Update CHANGELOG and flag name

ecc02ea

update docs

cd21632

Update docs

c56cdfc

chencs force-pushed the casie/tree-queue-sketch branch from 3a7af96 to c56cdfc Compare June 26, 2024 19:33

Logiraptor reviewed Jun 26, 2024

View reviewed changes

chencs changed the title ~~Implementing generalized tree queue that with node state implemented by tenant-querier assignments~~ Implementing generalized tree queue with node state implemented by tenant-querier assignments Jun 27, 2024

tacole02 approved these changes Jul 1, 2024

View reviewed changes

chencs force-pushed the casie/tree-queue-sketch branch 2 times, most recently from ca72570 to a121631 Compare July 1, 2024 22:53

Fix tenant removal on dequeue for legacy TreeQueue

bae99c7

chencs force-pushed the casie/tree-queue-sketch branch from a121631 to bae99c7 Compare July 1, 2024 23:00

remove unnecessary err check

794c3da

francoposa reviewed Jul 2, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

francoposa reviewed Jul 2, 2024

View reviewed changes

pkg/scheduler/queue/tenant_querier_assignment.go Outdated Show resolved Hide resolved

Fix CHANGELOG, add clarity to comments, and add nil check for queueEl…

a12ded4

…ement

chencs force-pushed the casie/tree-queue-sketch branch from 02d975e to a12ded4 Compare July 2, 2024 19:58

francoposa reviewed Jul 2, 2024

View reviewed changes

pkg/scheduler/queue/tree_queueing_algorithms.go Outdated Show resolved Hide resolved

pkg/scheduler/queue/tree_queueing_algorithms.go Show resolved Hide resolved

pkg/scheduler/queue/tree_queueing_algorithms.go Outdated Show resolved Hide resolved

francoposa reviewed Jul 2, 2024

View reviewed changes

tacole02 added the type/docs Improvements or additions to documentation label Jul 2, 2024

chencs added 2 commits July 2, 2024 15:58

Update docstrings and tests per PR feedback

b0c5dd4

Check tree item count after enqueues in test

da42243

Logiraptor approved these changes Jul 5, 2024

View reviewed changes

chencs merged commit 40cfafd into main Jul 5, 2024
29 checks passed

chencs deleted the casie/tree-queue-sketch branch July 5, 2024 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing generalized tree queue with node state implemented by tenant-querier assignments #7873

Implementing generalized tree queue with node state implemented by tenant-querier assignments #7873

chencs commented Apr 10, 2024 •

edited

Loading

francoposa Jun 11, 2024 •

edited

Loading

chencs Jun 11, 2024 •

edited

Loading

jdbaldry left a comment

francoposa commented Jun 12, 2024

francoposa Jun 17, 2024

Logiraptor Jun 26, 2024

chencs Jun 27, 2024

tacole02 left a comment

francoposa left a comment

Implementing generalized tree queue with node state implemented by tenant-querier assignments #7873

Implementing generalized tree queue with node state implemented by tenant-querier assignments #7873

Conversation

chencs commented Apr 10, 2024 • edited Loading

What this PR does

Why do we want to do this?

Checklist

francoposa Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

chencs Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

jdbaldry left a comment

Choose a reason for hiding this comment

francoposa commented Jun 12, 2024

francoposa Jun 17, 2024

Choose a reason for hiding this comment

Logiraptor Jun 26, 2024

Choose a reason for hiding this comment

chencs Jun 27, 2024

Choose a reason for hiding this comment

tacole02 left a comment

Choose a reason for hiding this comment

francoposa left a comment

Choose a reason for hiding this comment

chencs commented Apr 10, 2024 •

edited

Loading

francoposa Jun 11, 2024 •

edited

Loading

chencs Jun 11, 2024 •

edited

Loading