From 99a96a373575f91d59296459479adac4efbf3f71 Mon Sep 17 00:00:00 2001
From: Joao Afonso <joao.afonso@cern.ch>
Date: Thu, 19 Jan 2023 18:09:19 +0100
Subject: [PATCH] Resolve "Fix tape state change command idempotency when
resetting REPACKING/BROKEN/PENDING"
---
ReleaseNotes.md | 1 +
scheduler/Scheduler.cpp | 13 ++++---------
scheduler/SchedulerTest.cpp | 7 ++++++-
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/ReleaseNotes.md b/ReleaseNotes.md
index 1a929f7037..978fa7cb8c 100644
--- a/ReleaseNotes.md
+++ b/ReleaseNotes.md
@@ -16,6 +16,7 @@
- cta/CTA#189 - Avoid postgres logging frequent warnings about no transaction in progress
- cta/CTA#234 - Replace stoi with toUint64 in standalone cli tool
- cta/CTA#238 - Compilation fails when using cta::common::Configuration::getConfEntInt(...)
+- cta/CTA#273 - Fix tape state change command idempotency when resetting REPACKING/BROKEN/PENDING
### Continuous Integration
- cta/CTA#205 - Updating EOS4/EOS4 in versionlock for v4.8.95/v5.1.5
- cta/CTA#253 - Allow Failure for cta_valgrind tests
diff --git a/scheduler/Scheduler.cpp b/scheduler/Scheduler.cpp
index 7addc8f834..958255b396 100644
--- a/scheduler/Scheduler.cpp
+++ b/scheduler/Scheduler.cpp
@@ -1841,16 +1841,11 @@ void Scheduler::triggerTapeStateChange(const common::dataStructures::SecurityIde
// If previous and desired states are the same, do nothing
if (prev_state == new_state) return;
- // If previous state is already in transition (temporary state) to the desired state, do nothing
- if (prev_state == Tape::BROKEN_PENDING && new_state == Tape::BROKEN) return;
- if (prev_state == Tape::EXPORTED_PENDING && new_state == Tape::EXPORTED) return;
- if (prev_state == Tape::REPACKING_PENDING && new_state == Tape::REPACKING) return;
-
- // If previous state is temporary, user should wait for it to complete
+ // If previous state is PENDING (not of the same type), user should wait for it to complete
if (
- prev_state == Tape::BROKEN_PENDING
- || prev_state == Tape::EXPORTED_PENDING
- || prev_state == Tape::REPACKING_PENDING
+ (prev_state == Tape::BROKEN_PENDING && new_state != Tape::BROKEN)
+ || (prev_state == Tape::EXPORTED_PENDING && new_state != Tape::EXPORTED)
+ || (prev_state == Tape::REPACKING_PENDING && new_state != Tape::REPACKING)
) {
throw cta::exception::UserError("Cannot modify tape " + vid + " state while it is in a temporary internal state");
}
diff --git a/scheduler/SchedulerTest.cpp b/scheduler/SchedulerTest.cpp
index 9efdc742b9..a0d5e64ae6 100644
--- a/scheduler/SchedulerTest.cpp
+++ b/scheduler/SchedulerTest.cpp
@@ -6630,7 +6630,12 @@ INSTANTIATE_TEST_CASE_P(OStoreDBPlusMockSchedulerTestVFS, SchedulerTestTriggerTa
SchedulerTestParam(OStoreDBFactoryVFS, {Tape::REPACKING_PENDING, Tape::ACTIVE, Tape::REPACKING_PENDING, true, false}),
SchedulerTestParam(OStoreDBFactoryVFS, {Tape::BROKEN_PENDING, Tape::ACTIVE, Tape::BROKEN_PENDING, true, false}),
- SchedulerTestParam(OStoreDBFactoryVFS, {Tape::EXPORTED_PENDING, Tape::ACTIVE, Tape::EXPORTED_PENDING, true, false})
+ SchedulerTestParam(OStoreDBFactoryVFS, {Tape::EXPORTED_PENDING, Tape::ACTIVE, Tape::EXPORTED_PENDING, true, false}),
+
+ // The 'cleanup' flag should be reactivated when the same PENDING state is re-triggered
+ SchedulerTestParam(OStoreDBFactoryVFS, {Tape::REPACKING_PENDING, Tape::REPACKING, Tape::REPACKING_PENDING, false, true}),
+ SchedulerTestParam(OStoreDBFactoryVFS, {Tape::BROKEN_PENDING, Tape::BROKEN, Tape::BROKEN_PENDING, false, true}),
+ SchedulerTestParam(OStoreDBFactoryVFS, {Tape::EXPORTED_PENDING, Tape::EXPORTED, Tape::EXPORTED_PENDING, false, true})
));
#endif
--
GitLab