Rudder

Actually, the problem is linked to the Directives.
If we update the Meta-technique, but don't save the Directives , then it keeps its older values, so everything is invalid

We should, when a metatechnique is updated, update automagically all directive dependent upon it

One of the inconvenience seems to be that until the save of the Directive, there is no check of CFEngine syntax error.

Nicolas CHARLES wrote:

I searched on this issue.
metadata.xml changes are not detected during an update of the Technique Librairy

Well, I need more information about that because that:

"[2013-12-27 11:17:22] DEBUG com.normation.cfclerk.services.impl.TechniqueRepositoryImpl - Not reloading technique library as nothing changed since last reload"
so we don't know we have to reload/change something.

is not true. A Technique is considered modified if anything under it's "techniqueId/version" directory changed (file created, deleted, or modified). So the "expected_report.csv" should be like other files here (CFEngine template, metadata.xml, etc).

We need to make a large modification:
- detect when metadata/expected_report.csv are modified

The "metadata" part is a typo, isn't it? The file we are looking at for expected reports is "techniqueId/version/expected_report.csv"

- if they are modified, update all directive depending on them

OK, we can do that. We want to save directive, or just do a subset of things related to a save?

- regenerate.

That should be already done.

this is a chenge too big for me to do this afternoon, Francois, I'm assigning it to you

Checked with NPE that a modified expected_reports.csv commited into git leads to a library reload.

So I'm going to implement the directive update for them.

OK, thinking about that... I can't save directives: nothing changed for them, so the Directive Repo will just yell at me.

And as the acceptation datetime for that technique his more recent that the last generation, things related to Technique should be copied back...

Thinking (again) about that, actually the directives may have changed, as the metadata.xml may have change, and new variables may be availables...

And thinking (twice) about that, the "metadata/expected_report.csv" wasn't a typo, it was meant to be metada.xml or expected_report.csv".

Now, everything make sense, and I should just add a call back to save directives for modified techniques.

I'm going to only correct it for 2.9 for now, but I'm wondering if it should not be considered as a bug for previous release to. The ticket #4302 is here to track that.

François ARMAND wrote:

Vincent, the technical review is easy but please, try to thing to what cool go wrong when we just save directives on a Technique update. I'm pretty sure I forgot some hard to deal with cases.

• The comments in code say this happens when a Technique is changed without a version increment, for example when adding an optional variable. What happens if a mandatory variable is added to a Technique without incrementing it's version and this code gets run?
• similarly, what if a Technique is made unique when it wasn't previously?
• What about change requests? Might this Directive save trigger a new change request or invalidate other pending change requests on those directives?
• What about event logs? Are any created for the directive saves?

Jonathan CLARKE wrote:

François ARMAND wrote:

Vincent, the technical review is easy but please, try to thing to what cool go wrong when we just save directives on a Technique update. I'm pretty sure I forgot some hard to deal with cases.

I've given this some thought and here are some things to consider :

• The comments in code say this happens when a Technique is changed without a version increment, for example when adding an optional variable. What happens if a mandatory variable is added to a Technique without incrementing it's version and this code gets run?

This is the main problem here, The Directive will be saved (checks about Directive validity should be made before saving), and but the deployment may fail right after (the Variable will not be replaced, leading to a not valid cfengine file)

• similarly, what if a Technique is made unique when it wasn't previously?

Only one Directive (with the highest priority) will be deployed on each node if multiple Directives were based on that Technique

• What about change requests? Might this Directive save trigger a new change request or invalidate other pending change requests on those directives?

This will not trigger new change request (it does not go through the workflow here)
It may invalidate change request pending on those Directives

• What about event logs? Are any created for the directive saves?

There will be one event log per Directive saved (save trigger event log)

Jonathan CLARKE wrote:

François ARMAND wrote:

Vincent, the technical review is easy but please, try to thing to what cool go wrong when we just save directives on a Technique update. I'm pretty sure I forgot some hard to deal with cases.

I've given this some thought and here are some things to consider :

