Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dev.icinga.com #7355] Exclude option for TimePeriod definitions #2040

Closed
icinga-migration opened this issue Oct 8, 2014 · 31 comments
Closed
Labels
enhancement New feature or request
Milestone

Comments

@icinga-migration
Copy link

This issue has been migrated from Redmine: https://dev.icinga.com/issues/7355

Created by KeX on 2014-10-08 13:14:35 +00:00

Assignee: mfriedrich
Status: Resolved (closed on 2016-05-21 18:40:03 +00:00)
Target Version: 2.5.0
Last Update: 2016-05-21 18:40:03 +00:00 (in Redmine)


Hi

On the icinga-users mailnig list I've seen a discussion about excludes in timeperiodes are not possible in icinga2? We've used this feature for a couple of yours now in icinga1 without a problem. It would be great if this feature is available for icinga2 too.

This is the configuration for icinga1:

define timeperiod{
        timeperiod_name on_call
        alias                On Call
        monday          07:00-20:00
        tuesday          07:00-20:00
        wednesday   07:00-20:00
        thursday        07:00-20:00
        friday              07:00-20:00
        exclude          vacation
}

define timeperiod{
        timeperiod_name                       vacation
        alias                                              Vacation
        2014-01-11 - 2014-01-06         00:00-24:00
        2014-12-29 - 2014-12-31         00:00-24:00
}

And here the documentation:
http://docs.icinga.org/latest/en/objectdefinitions.html#timeperiod

It would be great if this feature finds its way back into icinga2.

Best regards,

Chris...

Attachments

Changesets

2016-05-21 17:02:42 +00:00 by Reamer 699644b

Implement exclude and include ability for timeranges

With this change we can include and exclude timeranges in timeranges. I
think we need this feature to support holidays and so on

refs #7355

Signed-off-by: Michael Friedrich <michael.friedrich@netways.de>

2016-05-21 18:33:09 +00:00 by Reamer 54e1c8a

Implement exclude and include ability for TimePeriod objects

This feature allows to exclude and include specific time period
objects and their time ranges from an existing time period object.

This comes in handy when e.g. excluding holidays.

fixes #7355

Signed-off-by: Michael Friedrich <michael.friedrich@netways.de>

Relations:

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2014-10-11 15:03:44 +00:00

  • Subject changed from Exclude in Timeperiodes to Exclude option for TimePeriod definitions

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2014-10-11 15:03:54 +00:00

  • Category set to libicinga

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2014-11-24 13:15:18 +00:00

  • Target Version set to 2.3.0

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2014-11-28 16:49:46 +00:00

  • Duplicated set to 7885

@icinga-migration
Copy link
Author

Updated by gbeutner on 2014-12-02 09:50:09 +00:00

  • Assigned to set to mfriedrich

@icinga-migration
Copy link
Author

Updated by gbeutner on 2014-12-02 09:50:51 +00:00

  • Target Version changed from 2.3.0 to 2.2.2

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2014-12-11 15:50:45 +00:00

  • Assigned to deleted mfriedrich
  • Target Version changed from 2.2.2 to 2.3.0

Won't happen this year, rescheduling it for the next feature release (so far).

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2015-01-26 15:05:57 +00:00

  • Target Version changed from 2.3.0 to 2.4.0

@icinga-migration
Copy link
Author

Updated by KeX on 2015-01-28 09:47:05 +00:00

Are there any news/plans for this? Cause we are using this heavily in Icinga1. We would like to migrate to Icinga2, but our configuration rely on this option.

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2015-01-28 10:08:24 +00:00

There are currently no dev resources available for implementing such a feature. Feel free to sponsor a patch.

@icinga-migration
Copy link
Author

Updated by tgelf on 2015-04-22 08:21:43 +00:00

**

I'd also love to see this implemented. "Notify 9to5 but not on holidays" is a very basic feature. It's not needed when playing around or when "just sending mails", but most enterprises with an (even small-sized) IT department have to make sure to not bother their employees on holidays. At least unless they agreed on doing so.

In case the "exclude" logic as in Icinga 1.x is too complex to get it working smoothless together with our generic "import" implementation I could also immagine a simpler variant like follows:

object TimePeriod   "Office hours on work days" {
  import "legacy-timeperiod" 

  ranges = {
    monday = "09:00-17:00"
    tuesday = "09:00-17:00"
    wednesday = "09:00-17:00"
    thursday = "09:00-17:00"
    friday = "09:00-12:00"
  }

  ignore_ranges = {
    "december 24" = "14:00-24:00"
    "december 25" = "00:00-24:00"
  }
}

Given this, one could create a config as follows:

object TimePeriod   "Office hours" {
  import "legacy-timeperiod"
  ranges += {
    monday = "09:00-17:00"
    tuesday = "09:00-17:00"
    wednesday = "09:00-17:00"
    thursday = "09:00-17:00"
    friday = "09:00-12:00"
  }
}

template TimePeriod   "Not on German Holidays" {
  ignore_ranges += {
    "december 24" = "14:00-24:00"
    "december 25" = "00:00-24:00"
    # ...
  }
}

template TimePeriod   "Not on Bavarian Holidays" {
  ignore_ranges += {
    "august 15" = "00:00-24:00"
    # ...
  }
}


object TimePeriod   "Office hours on work days" {
  import "Office Hours
  import "Not on German Holidays"
  import "Not on Bavarian Holidays"
}

Of course I could also live with a legacy-like exclude syntax .Implementing the above could be a quick win satisfying most needs while avoiding needless complexity.

Cheers,
Thomas

@icinga-migration
Copy link
Author

Updated by KeX on 2015-04-22 08:32:11 +00:00

We are using this in Icinga 1.x exactly the way, Thomas wrote it! His configuration snip would perfectly fit our needs for Icinga 2! This would be great for icinga2!

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2015-05-19 07:57:44 +00:00

  • Target Version deleted 2.4.0

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2015-06-22 11:14:07 +00:00

I am aware of the issue, and I also do like Tom's approach. Unfortunately I don't have a time estimation nor slot where this can be implemented in the next months.

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2015-06-23 13:36:39 +00:00

  • Target Version set to Backlog

@icinga-migration
Copy link
Author

Updated by gbeutner on 2015-08-27 14:36:41 +00:00

Implementing this with 'import' would probably be the easiest way. However there's an obvious drawback:

object TimePeriod "not lunch time" {
  ignore_ranges = {
    monday = "12:00-13:00"
    tuesday = "12:00-13:00"
    wednesday = "12:00-13:00"
    thursday = "12:00-13:00"
    friday = "12:00-13:00"
    saturday = "12:00-13:00"
    sunday = "12:00-13:00"
  }
}

object TimePeriod "not weekends" {
  ignore_ranges = {
    saturday = "0:00-24:00"
    sunday = "0:00-24:00"
  }
}

object TimePeriod "working hours" {
  import "24x7"
  import "not weekends"
  import "not lunch time"
}

Now, while it's immediately obvious to me why the excludes for saturday and sunday aren't working this might be difficult to grasp for ordinary users.

I suspect this feature would be more powerful if we implement it like in Icinga 1.x:

object TimePeriod "..." {
  ranges = { ... }
  includes = [ "tp-1", "tp-2" ]
  excludes = [ "holidays", ... ]
}

Also, this way users wouldn't have to build "negative" time periods (e.g. "not bavarian holidays").

@icinga-migration
Copy link
Author

Updated by KeX on 2015-08-28 07:37:13 +00:00

The 2nd example you gave, like in Icinga 1.x, would be exactly what I'm looking for.

  • ranges are the normal working hours
  • include on-duty ranges
  • exclude holidays

Would be a great enhancement for the notification system!

@icinga-migration
Copy link
Author

Updated by jerue on 2015-10-30 13:08:48 +00:00

The same like KeX.
It´s exactly what I´m looking for. It would be great if it eventually is in the featurelist sometime.

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2015-10-30 13:22:17 +00:00

We will re-evaluate all issues on Backlog after the 2.4 milestone release.

@icinga-migration
Copy link
Author

Updated by KeX on 2016-01-29 11:32:39 +00:00

