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

kvflowcontrol,admission: productionize replication admission control #98703

Closed
15 tasks done
irfansharif opened this issue Mar 15, 2023 · 1 comment
Closed
15 tasks done

kvflowcontrol,admission: productionize replication admission control #98703

irfansharif opened this issue Mar 15, 2023 · 1 comment
Labels
A-admission-control C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team

Comments

@irfansharif
Copy link
Contributor

irfansharif commented Mar 15, 2023 •
edited
Loading

Is your feature request related to a problem? Please describe.

Tracking issue to productionize #95563 and rolling it out into the wild (enabled by default, made safe-to-opt-into for production clusters):

  • Merge #98308, which integrate various kvflowcontrol,admission components end-to-end gated by cluster settings.
    • Support a "flow token tracking only" mode where we do end-to-end flow control token tracking but don't actually block at admit time due to lack of requisite flow tokens. It'll let us look at production systems and understand that we are losing performance isolation due to a lack of write flow control.
    • Support a "flow tokens only for elastic traffic" mode, to use flow control only for elastic traffic (index backfills, etc).
    • Backport as disabled-entirely to 23.1 release branch.
  • Add randomized/integration testing to verify we don't leak flow tokens, leakage that could result in complete write throughput collapse. We want to test all the interactions listed here, which include the raft transport stream breaking, nodes crashing, followers being paused/unpaused, caught up via snapshots or post-restart log appends, leaseholder/leadership changes, prolonged leaseholder != leader scenarios, replicas being GC-ed, command reproposals, lossy raft transport, ranges splitting/merging, log truncations, and raft membership changes.
  • Add roachtest(s) to quantify the impact of index backfills with and without replication admission control, and make sure we don't regress.
  • Enable "flow tokens for {regular,elastic} traffic" on 23.2 master.
  • Monitor and address CI fallout for two-ish weeks on master. Backport any bug fixes to 23.1 (where it's disabled by default).
  • Roll out the "flow tokens only for elastic traffic" to test-only/POC 23.1 clusters for actual clients, on CC or otherwise.
  • Roll out the "flow token tracking only" mode (described above) to 23.1 CC clusters.

Jira issue: CRDB-25455

Epic CRDB-25348
@irfansharif irfansharif added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-admission-control labels Mar 15, 2023
@irfansharif irfansharif mentioned this issue Mar 15, 2023
Closed
10 tasks
@exalate-issue-sync exalate-issue-sync bot added the T-kv KV Team label Mar 15, 2023
@irfansharif irfansharif mentioned this issue May 23, 2023
Merged
craig bot pushed a commit that referenced this issue May 26, 2023
@irfansharif
Merge #103757
bd04744 
103757: roach{prod,test}: add first-class support for disk snapshots r=irfansharif a=irfansharif

Part of #89978. Pre-cursor to #83826. Part of #98703.

Long-lived disk snapshots can drastically reduce testing time for scale tests. Tests, whether run by hand or through CI, need only run the long running fixture generating code (importing some dataset, generating it organically through workload, etc.) once snapshot fingerprints are changed, fingerprints that incorporate the major crdb version that generated them.

Here's an example run that freshly generates disk snapshots:

    === RUN   admission-control/index-backfill
    no existing snapshots found for admission-control/index-backfill (ac-index-backfill), doing pre-work
    created volume snapshot ac-index-backfill-0001-vunknown-1-n2-standard-8 for volume irfansharif-snapshot-0001-1 on irfansharif-snapshot-0001-1/n1
    using 1 newly created snapshot(s) with prefix "ac-index-backfill"
    detached and deleted volume irfansharif-snapshot-0001-1 from irfansharif-snapshot-0001
    created volume irfansharif-snapshot-0001-1
    attached volume irfansharif-snapshot-0001-1 to irfansharif-snapshot-0001
    mounted irfansharif-snapshot-0001-1 to irfansharif-snapshot-0001
    --- PASS: admission-control/index-backfill (79.14s)

Here's a subsequent run that makes use of the aforementioned disk snapshots:

    === RUN   admission-control/index-backfill
    using 1 pre-existing snapshot(s) with prefix "ac-index-backfill"
    detached and deleted volume irfansharif-snapshot-0001-1 from irfansharif-snapshot-0001
    created volume irfansharif-snapshot-0001-1
    attached volume irfansharif-snapshot-0001-1 to irfansharif-snapshot-0001
    mounted irfansharif-snapshot-0001-1 to irfansharif-snapshot-0001
    --- PASS: admission-control/index-backfill (43.47s)

We add the following APIs to the roachtest.Cluster interface, for tests to interact with disk snapshots. admission-control/index-backfill is an example test making use of these APIs.
```go
  type Cluster interface {
      // ...

      // CreateSnapshot creates volume snapshots of the cluster using
      // the given prefix. These snapshots can later be retrieved,
      // deleted or applied to already instantiated clusters.
      CreateSnapshot(ctx context.Context, snapshotPrefix string) error

      // ListSnapshots lists the individual volume snapshots that
      // satisfy the search criteria.
      ListSnapshots(
        ctx context.Context, vslo vm.VolumeSnapshotListOpts,
      ) ([]vm.VolumeSnapshot, error)

      // DeleteSnapshots permanently deletes the given snapshots.
      DeleteSnapshots(
        ctx context.Context, snapshots ...vm.VolumeSnapshot,
      ) error

      // ApplySnapshots applies the given volume snapshots to the
      // underlying cluster. This is a destructive operation as far as
      // existing state is concerned - all already-attached volumes are
      // detached and deleted to make room for new snapshot-derived
      // volumes. The new volumes are created using the same specs
      // (size, disk type, etc.) as the original cluster.
      ApplySnapshots(
        ctx context.Context, snapshots []vm.VolumeSnapshot,
      ) error
  }
```
These Cluster APIs are in turn is powered by the following additions to the vm.Provider interface, implemented by each cloud provider. GCE is the fully spec-ed out one for now.
```go
  type Provider interface {
      // ...

      // CreateVolume creates a new volume using the given options.
      CreateVolume(l *logger.Logger, vco VolumeCreateOpts) (Volume, error)

      // ListVolumes lists all volumes already attached to the given VM.
      ListVolumes(l *logger.Logger, vm *VM) ([]Volume, error)

      // DeleteVolume detaches and deletes the given volume from the
      // given VM.
      DeleteVolume(l *logger.Logger, volume Volume, vm *VM) error

      // AttachVolume attaches the given volume to the given VM.
      AttachVolume(l *logger.Logger, volume Volume, vm *VM) (string, error)

      // CreateVolumeSnapshot creates a snapshot of the given volume,
      // using the given options.
      CreateVolumeSnapshot(
        l *logger.Logger, volume Volume, vsco VolumeSnapshotCreateOpts,
      ) (VolumeSnapshot, error)

      // ListVolumeSnapshots lists the individual volume snapshots that
      // satisfy the search criteria.
      ListVolumeSnapshots(
        l *logger.Logger, vslo VolumeSnapshotListOpts,
      ) ([]VolumeSnapshot, error)

      // DeleteVolumeSnapshot permanently deletes the given snapshot.
      DeleteVolumeSnapshot(l *logger.Logger, snapshot VolumeSnapshot) error
  }
```
Since these snapshots necessarily outlive the tests, and we don't want them dangling perpetually, we introduce a prune-dangling roachtest that acts as a poor man's cron job, sifting through expired snapshots (>30days) and deleting them. For GCE at least it's not obvious to me how to create these snapshots in cloud buckets with a TTL built in, hence this hack. It looks like this (with change to the TTL):

    === RUN   prune-dangling
    pruned old snapshot ac-index-backfill-0001-vunknown-1-n2-standard-8
    --- PASS: prune-dangling (8.59s)

---

We add expose some of these APIs through the roachprod binary directly.
```
$ roachprod snapshot --help
  snapshot enables creating/listing/deleting/applying cluster snapshots

  Usage:
    roachprod snapshot [command]

  Available Commands:
    create      snapshot a named cluster, using the given snapshot name and description
    list        list all snapshots for the given cloud provider, optionally filtering by the given name
    delete      delete all snapshots for the given cloud provider optionally filtering by the given name
    apply       apply the named snapshots from the given cloud provider to the named cluster
```
---

About admission-control/index-backfill. It's a fully featured test that uses the TPC-C 100k dataset and runs a foreground load for 20k customers. It takes >4hrs to import this data set; with disk snapshots this step is skipped entirely and takes a few minutes. The actual test is trivial, we run the foreground load for 1hr and run a large index backfill concurrently. Before #98308, this results in wild performance oscillations. It's still a bit wild after flow control, but less so.

We slightly extend the tpc-e harness to make this happen, adding a few smarts: exposing a 'during' helper to run backfills concurrently with foreground load, integrate with --skip-init, estimated setup times, prometheus, and disk snapshots of course.

Release note: None

Co-authored-by: irfan sharif <[email protected]>
irfansharif added a commit to irfansharif/cockroach that referenced this issue Jun 10, 2023
@irfansharif
kvserver: fix clearrange/* tests
154b8d2 
Fixes cockroachdb#104696.
Fixes cockroachdb#104697.
Fixes cockroachdb#104698.
Part of cockroachdb#98703.

In 072c16d (added as part of cockroachdb#95637) we re-worked the locking
structure around the RaftTransport's per-RPC class level send queues.
When new send queues are instantiated or old ones deleted, we now also
maintain the kvflowcontrol connection tracker, so such maintenance now
needs to happen while holding a kvflowcontrol mutex. When rebasing
\cockroachdb#95637 onto master, we accidentally included earlier queue deletion
code without holding the appropriate mutex. Queue deletions now happened
twice which made it possible to hit a RaftTransport assertion about
expecting the right send queue to already exist.

Specifically, the following sequence was possible:
- (*RaftTransport).SendAsync is invoked, observes no queue for
  <nodeid,class>, creates it, and tracks it in the queues map.
  - It invokes an async worker W1 to process that send queue through
    (*RaftTransport).startProcessNewQueue. The async worker is
    responsible for clearing the tracked queue in the queues map once
    done.
- W1 expects to find the tracked queue in the queues map, finds it,
  proceeds.
- W1 is done processing. On its way out, W1 clears <nodeid,class> from
  the queues map the first time.
- (*RaftTransport).SendAsync is invoked by another goroutine, observes
  no queue for <nodeid,class>, creates it, and tracks it in the queues
  map.
  - It invokes an async worker W2 to process that send queue through
    (*RaftTransport).startProcessNewQueue. The async worker is
    responsible for clearing the tracked queue in the queues map once
    done.
- W1 blindly clears the <nodeid,class> raft send queue the second time.
- W2 expects to find the queue in the queues map, but doesn't, and
  fatals.

Release note: None
@irfansharif irfansharif mentioned this issue Jun 10, 2023
Merged
craig bot pushed a commit that referenced this issue Jun 10, 2023
@irfansharif
Merge #104699
6e38f67 
104699: kvserver: fix clearrange/* tests r=irfansharif a=irfansharif

Fixes #104696.
Fixes #104697.
Fixes #104698.
Part of #98703.

In 072c16d (added as part of #95637) we re-worked the locking structure around the RaftTransport's per-RPC class level send queues. When new send queues are instantiated or old ones deleted, we now also maintain the kvflowcontrol connection tracker, so such maintenance now needs to happen while holding a kvflowcontrol mutex. When rebasing \#95637 onto master, we accidentally included earlier queue deletion code without holding the appropriate mutex. Queue deletions now happened twice which made it possible to hit a RaftTransport assertion about expecting the right send queue to already exist.

Specifically, the following sequence was possible:
- `(*RaftTransport).SendAsync` is invoked, observes no queue for `<nodeid,class>`, creates it, and tracks it in the queues map.
  - It invokes an async worker W1 to process that send queue through `(*RaftTransport).startProcessNewQueue`. The async worker is responsible for clearing the tracked queue in the queues map once done.
- W1 expects to find the tracked queue in the queues map, finds it, proceeds.
- W1 is done processing. On its way out, W1 clears `<nodeid,class>` from the queues map the first time.
- `(*RaftTransport).SendAsync` is invoked by another goroutine, observes no queue for <nodeid,class>, creates it, and tracks it in the queues map.
  - It invokes an async worker W2 to process that send queue through `(*RaftTransport).startProcessNewQueue`. The async worker is responsible for clearing the tracked queue in the queues map once done.
- W1 blindly clears the `<nodeid,class>` raft send queue the second time.
- W2 expects to find the queue in the queues map, but doesn't, and fatals.

Release note: None

Co-authored-by: irfan sharif <[email protected]>
@irfansharif irfansharif mentioned this issue Jun 11, 2023
Closed
@irfansharif
Copy link
Contributor Author

#110036 is the last of it.

@github-project-automation github-project-automation bot moved this to Closed in KV Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-admission-control C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team
Projects
No open projects
KV
Archived in project
Milestone
No milestone
Development

No branches or pull requests
1 participant
@irfansharif