From 1609c76d62374cbcffd46ffd5afd4e19be72d934 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Botond=20D=C3=A9nes?= <bdenes@scylladb.com>
Date: Wed, 23 Aug 2023 07:51:58 -0400
Subject: [PATCH] tools/scylla-sstable: scrub: don't qurantine sstables after
 validate

Scylla sstable promises to *never* mutate its input sstables. This
promise was broken by `scylla sstable scrub --scrub-mode=validate`,
because validate moves invalid input sstables into qurantine. This is
unexpected and caused occasional failures in the scrub tests in
test_tools.py. Fix by propagating a flag down to
`scrub_sstables_validate_mode()` in `compaction.cc`, specifying whether
validate should qurantine invalid sstables, then set this flag to false
in `scylla-sstable.cc`. The existing test for validate-mode scrub is
ammended to check that the sstable is not mutated. The test now fails
before the fix and passes afterwards.

Fixes: #14309

Closes #15139
---
 compaction/compaction.cc            |  3 ++-
 compaction/compaction_descriptor.hh | 15 +++++++++++++--
 test/cql-pytest/test_tools.py       |  3 +++
 tools/scylla-sstable.cc             |  2 +-
 4 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/compaction/compaction.cc b/compaction/compaction.cc
index 86eb0b3598..57ce375e25 100644
--- a/compaction/compaction.cc
+++ b/compaction/compaction.cc
@@ -1733,7 +1733,8 @@ static future<compaction_result> scrub_sstables_validate_mode(sstables::compacti
         clogger.info("Finished scrubbing in validate mode {} - sstable is {}", sst->get_filename(), validation_errors == 0 ? "valid" : "invalid");
     }
 
-    if (validation_errors != 0) {
+    using scrub = sstables::compaction_type_options::scrub;
+    if (validation_errors != 0 && descriptor.options.as<scrub>().quarantine_sstables == scrub::quarantine_invalid_sstables::yes) {
         for (auto& sst : descriptor.sstables) {
             co_await sst->change_state(sstables::sstable_state::quarantine);
         }
diff --git a/compaction/compaction_descriptor.hh b/compaction/compaction_descriptor.hh
index 0e6fce2bec..6b81170ebe 100644
--- a/compaction/compaction_descriptor.hh
+++ b/compaction/compaction_descriptor.hh
@@ -72,6 +72,12 @@ public:
             only, // scrub only quarantined sstables
         };
         quarantine_mode quarantine_operation_mode = quarantine_mode::include;
+
+        using quarantine_invalid_sstables = bool_class<class quarantine_invalid_sstables_tag>;
+
+        // Should invalid sstables be moved into quarantine.
+        // Only applies to validate-mode.
+        quarantine_invalid_sstables quarantine_sstables = quarantine_invalid_sstables::yes;
     };
     struct reshard {
     };
@@ -108,8 +114,8 @@ public:
         return compaction_type_options(upgrade{});
     }
 
-    static compaction_type_options make_scrub(scrub::mode mode) {
-        return compaction_type_options(scrub{mode});
+    static compaction_type_options make_scrub(scrub::mode mode, scrub::quarantine_invalid_sstables quarantine_sstables = scrub::quarantine_invalid_sstables::yes) {
+        return compaction_type_options(scrub{.operation_mode = mode, .quarantine_sstables = quarantine_sstables});
     }
 
     template <typename... Visitor>
@@ -117,6 +123,11 @@ public:
         return std::visit(std::forward<Visitor>(visitor)..., _options);
     }
 
+    template <typename OptionType>
+    const auto& as() const {
+        return std::get<OptionType>(_options);
+    }
+
     const options_variant& options() const { return _options; }
 
     compaction_type type() const;
diff --git a/test/cql-pytest/test_tools.py b/test/cql-pytest/test_tools.py
index 57237b5024..faea7cbb98 100644
--- a/test/cql-pytest/test_tools.py
+++ b/test/cql-pytest/test_tools.py
@@ -832,3 +832,6 @@ def test_scrub_validate_mode(scylla_path, scrub_schema_file, scrub_good_sstable,
 
         subprocess.check_call([scylla_path, "sstable", "scrub", "--schema-file", scrub_schema_file, "--scrub-mode", "validate", "--output-dir", tmp_dir, scrub_bad_sstable])
         check_scrub_output_dir(tmp_dir, 0)
+
+        # Check that validate did not move the bad sstable into qurantine
+        assert os.path.exists(scrub_bad_sstable)
diff --git a/tools/scylla-sstable.cc b/tools/scylla-sstable.cc
index a56a5258af..5810b616c0 100644
--- a/tools/scylla-sstable.cc
+++ b/tools/scylla-sstable.cc
@@ -916,7 +916,7 @@ void scrub_operation(schema_ptr schema, reader_permit permit, const std::vector<
     scylla_sstable_table_state table_state(schema, permit, sst_man, output_dir);
 
     auto compaction_descriptor = sstables::compaction_descriptor(std::move(sstables));
-    compaction_descriptor.options = sstables::compaction_type_options::make_scrub(scrub_mode);
+    compaction_descriptor.options = sstables::compaction_type_options::make_scrub(scrub_mode, sstables::compaction_type_options::scrub::quarantine_invalid_sstables::no);
     compaction_descriptor.creator = [&table_state] (shard_id) { return table_state.make_sstable(); };
     compaction_descriptor.replacer = [] (sstables::compaction_completion_desc) { };