qmanager: add cleanup cancel upon receipt of final .free RPC

Problem: the final .free RPC does not free brokerless resources (e.g., rack-local ssds) with the current implementation of partial cancel. Add a full, cleanup cancel upon receipt of the final .free RPC. While exhibiting slighlty lower performance for a sequence of `.free` RPCs than supporting brokerless resource release in partial cancel, the full cancel is not subject to errors under various pruning filter configurations. Handling and preventing the edge-case errors will introduce significant complexity into the traverser and planner, and require updates to REAPI. We can revisit that implementation in the future if required by performance needs.
milroy · Oct 31, 2024 · 0501743 · 0501743
1 parent b5c7a00
commit 0501743
Showing 1 changed file with 27 additions and 21 deletions.
diff --git a/qmanager/policies/base/queue_policy_base.hpp b/qmanager/policies/base/queue_policy_base.hpp
@@ -646,14 +646,28 @@ class queue_policy_base_t : public resource_model::queue_adapter_base_t {
             case job_state_kind_t::ALLOC_RUNNING:
                 // deliberately fall through
             case job_state_kind_t::RUNNING:
-                if (cancel (h, job_it->second->id, R, true, full_removal) != 0) {
-                    flux_log_error (flux_h,
-                                    "%s: .free RPC partial cancel failed for jobid "
-                                    "%jd",
-                                    __FUNCTION__,
-                                    static_cast<intmax_t> (id));
-                    errno = EINVAL;
-                    goto out;
+                if (!final) {
+                    if (cancel (h, job_it->second->id, R, true, full_removal) != 0) {
+                        flux_log_error (flux_h,
+                                        "%s: .free RPC partial cancel failed for jobid "
+                                        "%jd",
+                                        __FUNCTION__,
+                                        static_cast<intmax_t> (id));
+                        errno = EINVAL;
+                        goto out;
+                    }
+                } else {
+                    // Run a full cancel to clean up all remaining allocated resources
+                    if (cancel (h, job_it->second->id, true) != 0) {
+                        flux_log_error (flux_h,
+                                        "%s: .free RPC full cancel failed for jobid "
+                                        "%jd",
+                                        __FUNCTION__,
+                                        static_cast<intmax_t> (id));
+                        errno = EPROTO;
+                        goto out;
+                    }
+                    full_removal = true;
                 }
                 // We still want to run the sched loop even if there's an inconsistent state
                 set_schedulability (true);
@@ -666,24 +680,16 @@ class queue_policy_base_t : public resource_model::queue_adapter_base_t {
                     // during cancel
                     auto job_sp = job_it->second;
                     m_jobs.erase (job_it);
-                    if (final && !full_removal) {
-                        // This error condition indicates a discrepancy between core and sched.
+                    if (full_removal && !final) {
+                        // This error condition can indicate a discrepancy between core and sched,
+                        // but commonly indicates partial cancel didn't clean up resources external
+                        // to a broker rank (e.g., ssds).
                         flux_log_error (flux_h,
-                                        "%s: Final .free RPC failed to remove all resources for "
+                                        "%s: removed all resources before final .free RPC for "
                                         "jobid "
                                         "%jd",
                                         __FUNCTION__,
                                         static_cast<intmax_t> (id));
-                        // Run a full cancel to clean up all remaining allocated resources
-                        if (cancel (h, job_sp->id, true) != 0) {
-                            flux_log_error (flux_h,
-                                            "%s: .free RPC full cancel failed for jobid "
-                                            "%jd",
-                                            __FUNCTION__,
-                                            static_cast<intmax_t> (id));
-                        }
-                        errno = EPROTO;
-                        goto out;
                     }
                 }
                 break;