From 3a46225107cbc559434a957252886b0f251a9e4d Mon Sep 17 00:00:00 2001 From: Sam Maloney <56830868+sam-maloney@users.noreply.github.com> Date: Fri, 14 Feb 2025 11:45:28 +0100 Subject: [PATCH] resource: enforce co-dependency of range keys Problem: recent change to RFC 14 explicitly makes max/operator/operand co-dependent; i.e., if one is given then all three must be, but this is not currently checked or enforced Add checks ensuring that if one of max/operator/operand is specified then all are, and adjusts the testsuite to match --- resource/libjobspec/jobspec.cpp | 30 ++++++++++++---- .../base/test/matcher_util_api_test01.cpp | 36 +------------------ .../resource_count_missing_max.yaml | 0 .../resource_count_missing_operand.yaml | 0 .../resource_count_missing_operator.yaml | 0 .../valid/resource_count_min_only.yaml | 15 ++++++++ 6 files changed, 40 insertions(+), 41 deletions(-) rename t/data/resource/jobspecs/validation/{valid => invalid}/resource_count_missing_max.yaml (100%) rename t/data/resource/jobspecs/validation/{valid => invalid}/resource_count_missing_operand.yaml (100%) rename t/data/resource/jobspecs/validation/{valid => invalid}/resource_count_missing_operator.yaml (100%) create mode 100644 t/data/resource/jobspecs/validation/valid/resource_count_min_only.yaml diff --git a/resource/libjobspec/jobspec.cpp b/resource/libjobspec/jobspec.cpp index 33daa1c50..98325ee98 100644 --- a/resource/libjobspec/jobspec.cpp +++ b/resource/libjobspec/jobspec.cpp @@ -66,14 +66,32 @@ void parse_yaml_count (Resource &res, const YAML::Node &cnode) if (!cnode["min"].IsScalar ()) { throw parse_error (cnode["min"], "Value of \"min\" must be a scalar"); } - if (cnode["max"] && !cnode["max"].IsScalar ()) { - throw parse_error (cnode["max"], "Value of \"max\" must be a scalar"); + if (cnode["max"]) { + if (!cnode["max"].IsScalar ()) { + throw parse_error (cnode["max"], "Value of \"max\" must be a scalar"); + } + if (!cnode["operator"] || !cnode["operand"]) { + throw parse_error (cnode, + "\"operator\" and \"operand\" required when \"max\" given"); + } } - if (cnode["operator"] && !cnode["operator"].IsScalar ()) { - throw parse_error (cnode["operator"], "Value of \"operator\" must be a scalar"); + if (cnode["operator"]) { + if (!cnode["operator"].IsScalar ()) { + throw parse_error (cnode["operator"], "Value of \"operator\" must be a scalar"); + } + if (!cnode["max"] || !cnode["operand"]) { + throw parse_error (cnode, + "\"max\" and \"operand\" required when \"operator\" given"); + } } - if (cnode["operand"] && !cnode["operand"].IsScalar ()) { - throw parse_error (cnode["operand"], "Value of \"operand\" must be a scalar"); + if (cnode["operand"]) { + if (!cnode["operand"].IsScalar ()) { + throw parse_error (cnode["operand"], "Value of \"operand\" must be a scalar"); + } + if (!cnode["max"] || !cnode["operator"]) { + throw parse_error (cnode, + "\"max\" and \"operator\" required when \"operand\" given"); + } } /* Validate values of entries */ diff --git a/resource/policies/base/test/matcher_util_api_test01.cpp b/resource/policies/base/test/matcher_util_api_test01.cpp index 789e070d6..c4980c115 100644 --- a/resource/policies/base/test/matcher_util_api_test01.cpp +++ b/resource/policies/base/test/matcher_util_api_test01.cpp @@ -31,8 +31,6 @@ using namespace Flux::resource_model; class test_jobspec_template { public: explicit test_jobspec_template (unsigned int min); - explicit test_jobspec_template (unsigned int min, unsigned int max); - explicit test_jobspec_template (unsigned int min, unsigned int max, std::string oper); explicit test_jobspec_template (unsigned int min, unsigned int max, std::string oper, @@ -42,8 +40,6 @@ class test_jobspec_template { private: int emit (json_t *count_obj); json_t *emit_count (unsigned int min); - json_t *emit_count (unsigned int min, unsigned int max); - json_t *emit_count (unsigned int min, unsigned int max, const std::string &oper); json_t *emit_count (unsigned int min, unsigned int max, const std::string &oper, @@ -114,18 +110,6 @@ json_t *test_jobspec_template::emit_count (unsigned int min) return json_pack ("{s:i}", "min", min); } -json_t *test_jobspec_template::emit_count (unsigned int min, unsigned int max) -{ - return json_pack ("{s:i s:i}", "min", min, "max", max); -} - -json_t *test_jobspec_template::emit_count (unsigned int min, - unsigned int max, - const std::string &oper) -{ - return json_pack ("{s:i s:i s:s}", "min", min, "max", max, "operator", oper.c_str ()); -} - json_t *test_jobspec_template::emit_count (unsigned int min, unsigned int max, const std::string &oper, @@ -155,15 +139,6 @@ test_jobspec_template::test_jobspec_template (unsigned int min) throw std::bad_alloc (); } -test_jobspec_template::test_jobspec_template (unsigned int min, unsigned int max) -{ - json_t *count_obj{nullptr}; - if ((count_obj = emit_count (min, max)) == nullptr) - throw std::bad_alloc (); - if ((emit (count_obj)) < 0) - throw std::bad_alloc (); -} - test_jobspec_template::test_jobspec_template (unsigned int min, unsigned int max, std::string oper, @@ -176,15 +151,6 @@ test_jobspec_template::test_jobspec_template (unsigned int min, throw std::bad_alloc (); } -test_jobspec_template::test_jobspec_template (unsigned int min, unsigned int max, std::string oper) -{ - json_t *count_obj{nullptr}; - if ((count_obj = emit_count (min, max, oper)) == nullptr) - throw std::bad_alloc (); - if ((emit (count_obj)) < 0) - throw std::bad_alloc (); -} - const std::string &test_jobspec_template::get () const { return m_job_spec_string; @@ -200,7 +166,7 @@ void test_plus_op () matcher_util_api_t api; // {min=1, max=10, operator='+', operand=1} - test_jobspec_template jobin_001 (1, 10); + test_jobspec_template jobin_001 (1, 10, "+", 1); Flux::Jobspec::Jobspec job_001{jobin_001.get ()}; count = api.calc_count (job_001.resources[0], std::numeric_limits::max ()); ok (count == 10, "{min=1, max=10, oper='+', op=1} is 10"); diff --git a/t/data/resource/jobspecs/validation/valid/resource_count_missing_max.yaml b/t/data/resource/jobspecs/validation/invalid/resource_count_missing_max.yaml similarity index 100% rename from t/data/resource/jobspecs/validation/valid/resource_count_missing_max.yaml rename to t/data/resource/jobspecs/validation/invalid/resource_count_missing_max.yaml diff --git a/t/data/resource/jobspecs/validation/valid/resource_count_missing_operand.yaml b/t/data/resource/jobspecs/validation/invalid/resource_count_missing_operand.yaml similarity index 100% rename from t/data/resource/jobspecs/validation/valid/resource_count_missing_operand.yaml rename to t/data/resource/jobspecs/validation/invalid/resource_count_missing_operand.yaml diff --git a/t/data/resource/jobspecs/validation/valid/resource_count_missing_operator.yaml b/t/data/resource/jobspecs/validation/invalid/resource_count_missing_operator.yaml similarity index 100% rename from t/data/resource/jobspecs/validation/valid/resource_count_missing_operator.yaml rename to t/data/resource/jobspecs/validation/invalid/resource_count_missing_operator.yaml diff --git a/t/data/resource/jobspecs/validation/valid/resource_count_min_only.yaml b/t/data/resource/jobspecs/validation/valid/resource_count_min_only.yaml new file mode 100644 index 000000000..56ac49bf7 --- /dev/null +++ b/t/data/resource/jobspecs/validation/valid/resource_count_min_only.yaml @@ -0,0 +1,15 @@ +version: 9999 +resources: + - type: slot + count: + min: 1 + label: foo + with: + - type: node + count: 1 +tasks: + - command: [ "app" ] + slot: foo + count: + per_slot: 1 +attributes: