Add deploy option for Sync jobs

[eurotux-tec]

In our company, we use Icinga Director automations to create the customer infrastructure when it's in the cloud (AWS).

Currently, there is an import source responsible for collecting information about the servers to be monitored and a sync rule in charge of creating the objects (hosts).

Since it's an infrastructure in the cloud it's highly dynamic, so an automation is necessary to keep the monitoring up to date. For this, we created three jobs to execute periodically (5 minutes) to run the import source, the sync rule and the configuration deployment. Considering the current approach, configurations are deployed every 5 minutes, regardless of whether there are changes or not. In our scenario where there are many satellites and each deployment implies sending the configuration to all satellites, the impact is significant.

In this PR, a change to Sync Job is proposed so that it is responsible not only for creating the configuration but also for deploying it (when necessary).

Do you think that this makes sense or can we do it in a different way? If you agree, please merge this. If you don't, please let me know how can I improve the patch so that is gets merged.

[Al2Klimov]

Hello @eurotux-tec and thank you for contributing!

Imagine you've configured that job, update Director and suddenly the job does much more than you expect. Would you like this kind of behavior in your production? (I'd flip my table.)

Considering the current approach, configurations are deployed every 5 minutes, regardless of whether there are changes or not.

@Thomas-Gelf Can you confirm this?

https://github.com/Icinga/icingaweb2-module-director/blob/d804448d79531e6b4349d69b45b4cba679ce8640/library/Director/Job/ConfigJob.php#L31-L33

Best, AK

[Thomas-Gelf]

@Al2Klimov: you can have an automated Config Deploy Job scheduled to run every minute or even faster:

• it will not deploy unless the generated configuration differs from the last deployed configuration
• it will not even spend time and try to render a new configuration unless
  • there are un-deployed activities in the Activity Log
  • you enabled the flag "Force Rendering"

If you then also define a grace period you can for example make sure that it deploys at most every X minutes, even if there are changes every few seconds. And when you select a Time Period you can for example make sure that automated Deployments occur only during office hours.

Well - and that's it. Works even in large environments with a massive amount of changes and lots of Import Sources and Sync Rules.

@eurotux-tec: thanks for your effort, but as @Al2Klimov already pointed out this change would be pretty unexpected. Quite some environments have only automated Import and Sync Jobs, but prefer to deploy manually as they do not trust their various config sources.

You wrote:

Considering the current approach, configurations are deployed every 5 minutes, regardless of whether there are changes or not.

Are you sure about this? This would be a severe bug.

[araujorm]

Hello.

There are situations when having a deploy forced every 5 minutes is undesirable, because then you can't be adding new stuff without it being deployed without wanting yet. So I prefer the alternative proposed by this patch, even knowing I can still have a deploy being made once in a while when I'm editing other stuff, but at least it's not always, and it will be because my very important sync job had to do it.

So, maybe not by default, since not everyone wants to have automated deploys, but to have this as an option is a must on certain environments. If it can be made optional, would anyone disagree on it being merged?

[cla-bot[bot]]

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @araujorm

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

• If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

[eurotux-tec]

Hello.

Patch has been reworked to not affect the default behaviour. Instead, a new option "Deploy" has been added to the Sync Job config. That option is only visible/affects if "Apply changes" is "Yes".

By default, the "Deploy" option is "No". Other possible values are:

