Css 4824/expire audit logs #1007
Conversation
internal/jimm/audit_log.go
Outdated
| for { | ||
| select { | ||
| case <-time.After(a.calculateNextPollDuration()): | ||
| deleted, err := a.db.CleanupAuditLogs(a.ctx, a.auditLogRetentionPeriod) |
There was a problem hiding this comment.
i think it would be very useful for us to monitor the duration of cleanup each day.. could you please add metrics for this and the number of deleted audit entries?
There was a problem hiding this comment.
Will do! Will update this comment when done
There was a problem hiding this comment.
Added histogram and haven't added total number of deleted logs, because they're gone anyway?
service.go
Outdated
| if err != nil { | ||
| return nil, errors.E(op, "failed to parse audit log retention period") | ||
| } | ||
| jimm.NewAuditLogCleanupService(ctx, s.jimm.Database, period).Start() |
There was a problem hiding this comment.
hmmm.. could we then just have a single method like StartAuditLogCleanupService instead of New().Start()?
There was a problem hiding this comment.
I prefer creation of service then start honestly, it's a nice separation of concerns, constructor vs behaviour
internal/jimm/audit_log.go
Outdated
| 0, | ||
| now.Location(), | ||
| ) | ||
| return midDayUTC.Sub(now) |
There was a problem hiding this comment.
A little bit confused by this function, we take the current year,month,day and the polling hours,minutes, seconds then we subtract the current time. I'll just think on this some more
There was a problem hiding this comment.
Good spot, negative hours will break this. It needs solving. Will update comment.
internal/jimm/audit_log.go
Outdated
| // for absolute consistency within ns apart. | ||
| func (a *auditLogCleanupService) calculateNextPollDuration() time.Duration { | ||
| now := time.Now().UTC() | ||
| nineAM := time.Date(now.Year(), now.Month(), now.Day(), pollDuration.Hours, 0, 0, 0, time.UTC) |
There was a problem hiding this comment.
shouldn't this be time.Date(now.Year(), now.Month(), now.Day(), pollDuration.Hours, 0, 0, 0, time.UTC).Add(0,0,1) ... so nine am tomorrow.. not today..
There was a problem hiding this comment.
That's handled below, but we need to check if its a negative duration first then turn it absolute
There was a problem hiding this comment.
also have added teststo show it work
charms/jimm/templates/jimm.env
Outdated
| {% endif %} | ||
| JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS={{audit_retention_period}} |
internal/db/audit.go
Outdated
| // CleanupAuditLogs cleans up audit logs after the auditLogRetentionPeriodInDays, | ||
| // HARD deleting them from the database. | ||
| func (d *Database) CleanupAuditLogs(ctx context.Context, auditLogRetentionPeriodInDays int) (int64, error) { | ||
| retentionDate := time.Now().AddDate(0, 0, -(auditLogRetentionPeriodInDays)) |
There was a problem hiding this comment.
we might run into problems testing because time is calculated here.. would be better to pass in the cut-off date.. better for testability
There was a problem hiding this comment.
This function is present in both this and #1008 so please discuss and align them
internal/db/audit.go
Outdated
| // HARD deleting them from the database. | ||
| func (d *Database) CleanupAuditLogs(ctx context.Context, auditLogRetentionPeriodInDays int) (int64, error) { | ||
| retentionDate := time.Now().AddDate(0, 0, -(auditLogRetentionPeriodInDays)) | ||
| duration := time.Since(time.Now()) |
There was a problem hiding this comment.
this doesn't work.. instead
start := time.Now()
internal/db/audit.go
Outdated
| Unscoped(). | ||
| Where("time < ?", retentionDate). | ||
| Delete(&dbmodel.AuditLogEntry{}) | ||
| servermon.QueryTimeAuditLogCleanUpHistogram.Observe(duration.Seconds()) |
There was a problem hiding this comment.
then
servermon.QueryTimeAuditLogCleanUpHistogram.Observe(time.Since(start))
Done that now |
| if nineAMDuration < 0 { | ||
| // Add 24 hours, flip it to an absolute duration, i.e., -10h == 10h | ||
| // and subtract it from 24 hours to calculate 9am tomorrow | ||
| d = time.Hour*24 - nineAMDuration.Abs() |
There was a problem hiding this comment.
This is the same as time.Hour*24 + nineAMDuration since we've already checked that nineAMDuration is negative.
| // and subtract it from 24 hours to calculate 9am tomorrow | ||
| d = time.Hour*24 - nineAMDuration.Abs() | ||
| } else { | ||
| d = nineAMDuration.Abs() |
There was a problem hiding this comment.
We don't need the .Abs() here because we've already checked if it's negative.
| c.Assert(logs, qt.HasLen, 3) | ||
|
|
||
| jimm.PollDuration.Hours = now.Hour() | ||
| jimm.PollDuration.Minutes = now.Minute() |
There was a problem hiding this comment.
Setting these values isn't doing anything since we don't use them in the function.
Description
Cleans up audit logs by a configuration option set in DAYS. Hardcoded the UTC timezone to cleanup at 9AM every day.
Engineering checklist
Check only items that apply
Test instructions
Notes for code reviewers