Are there any news on this Issue? For us this is the only show stopper to use icinga2 in production. I'm really looking forward to see this feature in icinga2.

@icinga-migration
Copy link
Author

Updated by drbayer on 2016-02-05 17:55:15 +00:00

+1

I'm trying to migrate from Nagios to Icinga2 and having some way to exclude ranges would be really helpful. As it is today, building a "business hours excluding holidays" (for example) is incredibly tedious.

@icinga-migration
Copy link
Author

Updated by Reamer on 2016-02-28 22:58:57 +00:00

  • File added 0001-Implement-exclude-and-include-ability-for-timeranges.patch

Hi,

i try to implement this feature. #71

Hope the code quality is good enough.

One Question. What's the different between this two locks "ObjectLock olock(timeranges)" and "ObjectLock dlock(segments)"?

Local tests are successfully.

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2016-03-11 20:55:40 +00:00

I did not test it yet, but there's three things to consider with checking the timeperiod ranges on-demand at runtime (IsInside())

  1. performance
  2. external interfaces requiring resolved time ranges
  3. do not use an extra flag for preferring includes. maybe there's a more elegant way to solve this.

@icinga-migration
Copy link
Author

Updated by Reamer on 2016-03-25 12:37:45 +00:00

  • File added 0001-Implement-exclude-and-include-ability-for-timeranges.patch

Hi,

thanks for feedback @dnsmichi

Next try with an other idea:
Merge other timezones into an existing one.

  1. Merge is only done on update -> So performance should be no problem. Of course we have new code, but also a new feature :D
  2. external interfaces should be no problem, because a merged timerange is as good as a normal timerange
  3. I doesn't know any possiblity without an extra flag. If someone has an idea, I’m glad to hear that.

@icinga-migration
Copy link
Author

Updated by Reamer on 2016-04-16 08:00:19 +00:00

Can i get any feedback?

@icinga-migration
Copy link
Author

Updated by KeX on 2016-04-20 07:23:56 +00:00

Hi,

I've testet your patch with Version 2.4.4. It works like expected. My timeperiodes.conf looks like this:

object TimePeriod "work_hours" {
  import "legacy-timeperiod"

  ranges = {
    monday = "07:00-20:00"
    tuesday = "07:00-20:00"
    wednesday = "07:00-20:00"
    thursday = "07:00-20:00"
    friday = "07:00-20:00"
  }
  excludes = ["vacation"]
}

object TimePeriod "vacation" {
  import "legacy-timeperiod"

  ranges = {
    "2016-04-19 - 2016-04-20" = "00:00-09:00"
    "2016-04-20" = "09:05-09:10"
  }
}

And tested via a passive check.
No notifications where sent yesterday and today from 00:00 to 09:00. And today from 09:05 to 09:10. After 09:10 notifications where sent again.

Can't say something about the code quality, but the feature does what we are looking for! I hope this patch finds it's way into the stable release.

Thanks for that!
Chris...

@icinga-migration
Copy link
Author

Updated by KeX on 2016-04-20 12:35:51 +00:00

... and it works with verison 2.4.5 too :)

Chris...

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2016-05-06 07:46:20 +00:00

  • Status changed from New to Assigned
  • Assigned to set to mfriedrich
  • Target Version changed from Backlog to 2.5.0

I'll have a look into the pending review. If nothing goes wrong, 2.5.0 is a good candidate.

@icinga-migration
Copy link
Author

Updated by Reamer on 2016-05-13 20:47:57 +00:00

If i can help you, please let me know.

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2016-05-21 18:34:22 +00:00

Review

Test-Config (adjust this for a time window for the excluded timeperiod where no checks will happen).

object Host "7355-host" {
  check_command = "random"
  check_interval = 30s
  retry_interval = 15s
  check_period = "sometimes"
}

object TimePeriod "sometimes" {
  import "legacy-timeperiod"

  prefer_includes = false
  includes = [ "7355-anytime" ]
  excludes = [ "7355-evening-silence" ]
}

object TimePeriod "7355-evening-silence" {
  import "legacy-timeperiod"