• The comments in code say this happens when a Technique is changed without a version increment, for example when adding an optional variable. What happens if a mandatory variable is added to a Technique without incrementing it's version and this code gets run?

If the modification is unconsistent with previous state, the directives will be broken and the promise regeneration will fail. Else, default value will be used.
For example, tested with a Rule having an "apt package" directive. I added a new mandatory variable, with no default value.
The directive was saved, and the following promise regeneration failed like that:

In logs:

[2013-12-31 10:47:46] ERROR com.normation.rudder.services.policies.RudderCf3PromisesFileWriterServiceImpl - Writing promises error in fileSet Set(Tml package id aptPackageInstallation/1.2, Tml name aptPackageInstallation, Tml destination aptPackageInstallation/1.2/aptPackageInstallation.cf)
com.normation.cfclerk.exceptions.VariableException: Mandatory variable TEST_MANDATORY is empty, can not write aptPackageInstallation/1.2/aptPackageInstallation.cf

In UI:

Deployment process was stopped due to an error:
⇨ Deployment error for process '80' at 2013/12/31 10:47:46
⇨ Exception caught during deployment process: Mandatory variable TEST_MANDATORY is empty, can not write aptPackageInstallation/1.2/aptPackageInstallation.cf

(oh, side note: can we stop calling that a "deployment", and change it "node configuration generation" or something like that?)

• similarly, what if a Technique is made unique when it wasn't previously?

Same rules applied as for other unique directives (i.e pick the most prioritary, or one at random).

• What about change requests? Might this Directive save trigger a new change request or invalidate other pending change requests on those directives?

No change request is generated for theses changed. One could be created, but it was not what was done for other technique library related things until now - ok, it's a directive change, but done to match a technique library change.
I believe that our current handling of technique change is broken, because we don't correlate it them with other part of the configuration. We can't do much better as long as we don't decorrelate the git repository of techniques and the Rudder internal representation of what is a Technique. But I disgress.
Given the current state of affair, the two options of handling a technique metadata modification repercussion in Directives, regarding workflows, are:

- don't use wf for the directive save, i.e consider that it's a "system" directive update (without it, the promise generated for the directive won't be correct)
- use wf: that is a normal modification, it's just that until it's validated, new generation won't be possible.

In either case, other wf related to directive based on that technique won't be validable anymore, as the requirement for "validability" won't be met.

I tend to prefer the first one on some aspect (the second can lead to loads of wf change request to validate), but the second one is the only one leading to a change request for metatechnique modification (with the possibility to not be able to deploy anything until change request related to directive updated due to technique change are validated).

• What about event logs? Are any created for the directive saves?

Event logs should be generated with a system user, but my last tests seems to not agree with that theorical behaviour. Of course, the theorical behavior is right, and so I'm looking why the observed one doesn't agree ;)

Pull request updated. Now, eventlogs are correcly generated when the directive changes and the directive is correctly saved with updated values.

Applied in changeset d468a45a9353943fe3a4d52b26d603f37390f465.

François ARMAND wrote:

• The comments in code say this happens when a Technique is changed without a version increment, for example when adding an optional variable. What happens if a mandatory variable is added to a Technique without incrementing it's version and this code gets run?

If the modification is unconsistent with previous state, the directives will be broken and the promise regeneration will fail. Else, default value will be used.

I'm not happy at all about using a default value in a variable without at least a WARNING to the user. It seems that modifying a Technique without incrementing it's version number is a dodgy case anyway, so I won't worry too much for now. Although I do see the need for this (for example, while creating a Technique incrementally and testing it), I definitely think this should come up with a clear warning in the UI and in logs along the lines of "WARNING: Technique name/version has been modified without incrementing it's version number!" and if any Directives get mandatory variables added with default values we should see a clear, precise list of them, something like "The following Directives have been automatically modified to add the new mandatory variable "Variable name" with default value "def val": Dir A, Dir B, etc".

