Skip to content

rgw: Reduce data sync parallelism in response to RADOS lock latency#48451

Closed
adamemerson wants to merge 2 commits intoceph:mainfrom
adamemerson:wip-rgw-sync-latency-spawnwindow
Closed

rgw: Reduce data sync parallelism in response to RADOS lock latency#48451
adamemerson wants to merge 2 commits intoceph:mainfrom
adamemerson:wip-rgw-sync-latency-spawnwindow

Conversation

@adamemerson
Copy link
Contributor

@adamemerson adamemerson commented Oct 11, 2022

Lock latency in RGWContinuousLeaseCR gets high enough under load that the locks end up timing out, leading to incorrect behavior.

Monitor lock latency and cut concurrent operations in half if it goes above ten seconds.

Cut currency to one if it goes about twenty seconds.

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@adamemerson adamemerson requested a review from cbodley October 11, 2022 18:54
@github-actions github-actions bot added the rgw label Oct 11, 2022
@cbodley cbodley requested review from ofriedma and yuvalif October 11, 2022 19:09
@adamemerson adamemerson force-pushed the wip-rgw-sync-latency-spawnwindow branch from c0b62dd to 5e2ef27 Compare October 11, 2022 20:35
@adamemerson adamemerson requested a review from cbodley October 11, 2022 20:36
@adamemerson adamemerson force-pushed the wip-rgw-sync-latency-spawnwindow branch from 5e2ef27 to b93e394 Compare October 11, 2022 20:38
Lock latency in RGWContinuousLeaseCR gets high enough under load that
the locks end up timing out, leading to incorrect behavior.

Monitor lock latency and cut concurrent operations in half if it goes
above ten seconds.

Cut currency to one if it goes about twenty seconds.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
@adamemerson adamemerson force-pushed the wip-rgw-sync-latency-spawnwindow branch from b93e394 to 2b2464f Compare October 11, 2022 20:41
Limited to onlly warn every five minutes.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
/// cut it to 1.
int64_t adj_concurrency(int64_t concurrency) {
using namespace std::literals;
auto threshold = (cct->_conf->rgw_sync_lease_period * 1s) / 12;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbodley I was thinking about this, and I think making this scale with the least time might be the wrong idea.

If we have a twenty minute lease time then ten or twenty seconds for the lock action to complete is still pretty egregious and suggests the we're overloading the OSD, and with it scaling this way we'd end up only throttling back with hundred second average latency.

Copy link
Contributor

Choose a reason for hiding this comment

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

(cc @mattbenjamin)

considering a deployment with one radosgw per zone where we could omit the leases entirely, this latency shouldn't matter at all - it could be 10 minutes or an hour, and sync would chug along (albeit slowly) without any errors

it wasn't until the RGWContinuousLeaseCR::is_locked() change from #47728 that this latency turned into actual sync errors. because this lease timer is the only thing that's sensitive to latency, i suggested that the throttling logic should aim to keep that latency in a range where we can actually make progress

however, lacking a better model for flow control or throttling here, maybe we should just revert that is_locked() part of #47728 - as-is, that seems to be making sync less reliable because it's shown that we have no control over this latency in our testing

i'm open to exploring band-aids here just to ship something, but this is complicated and it's hard for me to tell whether we're making things better or worse. even with your super-conservative throttling, we were still seeing extreme latencies here, right? that suggests the load is coming from somewhere else like ingest or /admin/log requests

Copy link
Contributor

Choose a reason for hiding this comment

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

@cbodley I guess let's review today and go with your and @adamemerson 's intuition on this for 5.3, at least

@cbodley
Copy link
Contributor

cbodley commented Nov 23, 2022

included in #48898

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants