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

Queries the actual store to fetch latest snapshot while serving the URL endpoint. #675

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

abdasgupta
Copy link
Contributor

@abdasgupta abdasgupta commented Nov 1, 2023

What this PR does / why we need it:
Currently, if someone queries /snapshot/latest endpoint in backup-restore server, latest snapshots are served from whatever is stored in the already running snapshotter. But, ideally, the snapshots should be fetched from the actual backend store and then serve to the end users. This PR fetches the latest snapshots from actual store when an end user executes /snapshot/latest endpoint.

One use case of this PR is e2e tests for compaction. Full snapshots are stored in same storage by both the snapshotter and compactor. Currently, /snapshot/latest endpoint serves the latest snapshots from the details stored in snapshotter. While snapshotter has the information about what latest snapshots are pushed by itself, it does not have the information about the full snapshots pushed by compactor which runs separately. So, in e2e test for compaction, if we want to check the latest snapshots through this endpoint after a compaction job, this endpoint provides stale data about full snapshot as it only has the info about full snapshots pushed by the snapshotter.

Which issue(s) this PR fixes:
Fixes partial gardener/etcd-druid#704

Special notes for your reviewer:

Release note:

The snapshots are fetched from the actual backend store when queried for latest snapshots on `/snapshot/latest` endpoint.

@abdasgupta abdasgupta requested a review from a team as a code owner November 1, 2023 20:43
@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Nov 1, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 1, 2023
@ishan16696 ishan16696 self-assigned this Nov 2, 2023
@ishan16696
Copy link
Member

ishan16696 commented Nov 2, 2023

But, ideally, the snapshots should be fetched from the actual backend store and then serve to the end users. This PR fetches the latest snapshots from actual store when an end user executes /snapshot/latest endpoint.

but why it should be fetched from snapstore as backup-restore leader already have this information as it is taking the snapshot, so why not use that rather then go to snapstore and list the snapshots ? Have you seen any weird behaviour like when /latest doesn't give latest snapshot information ?

@abdasgupta
Copy link
Contributor Author

But, ideally, the snapshots should be fetched from the actual backend store and then serve to the end users. This PR fetches the latest snapshots from actual store when an end user executes /snapshot/latest endpoint.

but why it should be fetched from snapstore as backup-restore leader already have this information as it is taking the snapshot, so why not use that rather then go to snapstore and list the snapshots ? Have you seen any weird behaviour like when /latest doesn't give latest snapshot information ?

Yes. I have seen weird behaviour while developing the e2e tests for compaction. Ideally, a URL endpoint should always fetch data from actual backend, not from any cache or something. Full snapshots are stored in same storage by both the snapshotter and compactor. While snapshotter has the information about what latest snapshots are pushed by itself, it does not have the information about the full snapshots pushed by compactor which runs separately. So, in e2e test for compaction, if we want to check the latest snapshots through this endpoint after a compaction job, this endpoint provides stale data about full snapshot as it only have the info about full snapshots pushed by the snapshotter.

@ishan16696
Copy link
Member

While snapshotter has the information about what latest snapshots are pushed by itself, it does not have the information about the full snapshots pushed by compactor which runs separately. So, in e2e test for compaction, if we want to check the latest snapshots through this endpoint after a compaction job, this endpoint provides stale data about full snapshot as it only have the info about full snapshots pushed by the snapshotter.

thanks for clarifying, now it make sense. I think it should be mentioned in PR description as it seems important point to me.

Copy link
Member

@ishan16696 ishan16696 left a comment

Choose a reason for hiding this comment

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

LGTM!!

@abdasgupta
Copy link
Contributor Author

I updated the PR description @ishan16696

@shreyas-s-rao shreyas-s-rao added this to the v0.27.0 milestone Nov 9, 2023
@abdasgupta abdasgupta merged commit cfa43dc into gardener:master Nov 9, 2023
9 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Nov 9, 2023
@abdasgupta abdasgupta deleted the imprv-latest-ss branch November 9, 2023 06:34
abdasgupta added a commit to abdasgupta/etcd-backup-restore that referenced this pull request Nov 13, 2023
ishan16696 pushed a commit that referenced this pull request Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants