From 87a92b4d04a4bc5ffa9607646a6b3c86adeb4b59 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Tue, 28 May 2024 11:08:53 +0200 Subject: [PATCH 1/8] Query timeperiod_entry directly into the final struct This adds the required fields and tags to timeperiod.Entry so that this struct can be used directly for querying the database. This removes the previous inline struct TimeperiodEntry that was present in the config loading code. All the initialization is now done in the Init() method instead of when copying the values between the structs. This is done in preparation for changes to schedules where timeperiod entries are loaded directly as part of schedules. --- internal/config/timeperiod.go | 61 +++++--------------- internal/timeperiod/timeperiod.go | 80 +++++++++++++++++--------- internal/timeperiod/timeperiod_test.go | 58 ++++++++++++------- 3 files changed, 105 insertions(+), 94 deletions(-) diff --git a/internal/config/timeperiod.go b/internal/config/timeperiod.go index 4127ef42..a5c481c5 100644 --- a/internal/config/timeperiod.go +++ b/internal/config/timeperiod.go @@ -2,13 +2,11 @@ package config import ( "context" - "database/sql" "fmt" "github.com/icinga/icinga-go-library/types" "github.com/icinga/icinga-notifications/internal/timeperiod" "github.com/jmoiron/sqlx" "go.uber.org/zap" - "time" ) func (r *RuntimeConfig) fetchTimePeriods(ctx context.Context, tx *sqlx.Tx) error { @@ -26,66 +24,37 @@ func (r *RuntimeConfig) fetchTimePeriods(ctx context.Context, tx *sqlx.Tx) error timePeriodsById[period.ID] = period } - type TimeperiodEntry struct { - ID int64 `db:"id"` - TimePeriodID int64 `db:"timeperiod_id"` - StartTime types.UnixMilli `db:"start_time"` - EndTime types.UnixMilli `db:"end_time"` - Timezone string `db:"timezone"` - RRule sql.NullString `db:"rrule"` - Description sql.NullString `db:"description"` - } - - var entryPtr *TimeperiodEntry + var entryPtr *timeperiod.Entry stmt = r.db.BuildSelectStmt(entryPtr, entryPtr) r.logger.Debugf("Executing query %q", stmt) - var entries []*TimeperiodEntry + var entries []*timeperiod.Entry if err := tx.SelectContext(ctx, &entries, stmt); err != nil { r.logger.Errorln(err) return err } - for _, row := range entries { - p := timePeriodsById[row.TimePeriodID] + for _, entry := range entries { + p := timePeriodsById[entry.TimePeriodID] if p == nil { r.logger.Warnw("ignoring entry for unknown timeperiod_id", - zap.Int64("timeperiod_entry_id", row.ID), - zap.Int64("timeperiod_id", row.TimePeriodID)) + zap.Int64("timeperiod_entry_id", entry.ID), + zap.Int64("timeperiod_id", entry.TimePeriodID)) continue } if p.Name == "" { - p.Name = fmt.Sprintf("Time Period #%d", row.TimePeriodID) - if row.Description.Valid { - p.Name += fmt.Sprintf(" (%s)", row.Description.String) + p.Name = fmt.Sprintf("Time Period #%d", entry.TimePeriodID) + if entry.Description.Valid { + p.Name += fmt.Sprintf(" (%s)", entry.Description.String) } } - loc, err := time.LoadLocation(row.Timezone) - if err != nil { - r.logger.Warnw("ignoring time period entry with unknown timezone", - zap.Int64("timeperiod_entry_id", row.ID), - zap.String("timezone", row.Timezone), - zap.Error(err)) - continue - } - - entry := &timeperiod.Entry{ - Start: row.StartTime.Time().Truncate(time.Second).In(loc), - End: row.EndTime.Time().Truncate(time.Second).In(loc), - TimeZone: row.Timezone, - } - - if row.RRule.Valid { - entry.RecurrenceRule = row.RRule.String - } - - err = entry.Init() + err := entry.Init() if err != nil { r.logger.Warnw("ignoring time period entry", - zap.Int64("timeperiod_entry_id", row.ID), - zap.String("rrule", entry.RecurrenceRule), + zap.Int64("timeperiod_entry_id", entry.ID), + zap.String("rrule", entry.RRule.String), zap.Error(err)) continue } @@ -94,9 +63,9 @@ func (r *RuntimeConfig) fetchTimePeriods(ctx context.Context, tx *sqlx.Tx) error r.logger.Debugw("loaded time period entry", zap.String("timeperiod", p.Name), - zap.Time("start", entry.Start), - zap.Time("end", entry.End), - zap.String("rrule", entry.RecurrenceRule)) + zap.Time("start", entry.StartTime.Time()), + zap.Time("end", entry.EndTime.Time()), + zap.String("rrule", entry.RRule.String)) } for _, p := range timePeriodsById { diff --git a/internal/timeperiod/timeperiod.go b/internal/timeperiod/timeperiod.go index cff97a1a..5f123494 100644 --- a/internal/timeperiod/timeperiod.go +++ b/internal/timeperiod/timeperiod.go @@ -1,6 +1,9 @@ package timeperiod import ( + "database/sql" + "github.com/icinga/icingadb/pkg/types" + "github.com/pkg/errors" "github.com/teambition/rrule-go" "log" "time" @@ -47,37 +50,62 @@ func (p *TimePeriod) NextTransition(base time.Time) time.Time { } type Entry struct { - Start, End time.Time - - // for future use - TimeZone string // or *time.Location + ID int64 `db:"id"` + TimePeriodID int64 `db:"timeperiod_id"` + StartTime types.UnixMilli `db:"start_time"` + EndTime types.UnixMilli `db:"end_time"` + Timezone string `db:"timezone"` + RRule sql.NullString `db:"rrule"` // RFC5545 RRULE + Description sql.NullString `db:"description"` + RotationMemberID sql.NullInt64 `db:"rotation_member_id"` + + initialized bool + rrule *rrule.RRule +} - RecurrenceRule string // RFC5545 RRULE - rrule *rrule.RRule +// TableName implements the contracts.TableNamer interface. +func (e *Entry) TableName() string { + return "timeperiod_entry" } -// Init initializes the rrule instance from the configured rrule string +// Init prepares the Entry for use after being read from the database. +// +// This includes loading the timezone information and parsing the recurrence rule if present. func (e *Entry) Init() error { - if e.rrule != nil || e.RecurrenceRule == "" { + if e.initialized { return nil } - option, err := rrule.StrToROptionInLocation(e.RecurrenceRule, e.Start.Location()) + loc, err := time.LoadLocation(e.Timezone) if err != nil { - return err + return errors.Wrapf(err, "timeperiod entry has an invalid timezone %q", e.Timezone) } - if option.Dtstart.IsZero() { - option.Dtstart = e.Start - } + // Timestamps in the database are stored with millisecond resolution while RRULE only operates on seconds. + // Truncate to whole seconds in case there is sub-second precision. + // Additionally, set the location so that all times in this entry are consistent with the timezone of the entry. + e.StartTime = types.UnixMilli(e.StartTime.Time().Truncate(time.Second).In(loc)) + e.EndTime = types.UnixMilli(e.EndTime.Time().Truncate(time.Second).In(loc)) - rule, err := rrule.NewRRule(*option) - if err != nil { - return err - } + if e.RRule.Valid { + option, err := rrule.StrToROptionInLocation(e.RRule.String, loc) + if err != nil { + return err + } + + if option.Dtstart.IsZero() { + option.Dtstart = e.StartTime.Time() + } - e.rrule = rule + rule, err := rrule.NewRRule(*option) + if err != nil { + return err + } + + e.rrule = rule + } + e.initialized = true return nil } @@ -88,11 +116,11 @@ func (e *Entry) Contains(t time.Time) bool { log.Printf("Can't initialize entry: %s", err) } - if t.Before(e.Start) { + if t.Before(e.StartTime.Time()) { return false } - if t.Before(e.End) { + if t.Before(e.EndTime.Time()) { return true } @@ -101,7 +129,7 @@ func (e *Entry) Contains(t time.Time) bool { } lastStart := e.rrule.Before(t, true) - lastEnd := lastStart.Add(e.End.Sub(e.Start)) + lastEnd := lastStart.Add(e.EndTime.Time().Sub(e.StartTime.Time())) // Whether the date time is between the last recurrence start and the last recurrence end return (t.Equal(lastStart) || t.After(lastStart)) && t.Before(lastEnd) } @@ -115,13 +143,13 @@ func (e *Entry) NextTransition(t time.Time) time.Time { log.Printf("Can't initialize entry: %s", err) } - if t.Before(e.Start) { + if t.Before(e.StartTime.Time()) { // The passed time is before the configured event start time - return e.Start + return e.StartTime.Time() } - if t.Before(e.End) { - return e.End + if t.Before(e.EndTime.Time()) { + return e.EndTime.Time() } if e.rrule == nil { @@ -129,7 +157,7 @@ func (e *Entry) NextTransition(t time.Time) time.Time { } lastStart := e.rrule.Before(t, true) - lastEnd := lastStart.Add(e.End.Sub(e.Start)) + lastEnd := lastStart.Add(e.EndTime.Time().Sub(e.StartTime.Time())) if (t.Equal(lastStart) || t.After(lastStart)) && t.Before(lastEnd) { // Base time is after the last transition begin but before the last transition end return lastEnd diff --git a/internal/timeperiod/timeperiod_test.go b/internal/timeperiod/timeperiod_test.go index 709e5655..3a86c6d3 100644 --- a/internal/timeperiod/timeperiod_test.go +++ b/internal/timeperiod/timeperiod_test.go @@ -1,8 +1,10 @@ package timeperiod_test import ( + "database/sql" "fmt" "github.com/icinga/icinga-notifications/internal/timeperiod" + "github.com/icinga/icingadb/pkg/types" "github.com/stretchr/testify/assert" "github.com/teambition/rrule-go" "testing" @@ -19,9 +21,13 @@ func TestEntry(t *testing.T) { end := berlinTime("2023-03-01 11:00:00") until := berlinTime("2023-03-03 09:00:00") e := &timeperiod.Entry{ - Start: start, - End: end, - RecurrenceRule: fmt.Sprintf("FREQ=DAILY;UNTIL=%s", until.UTC().Format(rrule.DateTimeFormat)), + StartTime: types.UnixMilli(start), + EndTime: types.UnixMilli(end), + Timezone: berlin, + RRule: sql.NullString{ + String: fmt.Sprintf("FREQ=DAILY;UNTIL=%s", until.UTC().Format(rrule.DateTimeFormat)), + Valid: true, + }, } t.Run("TimeAtFirstRecurrenceStart", func(t *testing.T) { @@ -73,9 +79,10 @@ func TestEntry(t *testing.T) { start := berlinTime("2023-03-25 01:00:00") end := berlinTime("2023-03-25 02:30:00") e := &timeperiod.Entry{ - Start: start, - End: end, - RecurrenceRule: "FREQ=DAILY", + StartTime: types.UnixMilli(start), + EndTime: types.UnixMilli(end), + Timezone: berlin, + RRule: sql.NullString{String: "FREQ=DAILY", Valid: true}, } assert.True(t, e.Contains(start)) @@ -95,9 +102,10 @@ func TestEntry(t *testing.T) { start := berlinTime("2023-03-01 08:00:00") end := berlinTime("2023-03-01 12:30:00") e := &timeperiod.Entry{ - Start: start, - End: end, - RecurrenceRule: "FREQ=DAILY", + StartTime: types.UnixMilli(start), + EndTime: types.UnixMilli(end), + Timezone: berlin, + RRule: sql.NullString{String: "FREQ=DAILY", Valid: true}, } t.Run("TimeAtFirstRecurrenceStart", func(t *testing.T) { @@ -124,9 +132,10 @@ func TestEntry(t *testing.T) { start := berlinTime("2023-03-25 01:00:00") end := berlinTime("2023-03-25 02:30:00") e := &timeperiod.Entry{ - Start: start, - End: end, - RecurrenceRule: "FREQ=DAILY", + StartTime: types.UnixMilli(start), + EndTime: types.UnixMilli(end), + Timezone: berlin, + RRule: sql.NullString{String: "FREQ=DAILY", Valid: true}, } assert.Equal(t, end, e.NextTransition(start), "next transition should match the first recurrence end") @@ -152,9 +161,10 @@ func TestTimePeriodTransitions(t *testing.T) { p := &timeperiod.TimePeriod{ Name: "Transition Test", Entries: []*timeperiod.Entry{{ - Start: start, - End: end, - RecurrenceRule: "FREQ=DAILY", + StartTime: types.UnixMilli(start), + EndTime: types.UnixMilli(end), + Timezone: berlin, + RRule: sql.NullString{String: "FREQ=DAILY", Valid: true}, }}, } @@ -170,14 +180,16 @@ func TestTimePeriodTransitions(t *testing.T) { Name: "Transition Test", Entries: []*timeperiod.Entry{ { - Start: start, - End: end, - RecurrenceRule: "FREQ=HOURLY;BYHOUR=1,3,5,7,9,11,13,15", + StartTime: types.UnixMilli(start), + EndTime: types.UnixMilli(end), + Timezone: berlin, + RRule: sql.NullString{String: "FREQ=HOURLY;BYHOUR=1,3,5,7,9,11,13,15", Valid: true}, }, { - Start: berlinTime("2023-03-27 08:00:00"), - End: berlinTime("2023-03-27 08:30:00"), - RecurrenceRule: "FREQ=HOURLY;BYHOUR=0,2,4,6,8,10,12,14", + StartTime: types.UnixMilli(berlinTime("2023-03-27 08:00:00")), + EndTime: types.UnixMilli(berlinTime("2023-03-27 08:30:00")), + Timezone: berlin, + RRule: sql.NullString{String: "FREQ=HOURLY;BYHOUR=0,2,4,6,8,10,12,14", Valid: true}, }, }, } @@ -206,8 +218,10 @@ func TestTimePeriodTransitions(t *testing.T) { }) } +const berlin = "Europe/Berlin" + func berlinTime(value string) time.Time { - loc, err := time.LoadLocation("Europe/Berlin") + loc, err := time.LoadLocation(berlin) if err != nil { panic(err) } From 0fbbe5200608bcbcd28317601415eac28a713ebd Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Tue, 28 May 2024 10:55:18 +0200 Subject: [PATCH 2/8] Implement zapcore.ObjectMarshaler for TimePeriod and Entry This allows to easily log those using zap.Object(). --- internal/config/timeperiod.go | 9 +++------ internal/timeperiod/timeperiod.go | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/internal/config/timeperiod.go b/internal/config/timeperiod.go index a5c481c5..9ed7274e 100644 --- a/internal/config/timeperiod.go +++ b/internal/config/timeperiod.go @@ -53,8 +53,7 @@ func (r *RuntimeConfig) fetchTimePeriods(ctx context.Context, tx *sqlx.Tx) error err := entry.Init() if err != nil { r.logger.Warnw("ignoring time period entry", - zap.Int64("timeperiod_entry_id", entry.ID), - zap.String("rrule", entry.RRule.String), + zap.Object("entry", entry), zap.Error(err)) continue } @@ -62,10 +61,8 @@ func (r *RuntimeConfig) fetchTimePeriods(ctx context.Context, tx *sqlx.Tx) error p.Entries = append(p.Entries, entry) r.logger.Debugw("loaded time period entry", - zap.String("timeperiod", p.Name), - zap.Time("start", entry.StartTime.Time()), - zap.Time("end", entry.EndTime.Time()), - zap.String("rrule", entry.RRule.String)) + zap.Object("timeperiod", p), + zap.Object("entry", entry)) } for _, p := range timePeriodsById { diff --git a/internal/timeperiod/timeperiod.go b/internal/timeperiod/timeperiod.go index 5f123494..55de311f 100644 --- a/internal/timeperiod/timeperiod.go +++ b/internal/timeperiod/timeperiod.go @@ -5,6 +5,7 @@ import ( "github.com/icinga/icingadb/pkg/types" "github.com/pkg/errors" "github.com/teambition/rrule-go" + "go.uber.org/zap/zapcore" "log" "time" ) @@ -19,6 +20,13 @@ func (p *TimePeriod) TableName() string { return "timeperiod" } +// MarshalLogObject implements the zapcore.ObjectMarshaler interface. +func (p *TimePeriod) MarshalLogObject(encoder zapcore.ObjectEncoder) error { + encoder.AddInt64("id", p.ID) + encoder.AddString("name", p.Name) + return nil +} + // Contains returns whether a point in time t is covered by this time period, i.e. there is an entry covering it. func (p *TimePeriod) Contains(t time.Time) bool { for _, e := range p.Entries { @@ -68,6 +76,18 @@ func (e *Entry) TableName() string { return "timeperiod_entry" } +// MarshalLogObject implements the zapcore.ObjectMarshaler interface. +func (e *Entry) MarshalLogObject(encoder zapcore.ObjectEncoder) error { + encoder.AddInt64("id", e.ID) + encoder.AddTime("start", e.StartTime.Time()) + encoder.AddTime("end", e.EndTime.Time()) + encoder.AddString("timezone", e.Timezone) + if e.RRule.Valid { + encoder.AddString("rrule", e.RRule.String) + } + return nil +} + // Init prepares the Entry for use after being read from the database. // // This includes loading the timezone information and parsing the recurrence rule if present. From 501ff1c6385061e175c7ba07a2ba35f0f279eb0b Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Mon, 6 May 2024 16:09:34 +0200 Subject: [PATCH 3/8] schema: Change as needed for the new rotations --- schema/pgsql/schema.sql | 72 +++++++++++++++++++--------- schema/pgsql/upgrades/026.sql | 88 +++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 23 deletions(-) create mode 100644 schema/pgsql/upgrades/026.sql diff --git a/schema/pgsql/schema.sql b/schema/pgsql/schema.sql index 419293c4..9b7ffca3 100644 --- a/schema/pgsql/schema.sql +++ b/schema/pgsql/schema.sql @@ -11,7 +11,7 @@ CREATE TYPE incident_history_event_type AS ENUM ( 'closed', 'notified' ); -CREATE TYPE frequency_type AS ENUM ( 'MINUTELY', 'HOURLY', 'DAILY', 'WEEKLY', 'MONTHLY', 'QUARTERLY', 'YEARLY' ); +CREATE TYPE rotation_type AS ENUM ( '24-7', 'partial', 'multi' ); CREATE TYPE notification_state_type AS ENUM ( 'pending', 'sent', 'failed' ); -- IPL ORM renders SQL queries with LIKE operators for all suggestions in the search bar, @@ -92,47 +92,73 @@ CREATE TABLE schedule ( CONSTRAINT pk_schedule PRIMARY KEY (id) ); +CREATE TABLE rotation ( + id bigserial, + schedule_id bigint NOT NULL REFERENCES schedule(id), + -- the lower the more important, starting at 0, avoids the need to re-index upon addition + priority integer NOT NULL, + name text NOT NULL, + mode rotation_type NOT NULL, + -- JSON with rotation-specific attributes + -- Needed exclusively by Web to simplify editing and visualisation + options text NOT NULL, + + -- A date in the format 'YYYY-MM-DD' when the first handoff should happen. + -- It is a string as handoffs are restricted to happen only once per day + first_handoff date NOT NULL, + + -- Set to the actual time of the first handoff. + -- If this is in the past during creation of the rotation, it is set to the creation time. + -- Used by Web to avoid showing shifts that never happened + actual_handoff bigint NOT NULL, + + -- each schedule can only have one rotation with a given priority starting at a given date + UNIQUE (schedule_id, priority, first_handoff), + + CONSTRAINT pk_rotation PRIMARY KEY (id) +); + CREATE TABLE timeperiod ( id bigserial, - owned_by_schedule_id bigint REFERENCES schedule(id), -- nullable for future standalone timeperiods + owned_by_rotation_id bigint REFERENCES rotation(id), -- nullable for future standalone timeperiods CONSTRAINT pk_timeperiod PRIMARY KEY (id) ); +CREATE TABLE rotation_member ( + id bigserial, + rotation_id bigint NOT NULL REFERENCES rotation(id), + contact_id bigint REFERENCES contact(id), + contactgroup_id bigint REFERENCES contactgroup(id), + position integer NOT NULL, + + UNIQUE (rotation_id, position), -- each position in a rotation can only be used once + + -- Two UNIQUE constraints prevent duplicate memberships of the same contact or contactgroup in a single rotation. + -- Multiple NULLs are not considered to be duplicates, so rows with a contact_id but no contactgroup_id are + -- basically ignored in the UNIQUE constraint over contactgroup_id and vice versa. The CHECK constraint below + -- ensures that each row has only non-NULL values in one of these constraints. + UNIQUE (rotation_id, contact_id), + UNIQUE (rotation_id, contactgroup_id), + CHECK (num_nonnulls(contact_id, contactgroup_id) = 1), + + CONSTRAINT pk_rotation_member PRIMARY KEY (id) +); + CREATE TABLE timeperiod_entry ( id bigserial, timeperiod_id bigint NOT NULL REFERENCES timeperiod(id), + rotation_member_id bigint REFERENCES rotation_member(id), -- nullable for future standalone timeperiods start_time bigint NOT NULL, end_time bigint NOT NULL, -- Is needed by icinga-notifications-web to prefilter entries, which matches until this time and should be ignored by the daemon. until_time bigint, timezone text NOT NULL, -- e.g. 'Europe/Berlin', relevant for evaluating rrule (DST changes differ between zones) rrule text, -- recurrence rule (RFC5545) - -- Contains the same frequency types as in the rrule string except the `QUARTERLY` one, which is only offered - -- by web that is represented as `FREQ=MONTHLY;INTERVAL=3` in a RRule string. So, this should be also ignored - -- by the daemon. - frequency frequency_type, - description text, CONSTRAINT pk_timeperiod_entry PRIMARY KEY (id) ); -CREATE TABLE schedule_member ( - schedule_id bigint NOT NULL REFERENCES schedule(id), - timeperiod_id bigint NOT NULL REFERENCES timeperiod(id), - contact_id bigint REFERENCES contact(id), - contactgroup_id bigint REFERENCES contactgroup(id), - - -- There is no PRIMARY KEY in that table as either contact_id or contactgroup_id should be allowed to be NULL. - -- Instead, there are two UNIQUE constraints that prevent duplicate entries. Multiple NULLs are not considered to - -- be duplicates, so rows with a contact_id but no contactgroup_id are basically ignored in the UNIQUE constraint - -- over contactgroup_id and vice versa. The CHECK constraint below ensures that each row has only non-NULL values - -- in one of these constraints. - UNIQUE (schedule_id, timeperiod_id, contact_id), - UNIQUE (schedule_id, timeperiod_id, contactgroup_id), - CHECK (num_nonnulls(contact_id, contactgroup_id) = 1) -); - CREATE TABLE source ( id bigserial, -- The type "icinga2" is special and requires (at least some of) the icinga2_ prefixed columns. diff --git a/schema/pgsql/upgrades/026.sql b/schema/pgsql/upgrades/026.sql new file mode 100644 index 00000000..c2d73b8c --- /dev/null +++ b/schema/pgsql/upgrades/026.sql @@ -0,0 +1,88 @@ +DO $$ BEGIN + CREATE TYPE rotation_type AS ENUM ( '24-7', 'partial', 'multi' ); +EXCEPTION + WHEN duplicate_object THEN null; +END $$; + +CREATE TABLE IF NOT EXISTS rotation ( + id bigserial, + schedule_id bigint NOT NULL REFERENCES schedule(id), + -- the lower the more important, starting at 0, avoids the need to re-index upon addition + priority integer NOT NULL, + name text NOT NULL, + mode rotation_type NOT NULL, + -- JSON with rotation-specific attributes + -- Needed exclusively by Web to simplify editing and visualisation + options text NOT NULL, + + -- A date in the format 'YYYY-MM-DD' when the first handoff should happen. + -- It is a string as handoffs are restricted to happen only once per day + first_handoff date NOT NULL, + + -- Set to the actual time of the first handoff. + -- If this is in the past during creation of the rotation, it is set to the creation time. + -- Used by Web to avoid showing shifts that never happened + actual_handoff bigint NOT NULL, + + -- each schedule can only have one rotation with a given priority starting at a given date + UNIQUE (schedule_id, priority, first_handoff), + + CONSTRAINT pk_rotation PRIMARY KEY (id) +); + +ALTER TABLE rule DROP COLUMN timeperiod_id; + +DROP TABLE IF EXISTS schedule_member; +DROP TABLE IF EXISTS rotation_member; + +DROP TABLE IF EXISTS timeperiod_entry; + +DROP TABLE timeperiod; +CREATE TABLE timeperiod ( + id bigserial, + owned_by_rotation_id bigint REFERENCES rotation(id), -- nullable for future standalone timeperiods + + CONSTRAINT pk_timeperiod PRIMARY KEY (id) +); + +CREATE TABLE rotation_member ( + id bigserial, + rotation_id bigint NOT NULL REFERENCES rotation(id), + contact_id bigint REFERENCES contact(id), + contactgroup_id bigint REFERENCES contactgroup(id), + position integer NOT NULL, + + UNIQUE (rotation_id, position), -- each position in a rotation can only be used once + + -- Two UNIQUE constraints prevent duplicate memberships of the same contact or contactgroup in a single rotation. + -- Multiple NULLs are not considered to be duplicates, so rows with a contact_id but no contactgroup_id are + -- basically ignored in the UNIQUE constraint over contactgroup_id and vice versa. The CHECK constraint below + -- ensures that each row has only non-NULL values in one of these constraints. + UNIQUE (rotation_id, contact_id), + UNIQUE (rotation_id, contactgroup_id), + CHECK (num_nonnulls(contact_id, contactgroup_id) = 1), + + CONSTRAINT pk_rotation_member PRIMARY KEY (id) +); + +CREATE TABLE timeperiod_entry ( + id bigserial, + timeperiod_id bigint NOT NULL REFERENCES timeperiod(id), + rotation_member_id bigint REFERENCES rotation_member(id), -- nullable for future standalone timeperiods + start_time bigint NOT NULL, + end_time bigint NOT NULL, + -- Is needed by icinga-notifications-web to prefilter entries, which matches until this time and should be ignored by the daemon. + until_time bigint, + timezone text NOT NULL, -- e.g. 'Europe/Berlin', relevant for evaluating rrule (DST changes differ between zones) + rrule text, -- recurrence rule (RFC5545) + + CONSTRAINT pk_timeperiod_entry PRIMARY KEY (id) +); + +ALTER TABLE rule ADD COLUMN timeperiod_id bigint REFERENCES timeperiod(id); + +DO $$ BEGIN + DROP TYPE frequency_type; +EXCEPTION + WHEN undefined_object THEN null; +END $$; From b6f524ef7c929961f46ae0369c1a1a5cd993b0b8 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Tue, 28 May 2024 13:27:45 +0200 Subject: [PATCH 4/8] Simplify schema upgrade for schedule rotations Reduce the schema file to the minimal changes that are needed to go from the old to the new version. This makes it easier to follow what has been changed and also allows keeping other timeperiods (if they were manually created as it was not yet possible to create those outside of schedules using the web interface so far). Comments were removed from here as they still exist in the full schema file and the purpose of the upgrade file is not to document the schema. --- schema/pgsql/upgrades/026.sql | 77 ++++++++--------------------------- 1 file changed, 17 insertions(+), 60 deletions(-) diff --git a/schema/pgsql/upgrades/026.sql b/schema/pgsql/upgrades/026.sql index c2d73b8c..74e8b9fc 100644 --- a/schema/pgsql/upgrades/026.sql +++ b/schema/pgsql/upgrades/026.sql @@ -1,88 +1,45 @@ -DO $$ BEGIN - CREATE TYPE rotation_type AS ENUM ( '24-7', 'partial', 'multi' ); -EXCEPTION - WHEN duplicate_object THEN null; -END $$; +-- IMPORTANT: This schema upgrade removes all schedule-related configuration as it was changed in an incompatible way! -CREATE TABLE IF NOT EXISTS rotation ( +CREATE TYPE rotation_type AS ENUM ( '24-7', 'partial', 'multi' ); + +CREATE TABLE rotation ( id bigserial, schedule_id bigint NOT NULL REFERENCES schedule(id), - -- the lower the more important, starting at 0, avoids the need to re-index upon addition priority integer NOT NULL, name text NOT NULL, mode rotation_type NOT NULL, - -- JSON with rotation-specific attributes - -- Needed exclusively by Web to simplify editing and visualisation options text NOT NULL, - - -- A date in the format 'YYYY-MM-DD' when the first handoff should happen. - -- It is a string as handoffs are restricted to happen only once per day first_handoff date NOT NULL, - - -- Set to the actual time of the first handoff. - -- If this is in the past during creation of the rotation, it is set to the creation time. - -- Used by Web to avoid showing shifts that never happened actual_handoff bigint NOT NULL, - - -- each schedule can only have one rotation with a given priority starting at a given date UNIQUE (schedule_id, priority, first_handoff), - CONSTRAINT pk_rotation PRIMARY KEY (id) ); -ALTER TABLE rule DROP COLUMN timeperiod_id; - -DROP TABLE IF EXISTS schedule_member; -DROP TABLE IF EXISTS rotation_member; - -DROP TABLE IF EXISTS timeperiod_entry; - -DROP TABLE timeperiod; -CREATE TABLE timeperiod ( - id bigserial, - owned_by_rotation_id bigint REFERENCES rotation(id), -- nullable for future standalone timeperiods - - CONSTRAINT pk_timeperiod PRIMARY KEY (id) -); - CREATE TABLE rotation_member ( id bigserial, rotation_id bigint NOT NULL REFERENCES rotation(id), contact_id bigint REFERENCES contact(id), contactgroup_id bigint REFERENCES contactgroup(id), position integer NOT NULL, - - UNIQUE (rotation_id, position), -- each position in a rotation can only be used once - - -- Two UNIQUE constraints prevent duplicate memberships of the same contact or contactgroup in a single rotation. - -- Multiple NULLs are not considered to be duplicates, so rows with a contact_id but no contactgroup_id are - -- basically ignored in the UNIQUE constraint over contactgroup_id and vice versa. The CHECK constraint below - -- ensures that each row has only non-NULL values in one of these constraints. + UNIQUE (rotation_id, position), UNIQUE (rotation_id, contact_id), UNIQUE (rotation_id, contactgroup_id), CHECK (num_nonnulls(contact_id, contactgroup_id) = 1), - CONSTRAINT pk_rotation_member PRIMARY KEY (id) ); -CREATE TABLE timeperiod_entry ( - id bigserial, - timeperiod_id bigint NOT NULL REFERENCES timeperiod(id), - rotation_member_id bigint REFERENCES rotation_member(id), -- nullable for future standalone timeperiods - start_time bigint NOT NULL, - end_time bigint NOT NULL, - -- Is needed by icinga-notifications-web to prefilter entries, which matches until this time and should be ignored by the daemon. - until_time bigint, - timezone text NOT NULL, -- e.g. 'Europe/Berlin', relevant for evaluating rrule (DST changes differ between zones) - rrule text, -- recurrence rule (RFC5545) +DROP TABLE schedule_member; - CONSTRAINT pk_timeperiod_entry PRIMARY KEY (id) -); +DELETE FROM timeperiod_entry WHERE timeperiod_id IN (SELECT id FROM timeperiod WHERE owned_by_schedule_id IS NOT NULL); +DELETE FROM timeperiod WHERE owned_by_schedule_id IS NOT NULL; + +ALTER TABLE timeperiod + DROP COLUMN owned_by_schedule_id, + ADD COLUMN owned_by_rotation_id bigint REFERENCES rotation(id); -ALTER TABLE rule ADD COLUMN timeperiod_id bigint REFERENCES timeperiod(id); +ALTER TABLE timeperiod_entry + DROP COLUMN frequency, + DROP COLUMN description, + ADD COLUMN rotation_member_id bigint REFERENCES rotation_member(id); -DO $$ BEGIN - DROP TYPE frequency_type; -EXCEPTION - WHEN undefined_object THEN null; -END $$; +DROP TYPE frequency_type; From 86d8e47d3d2e551be95f26e1c2cff3f044e7ead7 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Tue, 28 May 2024 11:18:07 +0200 Subject: [PATCH 5/8] Define schedules using rotations This is a fundamental and incompatible change to how schedules are defined. Now, a schedule consists of a list of rotations that's ordered by priority. Each rotation contains multiple members where each is either a contact or a contact group. Each member is linked to some timeperiod entries which defines when this member is active in the rotation. This commit already includes code for a feature that was planned but is possible using the web interface at the moment: multiple versions of the same rotation where the handoff time defines when a given version becomes active. With this change, for the time being, the TimePeriod type itself fulfills no real purpose and the timeperiod entries are directly loaded as part of the schedule, bypassing the timeperiod loading code. However, there still is the plan to add standalone timeperiods in the future, thus the timeperiod code is kept. More context for these changes: - https://github.com/Icinga/icinga-notifications-web/issues/177 - https://github.com/Icinga/icinga-notifications/pull/193 --- internal/config/schedule.go | 135 +++++++++++++------- internal/config/timeperiod.go | 4 - internal/config/verify.go | 40 +++--- internal/recipient/rotations.go | 115 +++++++++++++++++ internal/recipient/rotations_test.go | 169 +++++++++++++++++++++++++ internal/recipient/schedule.go | 79 +++++++----- internal/timeperiod/timeperiod.go | 3 +- internal/timeperiod/timeperiod_test.go | 2 +- 8 files changed, 442 insertions(+), 105 deletions(-) create mode 100644 internal/recipient/rotations.go create mode 100644 internal/recipient/rotations_test.go diff --git a/internal/config/schedule.go b/internal/config/schedule.go index b43af647..af3c7557 100644 --- a/internal/config/schedule.go +++ b/internal/config/schedule.go @@ -3,6 +3,7 @@ package config import ( "context" "github.com/icinga/icinga-notifications/internal/recipient" + "github.com/icinga/icinga-notifications/internal/timeperiod" "github.com/jmoiron/sqlx" "go.uber.org/zap" ) @@ -27,28 +28,93 @@ func (r *RuntimeConfig) fetchSchedules(ctx context.Context, tx *sqlx.Tx) error { zap.String("name", g.Name)) } - var memberPtr *recipient.ScheduleMemberRow - stmt = r.db.BuildSelectStmt(memberPtr, memberPtr) + var rotationPtr *recipient.Rotation + stmt = r.db.BuildSelectStmt(rotationPtr, rotationPtr) r.logger.Debugf("Executing query %q", stmt) - var members []*recipient.ScheduleMemberRow + var rotations []*recipient.Rotation + if err := tx.SelectContext(ctx, &rotations, stmt); err != nil { + r.logger.Errorln(err) + return err + } + + rotationsById := make(map[int64]*recipient.Rotation) + for _, rotation := range rotations { + rotationLogger := r.logger.With(zap.Object("rotation", rotation)) + + if schedule := schedulesById[rotation.ScheduleID]; schedule == nil { + rotationLogger.Warnw("ignoring schedule rotation for unknown schedule_id") + } else { + rotationsById[rotation.ID] = rotation + schedule.Rotations = append(schedule.Rotations, rotation) + + rotationLogger.Debugw("loaded schedule rotation") + } + } + + var rotationMemberPtr *recipient.RotationMember + stmt = r.db.BuildSelectStmt(rotationMemberPtr, rotationMemberPtr) + r.logger.Debugf("Executing query %q", stmt) + + var members []*recipient.RotationMember if err := tx.SelectContext(ctx, &members, stmt); err != nil { r.logger.Errorln(err) return err } + rotationMembersById := make(map[int64]*recipient.RotationMember) for _, member := range members { - memberLogger := makeScheduleMemberLogger(r.logger.SugaredLogger, member) + memberLogger := r.logger.With(zap.Object("rotation_member", member)) - if s := schedulesById[member.ScheduleID]; s == nil { - memberLogger.Warnw("ignoring schedule member for unknown schedule_id") + if rotation := rotationsById[member.RotationID]; rotation == nil { + memberLogger.Warnw("ignoring rotation member for unknown rotation_member_id") } else { - s.MemberRows = append(s.MemberRows, member) + member.TimePeriodEntries = make(map[int64]*timeperiod.Entry) + rotation.Members = append(rotation.Members, member) + rotationMembersById[member.ID] = member - memberLogger.Debugw("member") + memberLogger.Debugw("loaded schedule rotation member") } } + var entryPtr *timeperiod.Entry + stmt = r.db.BuildSelectStmt(entryPtr, entryPtr) + " WHERE rotation_member_id IS NOT NULL" + r.logger.Debugf("Executing query %q", stmt) + + var entries []*timeperiod.Entry + if err := tx.SelectContext(ctx, &entries, stmt); err != nil { + r.logger.Errorln(err) + return err + } + + for _, entry := range entries { + var member *recipient.RotationMember + if entry.RotationMemberID.Valid { + member = rotationMembersById[entry.RotationMemberID.Int64] + } + + if member == nil { + r.logger.Warnw("ignoring entry for unknown rotation_member_id", + zap.Int64("timeperiod_entry_id", entry.ID), + zap.Int64("timeperiod_id", entry.TimePeriodID)) + continue + } + + err := entry.Init() + if err != nil { + r.logger.Warnw("ignoring time period entry", + zap.Object("entry", entry), + zap.Error(err)) + continue + } + + member.TimePeriodEntries[entry.ID] = entry + } + + for _, schedule := range schedulesById { + schedule.RefreshRotations() + } + if r.Schedules != nil { // mark no longer existing schedules for deletion for id := range r.Schedules { @@ -72,38 +138,26 @@ func (r *RuntimeConfig) applyPendingSchedules() { if pendingSchedule == nil { delete(r.Schedules, id) } else { - for _, memberRow := range pendingSchedule.MemberRows { - memberLogger := makeScheduleMemberLogger(r.logger.SugaredLogger, memberRow) - - period := r.TimePeriods[memberRow.TimePeriodID] - if period == nil { - memberLogger.Warnw("ignoring schedule member for unknown timeperiod_id") - continue - } - - var contact *recipient.Contact - if memberRow.ContactID.Valid { - contact = r.Contacts[memberRow.ContactID.Int64] - if contact == nil { - memberLogger.Warnw("ignoring schedule member for unknown contact_id") - continue + for _, rotation := range pendingSchedule.Rotations { + for _, member := range rotation.Members { + memberLogger := r.logger.With( + zap.Object("rotation", rotation), + zap.Object("rotation_member", member)) + + if member.ContactID.Valid { + member.Contact = r.Contacts[member.ContactID.Int64] + if member.Contact == nil { + memberLogger.Warnw("rotation member has an unknown contact_id") + } } - } - var group *recipient.Group - if memberRow.GroupID.Valid { - group = r.Groups[memberRow.GroupID.Int64] - if group == nil { - memberLogger.Warnw("ignoring schedule member for unknown contactgroup_id") - continue + if member.ContactGroupID.Valid { + member.ContactGroup = r.Groups[member.ContactGroupID.Int64] + if member.ContactGroup == nil { + memberLogger.Warnw("rotation member has an unknown contactgroup_id") + } } } - - pendingSchedule.Members = append(pendingSchedule.Members, &recipient.Member{ - TimePeriod: period, - Contact: contact, - ContactGroup: group, - }) } if currentSchedule := r.Schedules[id]; currentSchedule != nil { @@ -116,12 +170,3 @@ func (r *RuntimeConfig) applyPendingSchedules() { r.pending.Schedules = nil } - -func makeScheduleMemberLogger(logger *zap.SugaredLogger, member *recipient.ScheduleMemberRow) *zap.SugaredLogger { - return logger.With( - zap.Int64("schedule_id", member.ScheduleID), - zap.Int64("timeperiod_id", member.TimePeriodID), - zap.Int64("contact_id", member.ContactID.Int64), - zap.Int64("contactgroup_id", member.GroupID.Int64), - ) -} diff --git a/internal/config/timeperiod.go b/internal/config/timeperiod.go index 9ed7274e..9263c52d 100644 --- a/internal/config/timeperiod.go +++ b/internal/config/timeperiod.go @@ -3,7 +3,6 @@ package config import ( "context" "fmt" - "github.com/icinga/icinga-go-library/types" "github.com/icinga/icinga-notifications/internal/timeperiod" "github.com/jmoiron/sqlx" "go.uber.org/zap" @@ -45,9 +44,6 @@ func (r *RuntimeConfig) fetchTimePeriods(ctx context.Context, tx *sqlx.Tx) error if p.Name == "" { p.Name = fmt.Sprintf("Time Period #%d", entry.TimePeriodID) - if entry.Description.Valid { - p.Name += fmt.Sprintf(" (%s)", entry.Description.String) - } } err := entry.Init() diff --git a/internal/config/verify.go b/internal/config/verify.go index d6b7bf41..6f1107d3 100644 --- a/internal/config/verify.go +++ b/internal/config/verify.go @@ -199,34 +199,28 @@ func (r *RuntimeConfig) debugVerifySchedule(id int64, schedule *recipient.Schedu return fmt.Errorf("schedule %p is inconsistent with RuntimeConfig.Schedules[%d] = %p", schedule, id, other) } - for i, member := range schedule.Members { - if member == nil { - return fmt.Errorf("Members[%d] is nil", i) - } - - if member.TimePeriod == nil { - return fmt.Errorf("Members[%d].TimePeriod is nil", i) + for i, rotation := range schedule.Rotations { + if rotation == nil { + return fmt.Errorf("Rotations[%d] is nil", i) } - if member.Contact == nil && member.ContactGroup == nil { - return fmt.Errorf("Members[%d] has neither Contact nor ContactGroup set", i) - } - - if member.Contact != nil && member.ContactGroup != nil { - return fmt.Errorf("Members[%d] has both Contact and ContactGroup set", i) - } + for j, member := range rotation.Members { + if member == nil { + return fmt.Errorf("Rotations[%d].Members[%d] is nil", i, j) + } - if member.Contact != nil { - err := r.debugVerifyContact(member.Contact.ID, member.Contact) - if err != nil { - return fmt.Errorf("Contact: %w", err) + if member.Contact != nil { + err := r.debugVerifyContact(member.ContactID.Int64, member.Contact) + if err != nil { + return fmt.Errorf("Contact: %w", err) + } } - } - if member.ContactGroup != nil { - err := r.debugVerifyGroup(member.ContactGroup.ID, member.ContactGroup) - if err != nil { - return fmt.Errorf("ContactGroup: %w", err) + if member.ContactGroup != nil { + err := r.debugVerifyGroup(member.ContactGroupID.Int64, member.ContactGroup) + if err != nil { + return fmt.Errorf("ContactGroup: %w", err) + } } } } diff --git a/internal/recipient/rotations.go b/internal/recipient/rotations.go new file mode 100644 index 00000000..ea28da6f --- /dev/null +++ b/internal/recipient/rotations.go @@ -0,0 +1,115 @@ +package recipient + +import ( + "cmp" + "slices" + "time" +) + +// rotationResolver stores all the rotations from a scheduled in a structured way that's suitable for evaluating them. +type rotationResolver struct { + // sortedByPriority is ordered so that the elements at a smaller index have higher precedence. + sortedByPriority []*rotationsWithPriority +} + +// rotationsWithPriority stores the different versions of the rotations with the same priority within a single schedule. +type rotationsWithPriority struct { + priority int32 + + // sortedByHandoff contains the different version of a specific rotation sorted by their ActualHandoff time. + // This allows using binary search to find the active version. + sortedByHandoff []*Rotation +} + +// update initializes the rotationResolver with the given rotations, resetting any previously existing state. +func (r *rotationResolver) update(rotations []*Rotation) { + // Group sortedByHandoff by priority using a temporary map with the priority as key. + prioMap := make(map[int32]*rotationsWithPriority) + for _, rotation := range rotations { + p := prioMap[rotation.Priority] + if p == nil { + p = &rotationsWithPriority{ + priority: rotation.Priority, + } + prioMap[rotation.Priority] = p + } + + p.sortedByHandoff = append(p.sortedByHandoff, rotation) + } + + // Copy it to a slice and sort it by priority so that these can easily be iterated by priority. + rs := make([]*rotationsWithPriority, 0, len(prioMap)) + for _, rotation := range prioMap { + rs = append(rs, rotation) + } + slices.SortFunc(rs, func(a, b *rotationsWithPriority) int { + return cmp.Compare(a.priority, b.priority) + }) + + // Sort the different versions of the same rotation (i.e. same schedule and priority, differing in their handoff + // time) by the handoff time so that the currently active version can be found with binary search. + for _, rotation := range rs { + slices.SortFunc(rotation.sortedByHandoff, func(a, b *Rotation) int { + return a.ActualHandoff.Time().Compare(b.ActualHandoff.Time()) + }) + } + + r.sortedByPriority = rs +} + +// getRotationsAt returns a slice of active rotations at the given time. +// +// For priority, there may be at most one active rotation version. This function return all rotation versions that +// are active at the given time t, ordered by priority (lower index has higher precedence). +func (r *rotationResolver) getRotationsAt(t time.Time) []*Rotation { + rotations := make([]*Rotation, 0, len(r.sortedByPriority)) + + for _, w := range r.sortedByPriority { + i, found := slices.BinarySearchFunc(w.sortedByHandoff, t, func(rotation *Rotation, t time.Time) int { + return rotation.ActualHandoff.Time().Compare(t) + }) + + // If a rotation version with sortedByHandoff[i].ActualHandoff == t is found, it just became valid and should be + // used. Otherwise, BinarySearchFunc returns the first index i after t so that: + // + // sortedByHandoff[i-1].ActualHandoff < t < sortedByHandoff[i].ActualHandoff + // + // Thus, the version at index i becomes active after t and the preceding one is still active. + if !found { + i-- + } + + // If all rotation versions have ActualHandoff > t, there is none that's currently active and i is negative. + if i >= 0 { + rotations = append(rotations, w.sortedByHandoff[i]) + } + } + + return rotations +} + +// getContactsAt evaluates the rotations by priority and returns all contacts active at the given time. +func (r *rotationResolver) getContactsAt(t time.Time) []*Contact { + rotations := r.getRotationsAt(t) + for _, rotation := range rotations { + for _, member := range rotation.Members { + for _, entry := range member.TimePeriodEntries { + if entry.Contains(t) { + var contacts []*Contact + + if member.Contact != nil { + contacts = append(contacts, member.Contact) + } + + if member.ContactGroup != nil { + contacts = append(contacts, member.ContactGroup.Members...) + } + + return contacts + } + } + } + } + + return nil +} diff --git a/internal/recipient/rotations_test.go b/internal/recipient/rotations_test.go new file mode 100644 index 00000000..bc139fe8 --- /dev/null +++ b/internal/recipient/rotations_test.go @@ -0,0 +1,169 @@ +package recipient + +import ( + "database/sql" + "github.com/icinga/icinga-go-library/types" + "github.com/icinga/icinga-notifications/internal/timeperiod" + "github.com/stretchr/testify/assert" + "testing" + "time" +) + +func Test_rotationResolver_getCurrentRotations(t *testing.T) { + contactWeekday := &Contact{FullName: "Weekday Non-Noon"} + contactWeekdayNoon := &Contact{FullName: "Weekday Noon"} + contactWeekend2024a := &Contact{FullName: "Weekend 2024 A"} + contactWeekend2024b := &Contact{FullName: "Weekend 2024 B"} + contactWeekend2025a := &Contact{FullName: "Weekend 2025 A"} + contactWeekend2025b := &Contact{FullName: "Weekend 2025 B"} + + // Helper function to parse strings into time.Time interpreted as UTC. + // Accepts values like "2006-01-02 15:04:05" and "2006-01-02" (assuming 00:00:00 as time). + parse := func(s string) time.Time { + var format string + + switch len(s) { + case len(time.DateTime): + format = time.DateTime + case len(time.DateOnly): + format = time.DateOnly + } + + t, err := time.ParseInLocation(format, s, time.UTC) + if err != nil { + panic(err) + } + return t + } + + var s rotationResolver + s.update([]*Rotation{ + // Weekend rotation starting 2024, alternating between contacts contactWeekend2024a and contactWeekend2024b + { + ActualHandoff: types.UnixMilli(parse("2024-01-01")), + Priority: 0, + Members: []*RotationMember{ + { + Contact: contactWeekend2024a, + TimePeriodEntries: map[int64]*timeperiod.Entry{ + 1: { + StartTime: types.UnixMilli(parse("2024-01-06")), // Saturday + EndTime: types.UnixMilli(parse("2024-01-07")), // Sunday + Timezone: "UTC", + RRule: sql.NullString{String: "FREQ=WEEKLY;INTERVAL=2;BYDAY=SA,SU", Valid: true}, + }, + }, + }, { + Contact: contactWeekend2024b, + TimePeriodEntries: map[int64]*timeperiod.Entry{ + 2: { + StartTime: types.UnixMilli(parse("2024-01-13")), // Saturday + EndTime: types.UnixMilli(parse("2024-01-14")), // Sunday + Timezone: "UTC", + RRule: sql.NullString{String: "FREQ=WEEKLY;INTERVAL=2;BYDAY=SA,SU", Valid: true}, + }, + }, + }, + }, + }, + + // Weekend rotation starting 2025 and replacing the previous one, + // alternating between contacts contactWeekend2025a and contactWeekend2025b + { + ActualHandoff: types.UnixMilli(parse("2025-01-01")), + Priority: 0, + Members: []*RotationMember{ + { + Contact: contactWeekend2025a, + TimePeriodEntries: map[int64]*timeperiod.Entry{ + 3: { + StartTime: types.UnixMilli(parse("2025-01-04")), // Saturday + EndTime: types.UnixMilli(parse("2025-01-05")), // Sunday + Timezone: "UTC", + RRule: sql.NullString{String: "FREQ=WEEKLY;INTERVAL=2;BYDAY=SA,SU", Valid: true}, + }, + }, + }, { + Contact: contactWeekend2025b, + TimePeriodEntries: map[int64]*timeperiod.Entry{ + 4: { + StartTime: types.UnixMilli(parse("2025-01-11")), // Saturday + EndTime: types.UnixMilli(parse("2025-01-12")), // Sunday + Timezone: "UTC", + RRule: sql.NullString{String: "FREQ=WEEKLY;INTERVAL=2;BYDAY=SA,SU", Valid: true}, + }, + }, + }, + }, + }, + + // Weekday rotations starting 2024, one for contactWeekday every day from 8 to 20 o'clock, + // with an override for 12 to 14 o'clock with contactWeekdayNoon. + { + ActualHandoff: types.UnixMilli(parse("2024-01-01")), + Priority: 1, + Members: []*RotationMember{ + { + Contact: contactWeekdayNoon, + TimePeriodEntries: map[int64]*timeperiod.Entry{ + 5: { + StartTime: types.UnixMilli(parse("2024-01-01 12:00:00")), // Monday + EndTime: types.UnixMilli(parse("2024-01-01 14:00:00")), // Monday + Timezone: "UTC", + RRule: sql.NullString{String: "FREQ=WEEKLY;BYDAY=MO,TU,WE,TH,FR", Valid: true}, + }, + }, + }, + }, + }, { + ActualHandoff: types.UnixMilli(parse("2024-01-01")), + Priority: 2, + Members: []*RotationMember{ + { + Contact: contactWeekday, + TimePeriodEntries: map[int64]*timeperiod.Entry{ + 6: { + StartTime: types.UnixMilli(parse("2024-01-01 08:00:00")), // Monday + EndTime: types.UnixMilli(parse("2024-01-01 20:00:00")), // Monday + Timezone: "UTC", + RRule: sql.NullString{String: "FREQ=WEEKLY;BYDAY=MO,TU,WE,TH,FR", Valid: true}, + }, + }, + }, + }, + }, + }) + + for ts := parse("2023-01-01"); ts.Before(parse("2027-01-01")); ts = ts.Add(30 * time.Minute) { + got := s.getContactsAt(ts) + + switch ts.Weekday() { + case time.Monday, time.Tuesday, time.Wednesday, time.Thursday, time.Friday: + if y, h := ts.Year(), ts.Hour(); y >= 2024 && 12 <= h && h < 14 { + if assert.Lenf(t, got, 1, "resolving rotations on %v should return one contact", ts) { + assert.Equal(t, contactWeekdayNoon, got[0]) + } + } else if y >= 2024 && 8 <= h && h < 20 { + if assert.Lenf(t, got, 1, "resolving rotations on %v should return one contact", ts) { + assert.Equal(t, contactWeekday, got[0]) + } + } else { + assert.Emptyf(t, got, "resolving rotations on %v should return no contacts", ts) + } + + case time.Saturday, time.Sunday: + switch y := ts.Year(); { + case y == 2024: + if assert.Lenf(t, got, 1, "resolving rotations on %v return one contact", ts) { + assert.Contains(t, []*Contact{contactWeekend2024a, contactWeekend2024b}, got[0]) + } + case y >= 2025: + if assert.Lenf(t, got, 1, "resolving rotations on %v return one contact", ts) { + assert.Contains(t, []*Contact{contactWeekend2025a, contactWeekend2025b}, got[0]) + } + default: + assert.Emptyf(t, got, "resolving rotations on %v should return no contacts", ts) + } + } + } +} diff --git a/internal/recipient/schedule.go b/internal/recipient/schedule.go index 24c30c2a..b3f7421c 100644 --- a/internal/recipient/schedule.go +++ b/internal/recipient/schedule.go @@ -2,51 +2,70 @@ package recipient import ( "database/sql" + "github.com/icinga/icinga-go-library/types" "github.com/icinga/icinga-notifications/internal/timeperiod" + "go.uber.org/zap/zapcore" "time" ) type Schedule struct { - ID int64 `db:"id"` - Name string `db:"name"` - Members []*Member `db:"-"` - MemberRows []*ScheduleMemberRow `db:"-"` + ID int64 `db:"id"` + Name string `db:"name"` + + Rotations []*Rotation `db:"-"` + rotationResolver rotationResolver } -type Member struct { - TimePeriod *timeperiod.TimePeriod - Contact *Contact - ContactGroup *Group +// RefreshRotations updates the internally cached rotations. +// +// This must be called after the Rotations member was updated for the change to become active. +func (s *Schedule) RefreshRotations() { + s.rotationResolver.update(s.Rotations) } -type ScheduleMemberRow struct { - ScheduleID int64 `db:"schedule_id"` - TimePeriodID int64 `db:"timeperiod_id"` - ContactID sql.NullInt64 `db:"contact_id"` - GroupID sql.NullInt64 `db:"contactgroup_id"` +type Rotation struct { + ID int64 `db:"id"` + ScheduleID int64 `db:"schedule_id"` + ActualHandoff types.UnixMilli `db:"actual_handoff"` + Priority int32 `db:"priority"` + Name string `db:"name"` + Members []*RotationMember `db:"-"` } -func (s *ScheduleMemberRow) TableName() string { - return "schedule_member" +// MarshalLogObject implements the zapcore.ObjectMarshaler interface. +func (r *Rotation) MarshalLogObject(encoder zapcore.ObjectEncoder) error { + encoder.AddInt64("id", r.ID) + encoder.AddInt64("schedule_id", r.ScheduleID) + encoder.AddInt32("priority", r.Priority) + encoder.AddString("name", r.Name) + return nil } -// GetContactsAt returns the contacts that are active in the schedule at the given time. -func (s *Schedule) GetContactsAt(t time.Time) []*Contact { - var contacts []*Contact - - for _, m := range s.Members { - if m.TimePeriod.Contains(t) { - if m.Contact != nil { - contacts = append(contacts, m.Contact) - } - - if m.ContactGroup != nil { - contacts = append(contacts, m.ContactGroup.Members...) - } - } +type RotationMember struct { + ID int64 `db:"id"` + RotationID int64 `db:"rotation_id"` + Contact *Contact `db:"-"` + ContactID sql.NullInt64 `db:"contact_id"` + ContactGroup *Group `db:"-"` + ContactGroupID sql.NullInt64 `db:"contactgroup_id"` + TimePeriodEntries map[int64]*timeperiod.Entry `db:"-"` +} + +func (r *RotationMember) MarshalLogObject(encoder zapcore.ObjectEncoder) error { + encoder.AddInt64("id", r.ID) + encoder.AddInt64("rotation_id", r.RotationID) + if r.ContactID.Valid { + encoder.AddInt64("contact_id", r.ContactID.Int64) } + if r.ContactGroupID.Valid { + encoder.AddInt64("contact_group_id", r.ContactGroupID.Int64) + } + return nil +} - return contacts +// GetContactsAt returns the contacts that are active in the schedule at the given time. +func (s *Schedule) GetContactsAt(t time.Time) []*Contact { + return s.rotationResolver.getContactsAt(t) } func (s *Schedule) String() string { diff --git a/internal/timeperiod/timeperiod.go b/internal/timeperiod/timeperiod.go index 55de311f..00dcc743 100644 --- a/internal/timeperiod/timeperiod.go +++ b/internal/timeperiod/timeperiod.go @@ -2,7 +2,7 @@ package timeperiod import ( "database/sql" - "github.com/icinga/icingadb/pkg/types" + "github.com/icinga/icinga-go-library/types" "github.com/pkg/errors" "github.com/teambition/rrule-go" "go.uber.org/zap/zapcore" @@ -64,7 +64,6 @@ type Entry struct { EndTime types.UnixMilli `db:"end_time"` Timezone string `db:"timezone"` RRule sql.NullString `db:"rrule"` // RFC5545 RRULE - Description sql.NullString `db:"description"` RotationMemberID sql.NullInt64 `db:"rotation_member_id"` initialized bool diff --git a/internal/timeperiod/timeperiod_test.go b/internal/timeperiod/timeperiod_test.go index 3a86c6d3..12c6b044 100644 --- a/internal/timeperiod/timeperiod_test.go +++ b/internal/timeperiod/timeperiod_test.go @@ -3,8 +3,8 @@ package timeperiod_test import ( "database/sql" "fmt" + "github.com/icinga/icinga-go-library/types" "github.com/icinga/icinga-notifications/internal/timeperiod" - "github.com/icinga/icingadb/pkg/types" "github.com/stretchr/testify/assert" "github.com/teambition/rrule-go" "testing" From 27c1180ed174ccc9323f704d1e440779e5539e43 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Mon, 3 Jun 2024 13:06:32 +0200 Subject: [PATCH 6/8] Require manually calling Init() for timeperiod entries Lazily initializing the timeperiod entries comes with problems regarding the error handling and would also result in race conditions if multiple callers use the object at the same time. This commit changes this so that Init() has to be called explicitly first, allowing proper error handling and read-only use of the object later. --- internal/recipient/rotations_test.go | 17 ++++++++++++++--- internal/timeperiod/timeperiod.go | 15 ++++++++------- internal/timeperiod/timeperiod_test.go | 16 ++++++++++++++++ 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/internal/recipient/rotations_test.go b/internal/recipient/rotations_test.go index bc139fe8..82bf3640 100644 --- a/internal/recipient/rotations_test.go +++ b/internal/recipient/rotations_test.go @@ -5,6 +5,7 @@ import ( "github.com/icinga/icinga-go-library/types" "github.com/icinga/icinga-notifications/internal/timeperiod" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "testing" "time" ) @@ -36,8 +37,7 @@ func Test_rotationResolver_getCurrentRotations(t *testing.T) { return t } - var s rotationResolver - s.update([]*Rotation{ + rotations := []*Rotation{ // Weekend rotation starting 2024, alternating between contacts contactWeekend2024a and contactWeekend2024b { ActualHandoff: types.UnixMilli(parse("2024-01-01")), @@ -132,7 +132,18 @@ func Test_rotationResolver_getCurrentRotations(t *testing.T) { }, }, }, - }) + } + + for _, r := range rotations { + for _, m := range r.Members { + for _, e := range m.TimePeriodEntries { + require.NoError(t, e.Init()) + } + } + } + + var s rotationResolver + s.update(rotations) for ts := parse("2023-01-01"); ts.Before(parse("2027-01-01")); ts = ts.Add(30 * time.Minute) { got := s.getContactsAt(ts) diff --git a/internal/timeperiod/timeperiod.go b/internal/timeperiod/timeperiod.go index 00dcc743..e5718845 100644 --- a/internal/timeperiod/timeperiod.go +++ b/internal/timeperiod/timeperiod.go @@ -6,7 +6,6 @@ import ( "github.com/pkg/errors" "github.com/teambition/rrule-go" "go.uber.org/zap/zapcore" - "log" "time" ) @@ -129,10 +128,11 @@ func (e *Entry) Init() error { } // Contains returns whether a point in time t is covered by this entry. +// +// This function may only be called after a successful call to Init(). func (e *Entry) Contains(t time.Time) bool { - err := e.Init() - if err != nil { - log.Printf("Can't initialize entry: %s", err) + if !e.initialized { + panic("timeperiod.Entry: called Contains() before Init()") } if t.Before(e.StartTime.Time()) { @@ -156,10 +156,11 @@ func (e *Entry) Contains(t time.Time) bool { // NextTransition returns the next recurrence start or end of this entry relative to the given time inclusively. // This function returns also time.Time's zero value if there is no transition that starts/ends at/after the // specified time. +// +// This function may only be called after a successful call to Init(). func (e *Entry) NextTransition(t time.Time) time.Time { - err := e.Init() - if err != nil { - log.Printf("Can't initialize entry: %s", err) + if !e.initialized { + panic("timeperiod.Entry: called NextTransition() before Init()") } if t.Before(e.StartTime.Time()) { diff --git a/internal/timeperiod/timeperiod_test.go b/internal/timeperiod/timeperiod_test.go index 12c6b044..1835dae9 100644 --- a/internal/timeperiod/timeperiod_test.go +++ b/internal/timeperiod/timeperiod_test.go @@ -6,6 +6,7 @@ import ( "github.com/icinga/icinga-go-library/types" "github.com/icinga/icinga-notifications/internal/timeperiod" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/teambition/rrule-go" "testing" "time" @@ -30,6 +31,8 @@ func TestEntry(t *testing.T) { }, } + require.NoError(t, e.Init()) + t.Run("TimeAtFirstRecurrenceStart", func(t *testing.T) { assert.True(t, e.Contains(start)) }) @@ -85,6 +88,8 @@ func TestEntry(t *testing.T) { RRule: sql.NullString{String: "FREQ=DAILY", Valid: true}, } + require.NoError(t, e.Init()) + assert.True(t, e.Contains(start)) tz := time.FixedZone("CET", 60*60) @@ -108,6 +113,8 @@ func TestEntry(t *testing.T) { RRule: sql.NullString{String: "FREQ=DAILY", Valid: true}, } + require.NoError(t, e.Init()) + t.Run("TimeAtFirstRecurrenceStart", func(t *testing.T) { assert.Equal(t, end, e.NextTransition(start)) }) @@ -137,6 +144,7 @@ func TestEntry(t *testing.T) { Timezone: berlin, RRule: sql.NullString{String: "FREQ=DAILY", Valid: true}, } + require.NoError(t, e.Init()) assert.Equal(t, end, e.NextTransition(start), "next transition should match the first recurrence end") @@ -168,6 +176,10 @@ func TestTimePeriodTransitions(t *testing.T) { }}, } + for _, e := range p.Entries { + require.NoError(t, e.Init()) + } + assert.Equal(t, end, p.NextTransition(start), "next transition should match the interval end") }) @@ -194,6 +206,10 @@ func TestTimePeriodTransitions(t *testing.T) { }, } + for _, e := range p.Entries { + require.NoError(t, e.Init()) + } + assert.Equal(t, end, p.NextTransition(start), "next transition should match the interval end") t.Run("TimeAfterFirstIntervalEnd", func(t *testing.T) { From 83678ae29fd232e3cea79dc088910969f5ffc744 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 5 Jun 2024 11:40:13 +0200 Subject: [PATCH 7/8] Add logging for recipient to contacts expansion The most relevant case where this is interesting is when a schedule expands to no contacts at the time of a notification. --- internal/incident/incident.go | 17 +++++++++++++---- internal/recipient/contact.go | 9 +++++++++ internal/recipient/group.go | 13 ++++++++++++- internal/recipient/recipient.go | 1 + internal/recipient/schedule.go | 8 ++++++++ 5 files changed, 43 insertions(+), 5 deletions(-) diff --git a/internal/incident/incident.go b/internal/incident/incident.go index e8adb81a..5fca1dfc 100644 --- a/internal/incident/incident.go +++ b/internal/incident/incident.go @@ -648,11 +648,20 @@ func (i *Incident) getRecipientsChannel(t time.Time) rule.ContactChannels { } if i.IsNotifiable(state.Role) { - for _, contact := range r.GetContactsAt(t) { - if contactChs[contact] == nil { - contactChs[contact] = make(map[int64]bool) - contactChs[contact][contact.DefaultChannelID] = true + contacts := r.GetContactsAt(t) + if len(contacts) > 0 { + i.logger.Debugw("Expanded recipient to contacts", + zap.Object("recipient", r), + zap.Objects("contacts", contacts)) + + for _, contact := range contacts { + if contactChs[contact] == nil { + contactChs[contact] = make(map[int64]bool) + contactChs[contact][contact.DefaultChannelID] = true + } } + } else { + i.logger.Warnw("Recipient expanded to no contacts", zap.Object("recipient", r)) } } } diff --git a/internal/recipient/contact.go b/internal/recipient/contact.go index e3503536..82732f1f 100644 --- a/internal/recipient/contact.go +++ b/internal/recipient/contact.go @@ -2,6 +2,7 @@ package recipient import ( "database/sql" + "go.uber.org/zap/zapcore" "time" ) @@ -21,6 +22,14 @@ func (c *Contact) GetContactsAt(t time.Time) []*Contact { return []*Contact{c} } +// MarshalLogObject implements the zapcore.ObjectMarshaler interface. +func (c *Contact) MarshalLogObject(encoder zapcore.ObjectEncoder) error { + // Use contact_id as key so that the type is explicit if logged as the Recipient interface. + encoder.AddInt64("contact_id", c.ID) + encoder.AddString("name", c.FullName) + return nil +} + var _ Recipient = (*Contact)(nil) type Address struct { diff --git a/internal/recipient/group.go b/internal/recipient/group.go index f960bf3e..243dde7b 100644 --- a/internal/recipient/group.go +++ b/internal/recipient/group.go @@ -1,6 +1,9 @@ package recipient -import "time" +import ( + "go.uber.org/zap/zapcore" + "time" +) type Group struct { ID int64 `db:"id"` @@ -21,4 +24,12 @@ func (g *Group) String() string { return g.Name } +// MarshalLogObject implements the zapcore.ObjectMarshaler interface. +func (g *Group) MarshalLogObject(encoder zapcore.ObjectEncoder) error { + // Use contact_id as key so that the type is explicit if logged as the Recipient interface. + encoder.AddInt64("group_id", g.ID) + encoder.AddString("name", g.Name) + return nil +} + var _ Recipient = (*Group)(nil) diff --git a/internal/recipient/recipient.go b/internal/recipient/recipient.go index a09f45e4..58f36866 100644 --- a/internal/recipient/recipient.go +++ b/internal/recipient/recipient.go @@ -10,6 +10,7 @@ import ( type Recipient interface { fmt.Stringer + zapcore.ObjectMarshaler GetContactsAt(t time.Time) []*Contact } diff --git a/internal/recipient/schedule.go b/internal/recipient/schedule.go index b3f7421c..d66ef027 100644 --- a/internal/recipient/schedule.go +++ b/internal/recipient/schedule.go @@ -23,6 +23,14 @@ func (s *Schedule) RefreshRotations() { s.rotationResolver.update(s.Rotations) } +// MarshalLogObject implements the zapcore.ObjectMarshaler interface. +func (s *Schedule) MarshalLogObject(encoder zapcore.ObjectEncoder) error { + // Use schedule_id as key so that the type is explicit if logged as the Recipient interface. + encoder.AddInt64("schedule_id", s.ID) + encoder.AddString("name", s.Name) + return nil +} + type Rotation struct { ID int64 `db:"id"` ScheduleID int64 `db:"schedule_id"` From d47fde41171042dd9d7f096559d02eebabea15d1 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 5 Jun 2024 11:40:31 +0200 Subject: [PATCH 8/8] Add /dump-schedules debug endpoint This allows to quickly dump all schedules and how they expand to contacts for the next two days for debugging/testing purposes. --- internal/listener/listener.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/internal/listener/listener.go b/internal/listener/listener.go index 4a3bc67b..566d9049 100644 --- a/internal/listener/listener.go +++ b/internal/listener/listener.go @@ -36,6 +36,7 @@ func NewListener(db *database.DB, runtimeConfig *config.RuntimeConfig, logs *log l.mux.HandleFunc("/process-event", l.ProcessEvent) l.mux.HandleFunc("/dump-config", l.DumpConfig) l.mux.HandleFunc("/dump-incidents", l.DumpIncidents) + l.mux.HandleFunc("/dump-schedules", l.DumpSchedules) return l } @@ -220,3 +221,32 @@ func (l *Listener) DumpIncidents(w http.ResponseWriter, r *http.Request) { enc.SetIndent("", " ") _ = enc.Encode(encodedIncidents) } + +func (l *Listener) DumpSchedules(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + w.WriteHeader(http.StatusMethodNotAllowed) + _, _ = fmt.Fprintln(w, "GET required") + return + } + + if !l.checkDebugPassword(w, r) { + return + } + + l.runtimeConfig.RLock() + defer l.runtimeConfig.RUnlock() + + for _, schedule := range l.runtimeConfig.Schedules { + fmt.Fprintf(w, "[id=%d] %q:\n", schedule.ID, schedule.Name) + + // Iterate in 30 minute steps as this is the granularity Icinga Notifications Web allows in the configuration. + // Truncation to seconds happens only for a more readable output. + step := 30 * time.Minute + start := time.Now().Truncate(time.Second) + for t := start; t.Before(start.Add(48 * time.Hour)); t = t.Add(step) { + fmt.Fprintf(w, "\t%v: %v\n", t, schedule.GetContactsAt(t)) + } + + fmt.Fprintln(w) + } +}