• Yes: if there are changes made by this sync job, a deploy will be made at the end, but only if there were no other pending changes before (so that an automated job doesn't deploy changes being made by humans at that same time)
• Yes (forced): a deploy will always be made (provided there are changes made by this sync job) even if there were other pending changes beforehand.

With these changes, and with the new feature being completely optional, is this pull request now acceptable?

Best regards.

[eurotux-tec]

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @araujorm

* If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please [contact us](https://icinga.com/company/contact/) if you think this is the case.

* If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

CLA has already been signed by the contributor.

[araujorm]

Hi. I've signed the CLA (with both of my emails) but it is not being recognized (same thing seems to be blocking https://github.com/Icinga/icingaweb2-module-director/pull/2180). Can someone force a re-check, or tell me if I'm doing something wrong? Also, can we have feedback about this patch, and get it merged if it's good?

[bobapple]

@cla-bot check

[Thomas-Gelf]

Feedback regarding this patch:

• please use camelCase for variable names
• rename $safe_to_deploy to $shouldDeploy and explicitly set it to false per default. There is currently no place in your code, where it might be undefined - don't worry. But it's better to have all variables initialized, to avoid potential issues with future changes
• when adding settings where we have no defaults in old setups, it's good practice to explicitly name the expected default value in the code: $this->getSetting('deploy', 'n')
• please reduce line 42 to if ($madeChanges && $shouldDeploy) and do all the other math above when initializing those variables
• echo/printf are bad. Please use $this->info/warning/error(), or the Icinga Logger class.
• no need to wipeInactiveStages, collectLogFiles is what you're looking for. The background daemon now does this job on it's own, so I would stay away from triggering it. To be on the safe side, you might still want to do so - go for it. The only objection I have: potential race conditions. There is currently no protection against collectLogFiles twice in parallel, so one task might delete an mark a deployment succeeded or failed, while the other task expects that get that stage (after asking the DB for missing ones), see's nothing - and marks the deployment as missing. Not your problem, just wanted to mention it. To fix it we must replace $deployment->store() with an explicit query with startup_succeeded IS NULL - that would help
• IMHO the Sync shouldn't fail because of deployment issues (Icinga not available, configuration errors) - I'd prefer to catch deployment problems and to emit warnings in such case
• I do not like y/f/n and the fake boolean on export. It's not a boolean, it's an enum - so feel free to name the setting 'deploy_changes' with options like 'side_effect_free', 'always' and 'never'

I'll add some more thoughts in my next comment.

[Thomas-Gelf]

Some thoughts:

We recently implemented Configuration Branches for the Icinga Director. The code is in place and working, schema changes have already been applied. The related GUI controls are (intentionally) still missing.

We're now able to apply changes to Configuration Branches, which are safe different realities - similar to branches in your VCS. Sync runs are part of this whole picture. Plans are to offer a "Sync to a temporary Branch" functionality. This will allow one to navigate all changes applied by a Sync rule directly in the Director UI, and to then merge or reject those changes at will, and all at once.

Also related: we implemented Push-only Import Sources, where instead of pulling data, you ship it to the Import Source (HTTP POST or similar). This is somewhat hidden, and leveraged only by some (or at least one) add-on modules. There we also trigger related Sync Rules in case the imported data changed.

This will probably ask for some additional flags on the Import and Sync objects themselves. Wanted to mention this, as you might have to expect some delay for this feature. Depending on where they move, we might prefer to shift your setting from the Job to the Sync rule itself.

Cheers, Thomas

[araujorm]

@Thomas-Gelf Good morning Thomas.

Thanks for sharing and for the feedback. Those new features look promising, we'll try to keep up the pace and adapt to them as needed.

We'll now work on the changes you mentioned and update this thread when ready.

[araujorm]

Hello.

Remade the proposed patch as follows:

• updated to merge with current master
• usage of camelCase for variable names and renamed introduced variables as such
• explicitly name the expected default value in the code when using getSetting
• reformulated some execution logic in order to only see if deploys should be made after knowing changes were made, and there use a switch statement for better readability
• renamed the deploy settings to side_effect_free ("Yes (side effect free)"), always ("Yes (always)") and never ("Never")
• trigger the deploy using ConditionalDeployment and ConditionalConfigRenderer objects, as implemented by ConfigJob in more recent code, thus avoiding the rest of the problems previously pointed bt @Thomas-Gelf

@Thomas-Gelf Can you see if it's good? Many thanks.

[araujorm]

Sorry, last version missed importing the necessary namespaces for ConditionalConfigRenderer and ConditionalDeployment, and needed to remove some others used in the first version of this patch. Fixed that in latest update to the branch.

[araujorm]

Yet another correction... the new logic wasn't working, since one must first check if there are other pending changes before the job eventually makes more. Fixed too, and is now according to:

• please reduce line 42 to if ($madeChanges && $shouldDeploy) and do all the other math above when initializing those variables

Sorry again, and thanks for your time to review.

[araujorm]

Hello.

We've refreshed the proposed commit so that it merges with the current master.

Could we have any feedback on whether it's acceptable, or if there is any other approach you'd prefer instead?

[lippserd]

@araujorm I just wanted to send you a quick note because it looks like you are still actively using Icinga and need these changes. At the end of this year we will start planning the feature release and will take this PR into account. Please excuse the long wait.

[araujorm]

Good afternoon. Branch of the merge request was rebased so that the MR is valid for latest release and master branch.