diff --git a/libpromises/attributes.c b/libpromises/attributes.c index b9fe8674ba..6a4408bf65 100644 --- a/libpromises/attributes.c +++ b/libpromises/attributes.c @@ -1132,6 +1132,21 @@ ContextConstraint GetContextConstraints(const EvalContext *ctx, const Promise *p a.expression = NULL; a.persistent = PromiseGetConstraintAsInt(ctx, "persistence", pp); + { + const char *tp = PromiseGetConstraintAsRval(pp, "timer_policy", RVAL_TYPE_SCALAR); + if (StringEqual(tp, "reset")) + { + a.timer = CONTEXT_STATE_POLICY_RESET; + } + else + { + /* Default to PRESERVE (absolute) for backward compatibility: + * classes: promises historically skip re-evaluation when the + * class is already defined, so the timer was never reset. */ + a.timer = CONTEXT_STATE_POLICY_PRESERVE; + } + } + { const char *context_scope = PromiseGetConstraintAsRval(pp, "scope", RVAL_TYPE_SCALAR); a.scope = ContextScopeFromString(context_scope); @@ -1143,7 +1158,9 @@ ContextConstraint GetContextConstraints(const EvalContext *ctx, const Promise *p for (int k = 0; CF_CLASSBODY[k].lval != NULL; k++) { - if (strcmp(cp->lval, "persistence") == 0 || strcmp(cp->lval, "scope") == 0) + if (StringEqual(cp->lval, "persistence") || + StringEqual(cp->lval, "scope") || + StringEqual(cp->lval, "timer_policy")) { continue; } diff --git a/libpromises/cf3.defs.h b/libpromises/cf3.defs.h index 71ca9f7f3b..7a25a2452a 100644 --- a/libpromises/cf3.defs.h +++ b/libpromises/cf3.defs.h @@ -1210,6 +1210,7 @@ typedef struct ContextScope scope; int nconstraints; int persistent; + PersistentClassPolicy timer; } ContextConstraint; /*************************************************************************/ diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c index 76c771388a..1c82b1dcb2 100644 --- a/libpromises/eval_context.c +++ b/libpromises/eval_context.c @@ -735,7 +735,7 @@ void EvalContextHeapPersistentSave(EvalContext *ctx, const char *name, unsigned // first see if we have an existing record, and if we should bother to update { - int existing_info_size = ValueSizeDB(dbp, key, strlen(key)); + int existing_info_size = ValueSizeDB(dbp, key, strlen(key) + 1); if (existing_info_size > 0) { PersistentClassInfo *existing_info = xcalloc(existing_info_size, 1); diff --git a/libpromises/mod_common.c b/libpromises/mod_common.c index 3d41cd4270..105e47fa86 100644 --- a/libpromises/mod_common.c +++ b/libpromises/mod_common.c @@ -226,6 +226,7 @@ const ConstraintSyntax CF_CLASSBODY[] = ConstraintSyntaxNewContext("not", "Evaluate the negation of string expression in normal form", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewContextList("select_class", "Select one of the named list of classes to define based on host identity. Default value: random_selection", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewContextList("xor", "Combine class sources with XOR", SYNTAX_STATUS_NORMAL), + ConstraintSyntaxNewOption("timer_policy", "absolute,reset", "Whether a persistent class restarts its counter when rediscovered. Default value: absolute", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewNull() }; diff --git a/libpromises/promises.c b/libpromises/promises.c index 4c917ef581..9e955f5496 100644 --- a/libpromises/promises.c +++ b/libpromises/promises.c @@ -705,16 +705,31 @@ Promise *ExpandDeRefPromise(EvalContext *ctx, const Promise *pp, bool *excluded) pcopy->org_pp = pp->org_pp; // if this is a class promise, check if it is already set, if so, skip + // Exception: persistent classes with timer_policy => "reset" must not + // be skipped — the promise needs to fire so the timer gets reset. if (strcmp("classes", PromiseGetPromiseType(pp)) == 0) { if (IsDefinedClass(ctx, CanonifyName(pcopy->promiser))) { - Log(LOG_LEVEL_DEBUG, - "Skipping evaluation of classes promise as class '%s' is already set", - CanonifyName(pcopy->promiser)); + const char *tp = PromiseGetConstraintAsRval(pp, "timer_policy", RVAL_TYPE_SCALAR); + int persistence = PromiseGetConstraintAsInt(ctx, "persistence", pp); - *excluded = true; - return pcopy; + if (StringEqual(tp, "reset") && persistence > 0) + { + Log(LOG_LEVEL_DEBUG, + "Class '%s' is already set but timer_policy is reset" + " — allowing promise evaluation to reset persistence timer", + CanonifyName(pcopy->promiser)); + } + else + { + Log(LOG_LEVEL_DEBUG, + "Skipping evaluation of classes promise as class '%s' is already set", + CanonifyName(pcopy->promiser)); + + *excluded = true; + return pcopy; + } } } diff --git a/libpromises/verify_classes.c b/libpromises/verify_classes.c index 7f903e2eb6..129ed61774 100644 --- a/libpromises/verify_classes.c +++ b/libpromises/verify_classes.c @@ -73,8 +73,37 @@ PromiseResult VerifyClassPromise(EvalContext *ctx, const Promise *pp, ARG_UNUSED return PROMISE_RESULT_FAIL; } - if (a.context.expression == NULL || - EvalClassExpression(ctx, a.context.expression, pp)) + bool class_expression_true = + (a.context.expression == NULL || + EvalClassExpression(ctx, a.context.expression, pp)); + + /* When a persistent class is already defined (loaded from DB), + * EvalClassExpression short-circuits and returns false. If the + * timer_policy is "reset", we still need to reset the persistence + * timer in the DB even though the class is already in the context. */ + if (!class_expression_true && + a.context.persistent > 0 && + a.context.timer == CONTEXT_STATE_POLICY_RESET && + IsDefinedClass(ctx, pp->promiser)) + { + StringSet *tags = StringSetNew(); + StringSetAdd(tags, xstrdup("source=promise")); + for (const Rlist *rp = PromiseGetConstraintAsList(ctx, "meta", pp); rp; rp = rp->next) + { + StringSetAdd(tags, xstrdup(RlistScalarValue(rp))); + } + Log(LOG_LEVEL_VERBOSE, + "C: + Resetting persistent class timer: '%s' (%d minutes)", + pp->promiser, a.context.persistent); + Buffer *buf = StringSetToBuffer(tags, ','); + EvalContextHeapPersistentSave(ctx, pp->promiser, a.context.persistent, + CONTEXT_STATE_POLICY_RESET, BufferData(buf)); + BufferDestroy(buf); + StringSetDestroy(tags); + return PROMISE_RESULT_NOOP; + } + + if (class_expression_true) { if (a.context.expression == NULL) { @@ -131,7 +160,7 @@ PromiseResult VerifyClassPromise(EvalContext *ctx, const Promise *pp, ARG_UNUSED pp->promiser, a.context.persistent); Buffer *buf = StringSetToBuffer(tags, ','); EvalContextHeapPersistentSave(ctx, pp->promiser, a.context.persistent, - CONTEXT_STATE_POLICY_RESET, BufferData(buf)); + a.context.timer, BufferData(buf)); BufferDestroy(buf); } if (inserted && (comment != NULL)) diff --git a/tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf b/tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf new file mode 100644 index 0000000000..30997d2a92 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf @@ -0,0 +1,92 @@ +####################################################### +# +# CFE-4681: classes: promises with timer_policy => "absolute" (default) +# +# Verify that timer_policy => "absolute" (the default) on a classes: +# promise preserves the persistence timer across agent runs. +# +# This is the counterpart to persistent_timer_policy_reset.cf and +# directly demonstrates the "promise is not evaluated" behaviour: +# once the persistent class is loaded from the DB on a subsequent +# run, the classes: promise is skipped in ExpandDeRefPromise (it is +# never handed to VerifyClassPromise), so the timer is left untouched. +# +# First run: expect "Creating persistent class ... policy preserve" +# Second run: expect "Skipping evaluation of classes promise ... is +# already set" and NO "Resetting persistent class". +# +# Note: the skip message is logged at LOG_LEVEL_DEBUG, so the second +# run uses -d. Both sub-agent runs happen in the test bundle so that +# the check bundle only evaluates assertions -- this avoids a spurious +# FAIL report during the agent's convergence passes. +# +####################################################### + +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +bundle agent init +{ + # Remove the persistent class DB to ensure a clean state. + files: + "$(sys.workdir)/state/cf_state.lmdb" + delete => tidy; + "$(sys.workdir)/state/cf_state.lmdb-lock" + delete => tidy; + "$(sys.workdir)/state/cf_state.lmdb.lock" + delete => tidy; +} + +bundle agent test +{ + meta: + "description" -> { "CFE-4681" } + string => "timer_policy => absolute (default) on classes: promises preserves the timer; the promise is skipped (not evaluated) when the class is already defined"; + + commands: + # First run: define the persistent class with timer_policy => "absolute". + !first_done:: + "$(sys.cf_agent) -Kv -f $(this.promise_filename).sub > $(G.testdir)/timer_policy_run1.log 2>&1" + contain => in_shell, + classes => always("first_done"); + + # Second run: the class is already loaded from the persistent DB, so with + # timer_policy => absolute the classes: promise must be skipped (not + # evaluated) and the timer must not be reset. Run with -d so the + # DEBUG-level skip message is captured. + first_done.!second_done:: + "$(sys.cf_agent) -Kd -f $(this.promise_filename).sub > $(G.testdir)/timer_policy_run2.log 2>&1" + contain => in_shell, + classes => always("second_done"); +} + +bundle agent check +{ + classes: + # Run 1 creates the persistent class with the preserve (absolute) policy. + "create_ok" expression => regline(".*Creating persistent class.*timer_policy_test_class.*policy preserve.*", + "$(G.testdir)/timer_policy_run1.log"); + # Run 2 must skip the promise because the class is already defined. + "skip_ok" expression => regline(".*Skipping evaluation of classes promise as class.*timer_policy_test_class.*is already set.*", + "$(G.testdir)/timer_policy_run2.log"); + # Run 2 must NOT reset the timer (no EvalContextHeapPersistentSave call). + "reset_seen" expression => regline(".*Resetting persistent class.*timer_policy_test_class.*", + "$(G.testdir)/timer_policy_run2.log"); + "ok" expression => "create_ok.skip_ok.!reset_seen"; + + reports: + DEBUG.!create_ok:: + "FAIL: first run did not log 'Creating persistent class ... policy preserve'"; + DEBUG.!skip_ok:: + "FAIL: second run did not skip the classes promise (expected 'Skipping evaluation of classes promise ... is already set')"; + DEBUG.reset_seen:: + "FAIL: second run reset the timer despite timer_policy => absolute"; + ok:: + "$(this.promise_filename) Pass"; + !ok:: + "$(this.promise_filename) FAIL"; +} diff --git a/tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf.sub b/tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf.sub new file mode 100644 index 0000000000..ca6dbb9c87 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf.sub @@ -0,0 +1,15 @@ +body common control +{ + bundlesequence => { run }; +} + +bundle agent run +{ + classes: + # Define persistent class with timer_policy => "absolute" + # This stores CONTEXT_STATE_POLICY_PRESERVE in the DB + "timer_policy_test_class" + expression => "any", + persistence => "120", + timer_policy => "absolute"; +} diff --git a/tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf b/tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf new file mode 100644 index 0000000000..925fb38c9e --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf @@ -0,0 +1,83 @@ +####################################################### +# +# CFE-4681: classes: promises with timer_policy => "reset" +# +# Verify that timer_policy => "reset" on a classes: promise +# causes the persistence timer to be reset on subsequent +# agent runs, even though the class is already defined +# (loaded from the persistent DB). +# +# First run: expect "Creating persistent class" +# Second run: expect "Resetting persistent class" (not skipped) +# +# Both sub-agent runs happen in the test bundle so that the check +# bundle only evaluates assertions -- this avoids a spurious FAIL +# report during the agent's convergence passes. +# +####################################################### + +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +bundle agent init +{ + # Remove the persistent class DB to ensure a clean state. + files: + "$(sys.workdir)/state/cf_state.lmdb" + delete => tidy; + "$(sys.workdir)/state/cf_state.lmdb-lock" + delete => tidy; + "$(sys.workdir)/state/cf_state.lmdb.lock" + delete => tidy; +} + +bundle agent test +{ + meta: + "description" -> { "CFE-4681" } + string => "timer_policy => reset on classes: promises resets the persistence timer on subsequent runs"; + + commands: + # First run: define the persistent class. + !first_done:: + "$(sys.cf_agent) -Kv -f $(this.promise_filename).sub > $(G.testdir)/timer_reset_run1.log 2>&1" + contain => in_shell, + classes => always("first_done"); + + # Second run: the class already exists in the DB; timer_policy => reset + # must cause the promise to be evaluated and the timer to be reset. + first_done.!second_done:: + "$(sys.cf_agent) -Kv -f $(this.promise_filename).sub > $(G.testdir)/timer_reset_run2.log 2>&1" + contain => in_shell, + classes => always("second_done"); +} + +bundle agent check +{ + classes: + "first_ok" expression => regline(".*Creating persistent class.*timer_reset_test_class.*", + "$(G.testdir)/timer_reset_run1.log"); + # Match the EvalContextHeapPersistentSave message specifically (it + # reports "... timer to N minutes (was M remaining)"). This only + # appears when the existing DB record is actually found, so it would + # NOT match the "C: + Resetting persistent class timer: ..." progress + # line that VerifyClassPromise logs regardless. This distinction is + # what makes the test catch a broken existing-record lookup. + "second_ok" expression => regline(".*Resetting persistent class 'timer_reset_test_class' timer to.*", + "$(G.testdir)/timer_reset_run2.log"); + "ok" expression => "first_ok.second_ok"; + + reports: + DEBUG.!first_ok:: + "FAIL: first run did not log 'Creating persistent class'"; + DEBUG.!second_ok:: + "FAIL: second run did not log 'Resetting persistent class' (short-circuit not bypassed)"; + ok:: + "$(this.promise_filename) Pass"; + !ok:: + "$(this.promise_filename) FAIL"; +} diff --git a/tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf.sub b/tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf.sub new file mode 100644 index 0000000000..a5624056a7 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf.sub @@ -0,0 +1,16 @@ +body common control +{ + bundlesequence => { run }; +} + +bundle agent run +{ + classes: + # Define persistent class with timer_policy => "reset" + # On second run, the timer should be reset even though + # the class is already defined from the persistent DB. + "timer_reset_test_class" + expression => "any", + persistence => "120", + timer_policy => "reset"; +} diff --git a/tests/unit/eval_context_test.c b/tests/unit/eval_context_test.c index e47b6e1a4b..77234beda8 100644 --- a/tests/unit/eval_context_test.c +++ b/tests/unit/eval_context_test.c @@ -152,6 +152,51 @@ void test_eval_with_token_from_list(void) StringSetDestroy(time_classes); } +static void test_persistent_class_timer_policy(void) +{ + EvalContext *ctx = EvalContextNew(); + + /* Save a persistent class with PRESERVE policy, 60 minute TTL */ + EvalContextHeapPersistentSave(ctx, "timer_test", 60, + CONTEXT_STATE_POLICY_PRESERVE, "tag1"); + + /* Verify the class loads correctly after PRESERVE save */ + EvalContextHeapPersistentLoadAll(ctx); + + { + const Class *cls = EvalContextClassGet(ctx, "default", "timer_test"); + assert_true(cls != NULL); + assert_string_equal("timer_test", cls->name); + } + + /* Save again with PRESERVE -- the function should early-return + * (class is preserved, not expired, same tags), leaving the DB + * record unchanged. We verify by loading persistent classes and + * checking the class is still defined. */ + EvalContextHeapPersistentSave(ctx, "timer_test", 60, + CONTEXT_STATE_POLICY_PRESERVE, "tag1"); + + /* Class should still be defined after the second PRESERVE save */ + { + const Class *cls = EvalContextClassGet(ctx, "default", "timer_test"); + assert_true(cls != NULL); + assert_string_equal("timer_test", cls->name); + } + + /* Save with RESET policy -- the record SHOULD be overwritten. + * The class should still be loadable afterward. */ + EvalContextHeapPersistentSave(ctx, "timer_test", 60, + CONTEXT_STATE_POLICY_RESET, "tag1"); + + { + const Class *cls = EvalContextClassGet(ctx, "default", "timer_test"); + assert_true(cls != NULL); + assert_string_equal("timer_test", cls->name); + } + + EvalContextDestroy(ctx); +} + int main() { PRINT_TEST_BANNER(); @@ -160,6 +205,7 @@ int main() const UnitTest tests[] = { unit_test(test_class_persistence), + unit_test(test_persistent_class_timer_policy), unit_test(test_changes_chroot), unit_test(test_eval_with_token_from_list), };