Add iteration support to etcd #52199

rosstimothy · 2025-02-14T19:57:08Z

Introduces a new Items(context.Context, backend.IterateParams) iter.Seq2[backend.Item, error] to the etcd backend. The core logic for this function was moved from GetRange, and GetRange now defers to calling Iterate and collecting a slice of items.

The motivation for this change is to attempt to reduce the complexity of pagination for consumers of GetRange. While backend.IterateRange and backend.StreamRange do already exists to serve the same purpose, they too have suffered from pagination bugs in the past. Additionally, this aims to unify iteration to using the iter package instead of the various homegrown iteration mechanisms that exist throughout the code base.

There is also one distinct difference to the iteration api: it allows retrieving items within the specified range in ascending or descending key order. While there may not be many use cases for this today, exposing this in the api from it's inception is easier than adding functional options, or IterateAscending/IterateDescending later.

rosstimothy · 2025-02-14T20:46:53Z

I attempted to determine the performance impact of this change by running a modified version of BenchmarkGetNodes to run against etcd and for the sake of time a single node count.

goos: darwin
goarch: arm64
pkg: github.com/gravitational/teleport/lib/services/local
cpu: Apple M2 Pro
                                       │ master.txt  │            new.txt            │
                                       │   sec/op    │   sec/op     vs base          │
GetNodes/tt(nodes=2000)-12   178.5m ± 2%   177.3m ± 1%  ~ (p=0.739 n=10)

                                       │  master.txt  │               new.txt               │
                                       │     B/op     │     B/op      vs base               │
GetNodes/tt(nodes=2000)-12   11.83Mi ± 4%   12.35Mi ± 2%  +4.37% (p=0.000 n=10)

                                       │ master.txt  │              new.txt               │
                                       │  allocs/op  │  allocs/op   vs base               │
GetNodes/tt(nodes=2000)-12   148.0k ± 0%   148.0k ± 0%  +0.03% (p=0.002 n=10)```

lib/backend/etcdbk/etcd.go

lib/backend/backend.go

espadolini

If we want to follow https://pkg.go.dev/iter#hdr-Naming_Conventions we should call the method Range or something like that, shouldn't we?

lib/backend/backend.go

lib/backend/etcdbk/etcd.go

lib/backend/backend.go

lib/backend/etcdbk/etcd.go

fspmarshall

LGTM once error handling is improved.

lib/backend/etcdbk/etcd.go

Introduces a new `Items(context.Context, backend.IterateParams) iter.Seq2[backend.Item, error]` to the etcd backend. The core logic for this function was moved from GetRange, and GetRange now defers to calling Items and collecting a slice of items. The motivation for this change is to attempt to reduce the complexity of pagination for consumers of GetRange. While backend.IterateRange and backend.StreamRange do already exists to serve the same purpose, they too have suffered from pagination bugs in the past. Additionally, this aims to unify iteration to using the iter package instead of the various homegrown iteration mechanisms that exist throughout the code base. There is also one distinct difference to the iteration api: it allows retrieving items within the specified range in ascending or descending key order. While there may not be many use cases for this today, exposing this in the api from it's inception is easier than adding functional options, or ItemsAscending/ItemsDescending later.

This builds on top of #52199 by updating the dynamo backend to implement backend.BackendWithItems. Both GetRange and DeleteRange were refactored to call Items instead of getAllRecords to unify logic and vet the implementation of Items. The custom pagination logic to retrieve items was also removed in favor of the built in query paginator from the aws sdk. In addition to simplifying logic this also removed some extraneous sorting.

This builds on top of #52199 by updating the dynamo backend to implement backend.BackendWithItems. Only GetRange, and not DeletRange, was refactored to use Items to retrieve a range.

This builds on top of #52199 by updating the memory backend to implement backend.BackendWithItems. Only GetRange, and not DeletRange, was refactored to use Items to retrieve a range.

This builds on top of #52199 by updating the sqlite backend to implement backend.BackendWithItems. Only GetRange, and not DeletRange, was refactored to use Items to retrieve a range.

This builds on top of #52199 by updating the postgres backend to implement backend.BackendWithItems. Only GetRange, and not DeletRange, was refactored to use Items to retrieve a range.

This builds on top of #52199 by updating the firestore backend to implement backend.BackendWithItems. Only GetRange, and not DeletRange, was refactored to use Items to retrieve a range. The iteration logic is slightly more complex here to ensure that documents in the collection are produced in the correct order no matter how the key was stored in the collection. Over the years the key has been stored in three different formats, and no single query will pull the items from all keys. There was a background migration task introduced in v17 to convert all keys to a single format. The iteration logic can be simplified once said migration is removed and all keys can be assumed to be of a single type.

This builds on top of #52199 by updating the sqlite backend to implement backend.BackendWithItems. Only GetRange, and not DeletRange, was refactored to use Items to retrieve a range.

This builds on top of #52199 by updating the postgres backend to implement backend.BackendWithItems. Only GetRange, and not DeletRange, was refactored to use Items to retrieve a range.

This builds on top of #52199 by updating the memory backend to implement backend.BackendWithItems. Only GetRange, and not DeletRange, was refactored to use Items to retrieve a range.

This builds on top of #52199 by updating the postgres backend to implement backend.BackendWithItems. Only GetRange, and not DeletRange, was refactored to use Items to retrieve a range.

This builds on top of #52199 by updating the sqlite backend to implement backend.BackendWithItems. Only GetRange, and not DeletRange, was refactored to use Items to retrieve a range.

This builds on top of #52199 by updating the dynamo backend to implement backend.BackendWithItems. Both GetRange and DeleteRange were refactored to call Items instead of getAllRecords to unify logic and vet the implementation of Items. The custom pagination logic to retrieve items was also removed in favor of the built in query paginator from the aws sdk. In addition to simplifying logic this also removed some extraneous sorting.

This builds on top of #52199 by updating the memory backend to implement backend.BackendWithItems. Only GetRange, and not DeletRange, was refactored to use Items to retrieve a range.

This builds on top of #52199 by updating the dynamo backend to implement backend.BackendWithItems. Both GetRange and DeleteRange were refactored to call Items instead of getAllRecords to unify logic and vet the implementation of Items. The custom pagination logic to retrieve items was also replaced with an iterator that works in a similar fashio to the query paginator from the aws sdk. In addition to simplifying logic this also removed some extraneous sorting.

This builds on top of #52199 by updating the firestore backend to implement backend.BackendWithItems. Only GetRange, and not DeletRange, was refactored to use Items to retrieve a range. The iteration logic is slightly more complex here to ensure that documents in the collection are produced in the correct order no matter how the key was stored in the collection. Over the years the key has been stored in three different formats, and no single query will pull the items from all keys. There was a background migration task introduced in v17 to convert all keys to a single format. The iteration logic can be simplified once said migration is removed and all keys can be assumed to be of a single type.

rosstimothy added the no-changelog Indicates that a PR does not require a changelog entry label Feb 14, 2025

rosstimothy force-pushed the tross/backend_iterate_etcd branch from fbff95f to 2d169d0 Compare February 14, 2025 20:44

rosstimothy force-pushed the tross/backend_iterate_etcd branch from 2d169d0 to 54cdcff Compare February 14, 2025 21:15

rosstimothy requested review from fspmarshall and espadolini February 14, 2025 21:37

rosstimothy marked this pull request as ready for review February 14, 2025 21:37

github-actions bot added the size/md label Feb 14, 2025

github-actions bot requested a review from GavinFrazar February 14, 2025 21:37

fspmarshall reviewed Feb 14, 2025

View reviewed changes

lib/backend/etcdbk/etcd.go Outdated Show resolved Hide resolved

lib/backend/etcdbk/etcd.go Outdated Show resolved Hide resolved

fspmarshall reviewed Feb 14, 2025

View reviewed changes

lib/backend/backend.go Outdated Show resolved Hide resolved

espadolini reviewed Feb 20, 2025

View reviewed changes

lib/backend/backend.go Outdated Show resolved Hide resolved

lib/backend/etcdbk/etcd.go Outdated Show resolved Hide resolved

rosstimothy force-pushed the tross/backend_iterate_etcd branch from 6c241ca to d8a4268 Compare February 20, 2025 16:26

rosstimothy requested review from fspmarshall and espadolini February 20, 2025 17:51

espadolini reviewed Feb 24, 2025

View reviewed changes

lib/backend/backend.go Outdated Show resolved Hide resolved

lib/backend/backend.go Outdated Show resolved Hide resolved

lib/backend/etcdbk/etcd.go Outdated Show resolved Hide resolved

fspmarshall approved these changes Feb 24, 2025

View reviewed changes

lib/backend/etcdbk/etcd.go Outdated Show resolved Hide resolved

rosstimothy requested a review from espadolini February 24, 2025 20:16

espadolini reviewed Feb 24, 2025

View reviewed changes

lib/backend/etcdbk/etcd.go Outdated Show resolved Hide resolved

rosstimothy force-pushed the tross/backend_iterate_etcd branch from b16e09f to 0f9445a Compare February 25, 2025 14:32

espadolini reviewed Feb 25, 2025

View reviewed changes

lib/backend/etcdbk/etcd.go Outdated Show resolved Hide resolved

lib/backend/etcdbk/etcd.go Outdated Show resolved Hide resolved

espadolini approved these changes Feb 25, 2025

View reviewed changes

public-teleport-github-review-bot bot removed the request for review from GavinFrazar February 25, 2025 16:15

rosstimothy force-pushed the tross/backend_iterate_etcd branch from b9dea9e to b4dd722 Compare February 25, 2025 16:28

rosstimothy force-pushed the tross/backend_iterate_etcd branch from b4dd722 to e97a438 Compare February 25, 2025 17:36

rosstimothy enabled auto-merge February 25, 2025 17:37

rosstimothy added this pull request to the merge queue Feb 25, 2025

Merged via the queue into master with commit 6e9a036 Feb 25, 2025
41 checks passed

rosstimothy deleted the tross/backend_iterate_etcd branch February 25, 2025 18:16

rosstimothy mentioned this pull request Feb 26, 2025

Add iteration support to dynamo #52517

Merged

rosstimothy mentioned this pull request Feb 26, 2025

Add iteration support to sqlite backend #52530

Merged

rosstimothy mentioned this pull request Feb 27, 2025

Add iteration support to memory backend #52575

Merged

rosstimothy mentioned this pull request Feb 27, 2025

Add iteration support to postgres backend #52589

Merged

rosstimothy mentioned this pull request Feb 27, 2025

Add iteration support to firestore #52597

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add iteration support to etcd #52199

Add iteration support to etcd #52199

Uh oh!

rosstimothy commented Feb 14, 2025 •

edited

Loading

Uh oh!

rosstimothy commented Feb 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

espadolini left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fspmarshall left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add iteration support to etcd #52199

Add iteration support to etcd #52199

Uh oh!

Conversation

rosstimothy commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rosstimothy commented Feb 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

espadolini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fspmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rosstimothy commented Feb 14, 2025 •

edited

Loading