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.

Sign up for GitHub

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

Jump to bottom

Stop channel bug fix and reload frequency #20425

Merged
merged 2 commits into from
Jul 15, 2024
Merged

Stop channel bug fix and reload frequency #20425

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ef5158
Stop channel bug fix and reload frequency
getvictor Jul 12, 2024
72d52f1
Fix potential race condition.
getvictor Jul 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions ee/server/calendar/google_calendar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,19 +270,14 @@ func (c *GoogleCalendar) GetAndUpdateEvent(event *fleet.CalendarEvent, genBodyFn
return nil, false, err
}
gEvent, err := c.config.API.GetEvent(details.ID, details.ETag)
var deleted bool
var deleted, channelStopped bool
switch {
// http.StatusNotModified is returned sometimes, but not always, so we need to check ETag explicitly later
case googleapi.IsNotModified(err):
return event, false, nil
// http.StatusNotFound should be very rare -- Google keeps events for a while after they are deleted
case isNotFound(err):
deleted = true
// If event was deleted, we need to stop watching it
err = c.config.API.Stop(details.ChannelID, details.ResourceID)
if err != nil {
level.Warn(c.config.Logger).Log("msg", "stopping Google calendar event watch", "err", err)
}
case err != nil:
return nil, false, ctxerr.Wrap(c.config.Context, err, "retrieving Google calendar event")
}
Expand All @@ -304,6 +299,7 @@ func (c *GoogleCalendar) GetAndUpdateEvent(event *fleet.CalendarEvent, genBodyFn
level.Warn(c.config.Logger).Log("msg", "deleting Google calendar event which was changed to all-day event", "err", err)
}
deleted = true
channelStopped = true
}

var endTime *time.Time
Expand All @@ -320,6 +316,7 @@ func (c *GoogleCalendar) GetAndUpdateEvent(event *fleet.CalendarEvent, genBodyFn
level.Warn(c.config.Logger).Log("msg", "deleting Google calendar event which is in the past", "err", err)
}
deleted = true
channelStopped = true
}
}
if !deleted {
Expand All @@ -335,6 +332,7 @@ func (c *GoogleCalendar) GetAndUpdateEvent(event *fleet.CalendarEvent, genBodyFn
level.Warn(c.config.Logger).Log("msg", "deleting Google calendar event which was changed to all-day event", "err", err)
}
deleted = true
channelStopped = true
}
}
if !deleted {
Expand All @@ -350,6 +348,14 @@ func (c *GoogleCalendar) GetAndUpdateEvent(event *fleet.CalendarEvent, genBodyFn
}
}

// If event was deleted/cancelled, we need to stop watching it
if !channelStopped {
Copy link
Collaborator

@sharon-fdm sharon-fdm Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@getvictor Are you sure there are no paths where the function returns and this will not be called even if it's needed?
Maybe consider putting under defer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK as is -- I reviewed the code. I will rework the code so that we don't stop/create a new channel to watch the calendar if event changes. Maybe that will help with callback latency. That change can go into the next release/patch.

err = c.config.API.Stop(details.ChannelID, details.ResourceID)
if err != nil {
level.Warn(c.config.Logger).Log("msg", "stopping Google calendar event watch", "err", err)
}
}
Comment on lines +351 to +357
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain the change? It seems there are two paths where the watching of the event is not stopped (all day events and one that says // Delete this event to prevent confusion. This operation should be rare. ). Were any of these two scenarios causing issues?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug was when we deleted the event from Google calendar, we did not stop watching the notification channel. When event is deleted, it comes back from Google calendar with status = "cancelled". With this change, we will now be stopping the notification channel for that case.


newStartDate := calculateNewEventDate(event.StartTime)

fleetEvent, err := c.CreateEvent(newStartDate, genBodyFn)
Expand Down
4 changes: 2 additions & 2 deletions server/cron/calendar_cron.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"
"fmt"
"github.com/google/uuid"
"slices"
"sync"
"time"
Expand All @@ -17,11 +16,12 @@ import (
"github.com/go-kit/log"
kitlog "github.com/go-kit/log"
"github.com/go-kit/log/level"
"github.com/google/uuid"
)

const (
calendarConsumers = 18
reloadFrequency = 12 * time.Hour
reloadFrequency = 30 * time.Minute
)

func NewCalendarSchedule(
Expand Down
Loading