sensu-go: Fix check subdue (broken and pulled prior to GA)

The fix

https://github.com/sensu/sensu-go/issues/1955#issuecomment-1074144897

Context

After spending some time debugging a test that only failed during certain times of day, I was finally able to isolate a test that I think behaves reliably.

The test that fails is TestIsSubdued/valid_subdue.

The reproduction I’ve created, which should hopefully fail any time of day, is here:

func TestTimeWindowWhenEarlyMorning(t *testing.T) {
	now, err := time.Parse(time.RFC1123Z, "Mon, 13 Aug 2018 5:30:00 -0900")
	if err != nil {
		t.Fatal(err)
	}
	subdue := &TimeWindowWhen{
		Days: TimeWindowDays{
			All: []*TimeWindowTimeRange{
				{
					Begin: now.Add(-time.Hour).Format(time.Kitchen),
					End:   now.Add(time.Hour).Format(time.Kitchen),
				},
			},
		},
	}
	subdued, err := subdue.InWindows(now)
	if err != nil {
		t.Fatal(err)
	}
	require.True(t, subdued)
}

Because Begin is an hour before now, and After is an hour after, the subdue should be true.

I believe the problem is that we are not consistently handling times in UTC. For instance, Check.IsSubdued does not use UTC, whereas the unit test TestIsSubdued does (to set up the test). In the test that I pasted to this issue, I have not used UTC, and observed a failure.

The InWindow method assumes that the time it receives is in UTC, so I think that something is not quite right.

What I can’t figure out is why the TestIsSubdued test does not fail every time - it should have a constant offset from UTC. Because it only fails at certain times of day, I am concerned that may be a deeper problem with the InWindow method. However, at this time, that is just conjecture.

Note that the TestIsSubdued test will never fail on CI systems, because they are on UTC. This really is a nasty bug!

About this issue

Original URL
State: closed
Created 6 years ago
Comments: 17 (15 by maintainers)

Most upvoted comments

I’d like to propose a solution to this issue, that changes how users specify subdue.

Instead of using the current TimeWindowWhen format (similar to above), I wonder if we could implement a simpler system that is easier to verify, by having a similar UX to calendaring products such as Google Calendar.

A user creates a single time window at a specific date and time. Say, August 14th, 2018 4:00 AM to August 14th, 2018 6:00 AM.
The user is presented with the option of repeating the time window. The options provided will be Daily, Weekly, Monthly, Yearly, or a selection of days of the week.

The client software will compute the duration of the intervals selected.

The data transmitted to the server will then be unambiguous. For example, here is what would be transmitted to subdue a check from Monday to Friday, from 4:00 am to 6:00 am.

{
  "start": "2018-08-14T4:00:00-08:00:00",
  "end": "2018-08-14T6:00:00-08:00:00",
  "repeat_calendar_days": [
    "monday",
    "tuesday",
    "wednesday",
    "thursday",
    "friday"
  ]
}

And here is what would be transmitted to repeat the time window every week:

{
  "start": "2018-08-14T4:00:00-08:00",
  "end": "2018-08-14T6:00:00-08:00",
  "repeat_weekly": true
}

If we send unambiguous datetimes to the server, then it doesn’t matter if the server is using UTC or another time zone - the Go time library will automatically handle the offset for us. For example: https://play.golang.org/p/xDClN-oKjWx

This is a breaking change. If we decide to move ahead with this, we should probably do it before GA. Otherwise, making this change will present additional difficulties.

This proposal also dovetails with the idea of allowing users to schedule events in Sensu with external calendaring software.

echlebek on Nov 19, 2018