Skip to content
This repository has been archived by the owner on Oct 11, 2024. It is now read-only.

Produce map of items that need backup details entries #1892

Merged
merged 13 commits into from
Dec 22, 2022

Conversation

ashmrtn
Copy link
Contributor

@ashmrtn ashmrtn commented Dec 21, 2022

Description

Generate a map[old ShortRef]new path.Path for every item sourced from a base snapshot during backup. Return this information at the end of BackupCollections so that callers can use it to merge backup details if desired.

This PR does a few auxiliary things as well:

  • thread current and previous path through hierarchy merging
  • add extra field to info tracked by corsoProgress to hold previous path
  • provide current and previous paths to streamBaseItems function

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
  • 🤖 Test
  • 💻 CI/Deployment
  • 🐹 Trivial/Minor

Issue(s)

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

@ashmrtn ashmrtn added the enhancement New feature or request label Dec 21, 2022
@ashmrtn ashmrtn self-assigned this Dec 21, 2022
@ashmrtn ashmrtn temporarily deployed to Testing December 21, 2022 00:16 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 21, 2022 00:16 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 21, 2022 00:16 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 21, 2022 00:17 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 21, 2022 00:17 — with GitHub Actions Inactive
@ashmrtn ashmrtn changed the title 1800 collect base items Produce map of items that need backup details entries Dec 21, 2022
src/internal/kopia/upload.go Outdated Show resolved Hide resolved
@ashmrtn ashmrtn temporarily deployed to Testing December 21, 2022 16:46 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 21, 2022 16:46 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 21, 2022 16:46 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 21, 2022 16:46 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 21, 2022 16:47 — with GitHub Actions Inactive
@ashmrtn ashmrtn force-pushed the 1800-details-updater branch from 4551fcb to 682b7af Compare December 21, 2022 23:44
if d.info == nil {
// TODO(ashmrtn): We should probably be returning an error here?
if d.prevPath == nil {
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an error log here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't have access to a context here so unless I stash a logger in the struct instance there's not a whole lot we can do :/

Copy link
Contributor

Choose a reason for hiding this comment

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

The logger will fall back to a singleton instance if given the background context. You might miss out on some logging within tests, but for the CLI it'll still write out as expected.

Copy link
Contributor Author

@ashmrtn ashmrtn Dec 22, 2022

Choose a reason for hiding this comment

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

thinking about this more broadly, I think we should be storing errors in the struct and returning them at the end. Otherwise we could miss the fact that we failed to add some details. I'll make a follow up ticket for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tracking with #1915

Base automatically changed from 1800-details-updater to main December 22, 2022 00:09
This is mostly for streaming base directory entries since the path is
not immediately available for them. Will be used to produce metadata
required to merge backup details from base snapshots.
Add more metadata like the previous path to progress entries and track
entries for items in the base snapshot. This will allow adding details
entries for base snapshot items eventually.
@@ -183,6 +179,30 @@ func (cp *corsoProgress) FinishedFile(relativePath string, err error) {
}

cp.deets.AddFolders(folders)
Copy link
Contributor

Choose a reason for hiding this comment

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

Following up on the slack discussion - where we've changed this to AddFoldersForItem in main . I think we should just defer the AddFoldersForItem for an item that is being sourced from the base snapshot to when it is being "merged into details"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, that makes the most sense. A bit of a bummer cause I wanted to keep the code for building the set of directories limited to as few places as possible. Guess I'll move it to a helper function somewhere

Use a map[old shortref]new path.Path to track entries sourced from the
base snapshot. kopia.Wrapper can't directly add the ItemInfo for these
to the Details, but it can return the map to something that can.

Also always add entries for the folders of an item.
Return the map[old shortref]new path.Path fo items sourced from a base
snapshot. This will give BackupOp the information it needs to source
those ItemInfo entries from base snapshots.
@ashmrtn ashmrtn temporarily deployed to Testing December 22, 2022 21:34 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 22, 2022 21:34 — with GitHub Actions Inactive
@aviator-app
Copy link
Contributor

aviator-app bot commented Dec 22, 2022

Aviator status

Aviator will automatically update this comment as the status of the PR changes.

This PR was merged using Aviator.

@ashmrtn ashmrtn temporarily deployed to Testing December 22, 2022 21:34 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 22, 2022 21:35 — with GitHub Actions Inactive
@aviator-app aviator-app bot temporarily deployed to Testing December 22, 2022 21:57 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing December 22, 2022 21:57 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing December 22, 2022 21:57 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing December 22, 2022 21:58 Inactive
@aviator-app aviator-app bot added the blocked Upstream item prevents completion label Dec 22, 2022
@aviator-app
Copy link
Contributor

aviator-app bot commented Dec 22, 2022

PR failed to merge with reason: some CI status(es) failed.
Failed CI(s): Linting

@aviator-app aviator-app bot removed the blocked Upstream item prevents completion label Dec 22, 2022
@sonarqubecloud
Copy link

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 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ashmrtn ashmrtn temporarily deployed to Testing December 22, 2022 22:10 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 22, 2022 22:10 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 22, 2022 22:10 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 22, 2022 22:11 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 22, 2022 22:11 — with GitHub Actions Inactive
@ashmrtn ashmrtn temporarily deployed to Testing December 22, 2022 22:11 — with GitHub Actions Inactive
@aviator-app aviator-app bot merged commit 6b689b7 into main Dec 22, 2022
@aviator-app aviator-app bot deleted the 1800-collect-base-items branch December 22, 2022 22:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request mergequeue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants