From d3f68ae4e84d67a23c12fcbfe6bf02336a6a1ffd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kiss=20K=C3=A1roly?= <kiss.karoly@rackhost.hu>
Date: Mon, 18 Jul 2022 10:09:50 +0200
Subject: [PATCH] replication: ignore bookmarks when computing incremental path

fixes https://github.com/zrepl/zrepl/issues/490
closes https://github.com/zrepl/zrepl/pull/619

Co-authored-by: Christian Schwarz <me@cschwarz.com>
---
 docs/changelog.rst                  |  2 ++
 replication/logic/diff/diff.go      | 19 +++++++++++++++++++
 replication/logic/diff/diff_test.go | 14 ++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/docs/changelog.rst b/docs/changelog.rst
index 189ec84..edbce5e 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -29,6 +29,8 @@ Developers should consult the git commit log or GitHub issue tracker.
     The Grafana dashboard in :repomasterlink:`dist/grafana` has been updated.
 
 * |bugfix| transient zrepl status error: ``Post "http://unix/status": EOF``
+* |bugfix| don't treat receive-side bookmarks as a replication conflict.
+  This facilitates chaining of replication jobs. See :issue:`490`.
 
 0.5
 ---
diff --git a/replication/logic/diff/diff.go b/replication/logic/diff/diff.go
index 477a334..42ef48c 100644
--- a/replication/logic/diff/diff.go
+++ b/replication/logic/diff/diff.go
@@ -88,8 +88,27 @@ func SortVersionListByCreateTXGThenBookmarkLTSnapshot(fsvslice []*FilesystemVers
 	return sorted
 }
 
+func StripBookmarksFromVersionList(fsvslice []*FilesystemVersion) []*FilesystemVersion {
+	fslice := make([]*FilesystemVersion, 0, len(fsvslice))
+	for _, fv := range fsvslice {
+		if fv.Type != FilesystemVersion_Bookmark {
+			fslice = append(fslice, fv)
+		}
+	}
+	return fslice
+}
+
 func IncrementalPath(receiver, sender []*FilesystemVersion) (incPath []*FilesystemVersion, conflict error) {
 
+	// Receive-side bookmarks can't be used as incremental-from,
+	// and don't cause recv to fail if there is a newer bookmark than incremetal-form on the receiver.
+	// So, simply mask them out.
+	// This will also hide them in the report, but it keeps the code in this function simple,
+	// and a user who complains about them missing in a conflict message will likely require
+	// more education about bookmarks than a slightly more accurate error message. They'll get
+	// that when they open an issue.
+	receiver = StripBookmarksFromVersionList(receiver)
+
 	receiver = SortVersionListByCreateTXGThenBookmarkLTSnapshot(receiver)
 	sender = SortVersionListByCreateTXGThenBookmarkLTSnapshot(sender)
 
diff --git a/replication/logic/diff/diff_test.go b/replication/logic/diff/diff_test.go
index d8ea0c5..d9b1511 100644
--- a/replication/logic/diff/diff_test.go
+++ b/replication/logic/diff/diff_test.go
@@ -152,4 +152,18 @@ func TestIncrementalPath_BookmarkSupport(t *testing.T) {
 		assert.Equal(t, l("@a,1", "@b,2"), path)
 	})
 
+	// test that receive-side bookmarks younger than the most recent common ancestor do not cause a conflict
+	doTest(l("@a,1", "#b,2"), l("@a,1", "@c,3"), func(path []*FilesystemVersion, conflict error) {
+		assert.NoError(t, conflict)
+		require.Len(t, path, 2)
+		assert.Equal(t, l("@a,1")[0], path[0])
+		assert.Equal(t, l("@c,3")[0], path[1])
+	})
+	doTest(l("#a,1"), l("@a,1", "@b,2"), func(path []*FilesystemVersion, conflict error) {
+		assert.Nil(t, path)
+		ca, ok := conflict.(*ConflictNoCommonAncestor)
+		require.True(t, ok)
+		assert.Equal(t, l(), ca.SortedReceiverVersions, "See comment in IncrementalPath() on why we don't include the boomkmark here")
+	})
+
 }