(oh, side note: can we stop calling that a "deployment", and change it "node configuration generation" or something like that?)

Oh, god, yes please! It's about time we renamed all this nonsense about deployments and "tml" in this part of the code. These terms make no sense, please destroy them with violence ASAP!

• What about change requests? Might this Directive save trigger a new change request or invalidate other pending change requests on those directives?

No change request is generated for theses changed.

OK, I think that's fine, as reloading the Technique library is an admin's job. It basically requires superpowers, which is not really compatible with going through workflows.

• What about event logs? Are any created for the directive saves?

Event logs should be generated with a system user, but my last tests seems to not agree with that theorical behaviour. Of course, the theorical behavior is right, and so I'm looking why the observed one doesn't agree ;)

I'm not sure I agree.

In the case where a mandatory variable is added, then, yes, of course a change log should be generated, but the actor should be the actor who reloaded the Technique library (when known) - or at least he should be mentioned in the event log message.

However, in other cases, we are not modifying the Directives (in the same way that we don't have a change request - because we're not changing the target configuration on nodes which is what Rudder is about) so I don't think there should be any event logs. These are technical changes, that don't change anything at all from a user's point of view. What is happening here is that we are modifying the Techniques, so we should have an event log on them. This would be the same as when we change a Group: we don't have an event log for each Rule that uses that Group.

Does this make sense? Does this require changes?

Jonathan CLARKE wrote:

François ARMAND wrote:

• The comments in code say this happens when a Technique is changed without a version increment, for example when adding an optional variable. What happens if a mandatory variable is added to a Technique without incrementing it's version and this code gets run?

If the modification is unconsistent with previous state, the directives will be broken and the promise regeneration will fail. Else, default value will be used.

I'm not happy at all about using a default value in a variable without at least a WARNING to the user. It seems that modifying a Technique without incrementing it's version number is a dodgy case anyway, so I won't worry too much for now. Although I do see the need for this (for example, while creating a Technique incrementally and testing it), I definitely think this should come up with a clear warning in the UI and in logs along the lines of "WARNING: Technique name/version has been modified without incrementing it's version number!" and if any Directives get mandatory variables added with default values we should see a clear, precise list of them, something like "The following Directives have been automatically modified to add the new mandatory variable "Variable name" with default value "def val": Dir A, Dir B, etc".

It will be tracked on ticket #4307.

(oh, side note: can we stop calling that a "deployment", and change it "node configuration generation" or something like that?)

Oh, god, yes please! It's about time we renamed all this nonsense about deployments and "tml" in this part of the code. These terms make no sense, please destroy them with violence ASAP!

ASAP being the next release ? Or something more violent ?
Tracked in ticket #4308 (for now, targeted for 2.10).

[...]

• What about event logs? Are any created for the directive saves?

Event logs should be generated with a system user, but my last tests seems to not agree with that theorical behaviour. Of course, the theorical behavior is right, and so I'm looking why the observed one doesn't agree ;)

I'm not sure I agree.

In the case where a mandatory variable is added, then, yes, of course a change log should be generated, but the actor should be the actor who reloaded the Technique library (when known) - or at least he should be mentioned in the event log message.

That's the case, but in my test, the user starting the techniuqe reload was either "unknow rest api user" or "system user" (for automatic technique library reload).

However, in other cases, we are not modifying the Directives (in the same way that we don't have a change request - because we're not changing the target configuration on nodes which is what Rudder is about) so I don't think there should be any event logs. These are technical changes, that don't change anything at all from a user's point of view. What is happening here is that we are modifying the Techniques, so we should have an event log on them. This would be the same as when we change a Group: we don't have an event log for each Rule that uses that Group.

Does this make sense? Does this require changes?

Actually, the implementation is: generate an event log only if the directive actually change, so if (at least one) parameter value changed. So, I'm not sure about what was under your "in other cases", but I thing we are aligned. To be more specific: for example, the deletion of a variable, or the modification of its default value would lead to an event logged for relevant directive.

Hope it helps clarify things.