  ranges = {
    "monday"    = "19:00-24:00"
    "tuesday"   = "19:00-24:00"
    "wednesday" = "19:00-24:00"
    "thursday"  = "19:00-24:00"
    "friday"    = "19:00-24:00"
    "saturday"  = "19:00-24:00"
    "sunday"    = "19:00-24:00"
  }
}

object TimePeriod "7355-anytime" {
  import "legacy-timeperiod"

  display_name = "Icinga 2 24x7 TimePeriod"
  ranges = {
    "monday"    = "00:00-24:00"
    "tuesday"   = "00:00-24:00"
    "wednesday" = "00:00-24:00"
    "thursday"  = "00:00-24:00"
    "friday"    = "00:00-24:00"
    "saturday"  = "00:00-24:00"
    "sunday"    = "00:00-24:00"
  }
}

Log:

# icinga2 daemon -x debug | grep 7355

[2016-05-21 19:37:36 +0200] notice/CheckerComponent: Skipping check for object '7355-host': not in check_period

If I remove "prefer_includes" checks are executed again, since "7355-anytime" wins against the excluded timeperiod.

Feature

To be honest I thought of just adding excludes and not any kind of includes, as you already have the possibility to import other timeperiod objects. Although it is cumbersome to override and modify existing timeperiod attributes from a template. Especially given its old legacy syntax.

So from an end user perspective, it totally makes sense to support both, includes and excludes. Obviously not that many users will go for includes, but rather exclude their holidays (and only update that timeperiod object then).

Code

Added some new lines, changes the variable names to capitalised ones. The underscore in variables is old and will be gone once in a while.

I've also modified the debug log entry for the checker component to highlight which time period prevents check execution. Same goes for notifications (and users) log entries.

[2016-05-21 20:22:53 +0200] notice/CheckerComponent: Skipping check for object '7355-host': not in check period 'prod-notification'

Documentation

Added an example using holidays and weekend excludes as sub chapter for the time periods. That way the users will see how time period exclusions work in a reasonable example. It also illustrates the different ways of specifying the ranges which was missing in there.

IsInside

Those calls from inside the CheckerComponent as well as API work very well since the segments are already updated.

External interfaces

External interfaces such as DB IDO will only dump a very "static" time period time range.

CREATE TABLE IF NOT EXISTS icinga_timeperiods (
  timeperiod_id bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  instance_id bigint unsigned default 0,
  config_type smallint default 0,
  timeperiod_object_id bigint unsigned default 0,
  alias TEXT character set latin1  default '',
  PRIMARY KEY  (timeperiod_id),
  UNIQUE KEY instance_id (instance_id,config_type,timeperiod_object_id)
) ENGINE=InnoDB  COMMENT='Timeperiod definitions';

CREATE TABLE IF NOT EXISTS icinga_timeperiod_timeranges (
  timeperiod_timerange_id bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  instance_id bigint unsigned default 0,
  timeperiod_id bigint unsigned default 0,
  day smallint default 0,
  start_sec  int default 0,
  end_sec  int default 0,
  PRIMARY KEY  (timeperiod_timerange_id)
) ENGINE=InnoDB  COMMENT='Timeperiod definitions';

meaning to say, all time ranges parsed from the configuration will be put into the database. Nothing you possibly could rely onto Especially not in Icinga 1.x where the excludes where implemented as well, but not exported into such interfaces.

It may of course be relevant for SLA reports to somehow export the config options (although this would require additional DB IDO tables, as timeperiods would have an 1..n relationship to includes/excludes being an array.
That way they could calculate the time period themselves. Obviously I don't have any special use case for it now, since such periods normally suppress something - checks, notifications which are probably out of sla scope anyways.
If someone wants to have that exported, please open a new discussion issue then.

Conclusion

I've tested this now for a couple of hours and reviewed it (doing that on a Saturday on vacation, no time otherwise). I don't have any better names nor a suggestion for prefer_includes.

I like the patch, and included my modifications, I've merged it to master. Thanks a lot :)

@icinga-migration
Copy link
Author

Updated by Reamer on 2016-05-21 18:40:03 +00:00

  • Status changed from Assigned to Resolved
  • Done % changed from 0 to 100

Applied in changeset 54e1c8a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant