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.

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

control_plane/attachment_service: implement PlacementPolicy::Secondary, configuration updates #6521

Merged
merged 15 commits into from
Mar 1, 2024

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented Jan 30, 2024

Problem

During onboarding, the control plane may attempt ad-hoc creation of a secondary location to facilitate live migration. This gives us two problems to solve:

  • Accept 'Secondary' mode in /location_config and use it to put the tenant into secondary mode on some physical pageserver, then pass through /tenant/xyz/secondary/download requests
  • Create tenants with no generation initially, since the initial Secondary mode call will not provide us a generation.

This PR also fixes modification of a tenant's TenantConf during /location_conf, which was previously ignored, and refines the flow for config modification:

  • avoid bumping generations when the only reason we're reconciling an attached location is a config change
  • increment TenantState.sequence when spawning a reconciler: usually schedule() does this, but when we do config changes that doesn't happen, so without this change waiters would think reconciliation was done immediately. sequence is a bit of a murky thing right now, as it's dual-purposed for tracking waiters, and for checking if an existing reconciliation is already making updates to our current sequence. I'll follow up at some point to clarify it's purpose.
  • test config modification at the end of onboarding test

Summary of changes

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@jcsp jcsp added t/feature Issue type: feature, for new features or requests c/storage Component: storage labels Jan 30, 2024
Copy link

github-actions bot commented Jan 30, 2024

2484 tests run: 2361 passed, 0 failed, 123 skipped (full report)


Flaky tests (1)

Postgres 15

  • test_remote_storage_upload_queue_retries: debug

Code coverage* (full report)

  • functions: 28.7% (6931 of 24161 functions)
  • lines: 47.2% (42560 of 90170 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
d6f6cd9 at 2024-03-01T20:23:44.355Z :recycle:

@jcsp jcsp force-pushed the jcsp/attachment-service-v6-secondary branch 3 times, most recently from 4313c0a to 06bb013 Compare February 5, 2024 13:38
@jcsp jcsp changed the title control_plane/attachment_service: implement PlacementPolicy::Secondary control_plane/attachment_service: implement PlacementPolicy::Secondary, configuration updates Feb 29, 2024
@jcsp jcsp force-pushed the jcsp/attachment-service-v6-secondary branch from 06bb013 to b74c750 Compare February 29, 2024 09:47
@jcsp jcsp requested a review from VladLazar February 29, 2024 09:55
@jcsp jcsp marked this pull request as ready for review February 29, 2024 09:56
@jcsp jcsp requested a review from a team as a code owner February 29, 2024 09:56
@jcsp jcsp force-pushed the jcsp/attachment-service-v6-secondary branch from bbf87f5 to 4abbfdc Compare February 29, 2024 15:30
Copy link
Contributor

@VladLazar VladLazar left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of doing onboarding through the location config api, but as discussed out of band it makes things easier on the control plane

control_plane/attachment_service/src/reconciler.rs Outdated Show resolved Hide resolved
control_plane/attachment_service/src/service.rs Outdated Show resolved Hide resolved
control_plane/attachment_service/src/tenant_state.rs Outdated Show resolved Hide resolved
control_plane/attachment_service/src/service.rs Outdated Show resolved Hide resolved
control_plane/attachment_service/src/persistence.rs Outdated Show resolved Hide resolved
control_plane/attachment_service/src/reconciler.rs Outdated Show resolved Hide resolved
control_plane/attachment_service/src/service.rs Outdated Show resolved Hide resolved
@jcsp jcsp requested a review from VladLazar March 1, 2024 11:01
@jcsp jcsp merged commit 20d0939 into main Mar 1, 2024
53 checks passed
@jcsp jcsp deleted the jcsp/attachment-service-v6-secondary branch March 1, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/storage Component: storage t/feature Issue type: feature, for new features or requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants