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

SDK option and logic to drop assist bases #3983

Merged
merged 5 commits into from
Aug 9, 2023

Conversation

ashmrtn
Copy link
Contributor

@ashmrtn ashmrtn commented Aug 7, 2023

Add a way for SDK users to drop
kopia-assisted incremental bases
thus forcing item data redownload
if the item wasn't sourced from
a merge base


Does this PR need a docs update or release note?

  • ✅ Yes, it's included
  • 🕐 Yes, but in a later PR
  • ⛔ No

Type of change

  • 🌻 Feature
  • 🐛 Bugfix
  • 🗺️ Documentation
  • 🤖 Supportability/Tests
  • 💻 CI/Deployment
  • 🧹 Tech Debt/Cleanup

Issue(s)

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

Right now doesn't change functionality but will be passed through the
stack.
Add logic to drop assist bases if asked to and add tests.
@ashmrtn ashmrtn added the backup label Aug 7, 2023
@ashmrtn ashmrtn self-assigned this Aug 7, 2023
@aviator-app
Copy link
Contributor

aviator-app bot commented Aug 7, 2023

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.


See the real-time status of this PR on the Aviator webapp.

@ashmrtn ashmrtn temporarily deployed to Testing August 7, 2023 22:53 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing August 7, 2023 22:53 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing August 7, 2023 22:53 — with GitHub Actions Inactive
src/pkg/control/options.go Outdated Show resolved Hide resolved
@ashmrtn ashmrtn temporarily deployed to Testing August 7, 2023 22:54 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing August 7, 2023 22:54 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing August 7, 2023 22:54 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing August 7, 2023 22:54 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing August 7, 2023 22:54 — with GitHub Actions Inactive
src/internal/operations/backup.go Outdated Show resolved Hide resolved
Comment on lines +321 to +324
// TODO(ashmrtn): This should probably just return a collection that deletes
// the entire subtree instead of returning an additional bool. That way base
// selection is controlled completely by flags and merging is controlled
// completely by collections.
Copy link
Contributor

Choose a reason for hiding this comment

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

could this cause issues if the backup only partially processes? ie: deletes the subtree, but doesn't complete the rest of the backup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it should be safe due to how we select bases and persist data. The "deletion" happens during hierarchy merging so it's really just excluding directories from the base when building the in-memory state

If corso crashes partway through the item data backup with the "deleted" state in-memory then we won't select that as a base with the new corso extension base selection logic (it won't have a details snapshot)

If corso hits some error persisting item data while making the snapshot but does still persist the details then the base will be eligible as an assist base on the next backup. That's safe though because the assist base contains all the details we'll need and the original merge base still has all the undeleted items

src/pkg/control/options.go Outdated Show resolved Hide resolved
src/internal/operations/manifests.go Outdated Show resolved Hide resolved
Mostly naming and nits.
Just swap value of input for dropping assist bases.
@ashmrtn ashmrtn temporarily deployed to Testing August 9, 2023 16:48 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing August 9, 2023 16:48 — with GitHub Actions Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 9, 2023 16:49 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 9, 2023 16:49 Inactive
@sonarcloud
Copy link

sonarcloud bot commented Aug 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@aviator-app aviator-app bot temporarily deployed to Testing August 9, 2023 16:49 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 9, 2023 16:50 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 9, 2023 16:50 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 9, 2023 16:50 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 9, 2023 16:50 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 9, 2023 16:50 Inactive
@aviator-app aviator-app bot merged commit ffa155a into main Aug 9, 2023
22 checks passed
@aviator-app aviator-app bot deleted the 2360-disable-assist-incremental branch August 9, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants