From 383b93f097f4075d03ddf7284b58e19c9d743c05 Mon Sep 17 00:00:00 2001
From: ashmrtn <ashmrtnz@alcion.ai>
Date: Wed, 18 Jan 2023 13:41:58 -0800
Subject: [PATCH] Minor snapshot lookup refactor and logging addition (#2164)

## Description

Make some parts of the lookup logic more uniform as to when reasons are added to things.

Add logging to make the process easier to debug

## Does this PR need a docs update or release note?

- [ ] :white_check_mark: Yes, it's included
- [ ] :clock1: Yes, but in a later PR
- [x] :no_entry: No

## Type of change

- [ ] :sunflower: Feature
- [ ] :bug: Bugfix
- [ ] :world_map: Documentation
- [ ] :robot: Test
- [ ] :computer: CI/Deployment
- [x] :broom: Tech Debt/Cleanup

## Issue(s)

* #1675
* #2149

## Test Plan

- [x] :muscle: Manual
- [x] :zap: Unit test
- [ ] :green_heart: E2E
---
 src/internal/kopia/snapshot_manager.go | 54 ++++++++++++++------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/src/internal/kopia/snapshot_manager.go b/src/internal/kopia/snapshot_manager.go
index b3bd2a151..d4d47f37d 100644
--- a/src/internal/kopia/snapshot_manager.go
+++ b/src/internal/kopia/snapshot_manager.go
@@ -117,6 +117,7 @@ func getLastIdx(
 // adds it to the returned list. Otherwise no incomplete manifest is returned.
 // Returns nil if there are no complete or incomplete manifests in mans.
 func manifestsSinceLastComplete(
+	ctx context.Context,
 	mans []*snapshot.Manifest,
 ) ([]*snapshot.Manifest, bool) {
 	var (
@@ -134,9 +135,10 @@ func manifestsSinceLastComplete(
 
 		if len(m.IncompleteReason) > 0 {
 			if !foundIncomplete {
+				res = append(res, m)
 				foundIncomplete = true
 
-				res = append(res, m)
+				logger.Ctx(ctx).Infow("found incomplete snapshot", "snapshot_id", m.ID)
 			}
 
 			continue
@@ -147,6 +149,8 @@ func manifestsSinceLastComplete(
 		res = append(res, m)
 		foundComplete = true
 
+		logger.Ctx(ctx).Infow("found complete snapshot", "snapshot_id", m.ID)
+
 		break
 	}
 
@@ -164,7 +168,7 @@ func fetchPrevManifests(
 	foundMans map[manifest.ID]*ManifestEntry,
 	reason Reason,
 	tags map[string]string,
-) ([]*ManifestEntry, error) {
+) ([]*snapshot.Manifest, error) {
 	allTags := map[string]string{}
 
 	for _, k := range reason.TagKeys() {
@@ -188,8 +192,7 @@ func fetchPrevManifests(
 	// We have a complete cached snapshot and it's the most recent. No need
 	// to do anything else.
 	if lastCompleteIdx == len(metas)-1 {
-		man.Reasons = append(man.Reasons, reason)
-		return nil, nil
+		return []*snapshot.Manifest{man.Manifest}, nil
 	}
 
 	// TODO(ashmrtn): Remainder of the function can be simplified if we can inject
@@ -209,24 +212,21 @@ func fetchPrevManifests(
 		return nil, errors.Wrap(err, "fetching previous manifests")
 	}
 
-	found, hasCompleted := manifestsSinceLastComplete(mans)
-	res := make([]*ManifestEntry, 0, len(found))
-
-	for _, m := range found {
-		res = append(res, &ManifestEntry{
-			Manifest: m,
-			Reasons:  []Reason{reason},
-		})
-	}
+	found, hasCompleted := manifestsSinceLastComplete(ctx, mans)
 
 	// If we didn't find another complete manifest then we need to mark the
 	// previous complete manifest as having this ResourceOwner, Service, Category
 	// as the reason as well.
 	if !hasCompleted && man != nil {
-		man.Reasons = append(man.Reasons, reason)
+		found = append(found, man.Manifest)
+		logger.Ctx(ctx).Infow(
+			"reusing cached complete snapshot",
+			"snapshot_id",
+			man.ID,
+		)
 	}
 
-	return res, nil
+	return found, nil
 }
 
 // fetchPrevSnapshotManifests returns a set of manifests for complete and maybe
@@ -252,6 +252,14 @@ func fetchPrevSnapshotManifests(
 	// we can pass in. Can be expanded to return more than the most recent
 	// snapshots, but may require more memory at runtime.
 	for _, reason := range reasons {
+		logger.Ctx(ctx).Infow(
+			"searching for previous manifests for reason",
+			"service",
+			reason.Service.String(),
+			"category",
+			reason.Category.String(),
+		)
+
 		found, err := fetchPrevManifests(
 			ctx,
 			sm,
@@ -278,18 +286,16 @@ func fetchPrevSnapshotManifests(
 		for _, m := range found {
 			man := mans[m.ID]
 			if man == nil {
-				mans[m.ID] = m
+				mans[m.ID] = &ManifestEntry{
+					Manifest: m,
+					Reasons:  []Reason{reason},
+				}
+
 				continue
 			}
 
-			// If the manifest already exists and it's incomplete then we should
-			// merge the reasons for consistency. This will become easier to handle
-			// once we update how checkpoint manifests are tagged.
-			if len(man.IncompleteReason) == 0 {
-				continue
-			}
-
-			man.Reasons = append(man.Reasons, m.Reasons...)
+			// This manifest has multiple reasons for being chosen. Merge them here.
+			man.Reasons = append(man.Reasons, reason)
 		}
 	}