Skip to content

Commit

Permalink
fix: scan for first week of existing logs (#1343)
Browse files Browse the repository at this point in the history 
## Description:
Prior to this change, if there were no logs for the current week, we'd
return saying logs did not exist even though they do - logs for prior
weeks could exist. This PR fixes this bug by scanning for the first week
of existing logs, then scanning for as many week of logs exist beyond
that, within the retention period.

## Is this change user facing?
YES
  • Loading branch information
@tedim52
tedim52 authored Sep 19, 2023
1 parent bdbfadd commit 3905782
Show file tree
Hide file tree
Showing 2 changed files with 135 additions and 17 deletions.
36 changes: 28 additions & 8 deletions ...t_implementations/persistent_volume/stream_logs_strategy/per_week_stream_logs_strategy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/sirupsen/logrus"
"golang.org/x/exp/slices"
"io"
"os"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -51,7 +52,11 @@ func (strategy *PerWeekStreamLogsStrategy) StreamLogs(
conjunctiveLogLinesFiltersWithRegex []logline.LogLineFilterWithRegex,
shouldFollowLogs bool,
) {
paths := strategy.getRetainedLogsFilePaths(fs, volume_consts.LogRetentionPeriodInWeeks, string(enclaveUuid), string(serviceUuid))
paths, err := strategy.getRetainedLogsFilePaths(fs, volume_consts.LogRetentionPeriodInWeeks, string(enclaveUuid), string(serviceUuid))
if err != nil {
streamErrChan <- stacktrace.Propagate(err, "An error occurred retrieving log file paths for service '%v' in enclave '%v'.", serviceUuid, enclaveUuid)
return
}
if len(paths) == 0 {
streamErrChan <- stacktrace.NewError(
`No logs file paths for service '%v' in enclave '%v' were found. This means either:
Expand Down Expand Up @@ -156,15 +161,30 @@ func (strategy *PerWeekStreamLogsStrategy) StreamLogs(
// - The +1 is because we retain an extra week of logs compared to what we promise to retain for safety.
// - The list of file paths is returned in order of oldest logs to most recent logs e.g. [ 3/80124/1234.json, /4/801234/1234.json, ...]
// - If a file path does not exist, the function with exits and returns whatever file paths were found
func (strategy *PerWeekStreamLogsStrategy) getRetainedLogsFilePaths(
filesystem volume_filesystem.VolumeFilesystem,
retentionPeriodInWeeks int,
enclaveUuid, serviceUuid string) []string {
func (strategy *PerWeekStreamLogsStrategy) getRetainedLogsFilePaths(filesystem volume_filesystem.VolumeFilesystem, retentionPeriodInWeeks int, enclaveUuid, serviceUuid string) ([]string, error) {
var paths []string
currentTime := strategy.time.Now()

// get log file paths as far back as they exist
// scan for first existing log file
firstWeekWithLogs := 0
for i := 0; i < (retentionPeriodInWeeks + 1); i++ {
year, week := strategy.time.Now().Add(time.Duration(-i) * oneWeek).ISOWeek()
year, week := currentTime.Add(time.Duration(-i) * oneWeek).ISOWeek()
filePathStr := fmt.Sprintf(volume_consts.PerWeekFilePathFmtStr, volume_consts.LogsStorageDirpath, strconv.Itoa(year), strconv.Itoa(week), enclaveUuid, serviceUuid, volume_consts.Filetype)
if _, err := filesystem.Stat(filePathStr); err == nil {
paths = append(paths, filePathStr)
firstWeekWithLogs = i
break
} else {
// return if error is not due to nonexistent file path
if !os.IsNotExist(err) {
return paths, err
}
}
}

// scan for remaining files as far back as they exist
for i := firstWeekWithLogs + 1; i < (retentionPeriodInWeeks + 1); i++ {
year, week := currentTime.Add(time.Duration(-i) * oneWeek).ISOWeek()
filePathStr := fmt.Sprintf(volume_consts.PerWeekFilePathFmtStr, volume_consts.LogsStorageDirpath, strconv.Itoa(year), strconv.Itoa(week), enclaveUuid, serviceUuid, volume_consts.Filetype)
if _, err := filesystem.Stat(filePathStr); err != nil {
break
Expand All @@ -175,7 +195,7 @@ func (strategy *PerWeekStreamLogsStrategy) getRetainedLogsFilePaths(
// reverse for oldest to most recent
slices.Reverse(paths)

return paths
return paths, nil
}

// tail -f [filepath]
Expand Down
116 changes: 107 additions & 9 deletions ...lementations/persistent_volume/stream_logs_strategy/per_week_stream_logs_strategy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,9 @@ func TestGetRetainedLogsFilePaths(t *testing.T) {

mockTime := logs_clock.NewMockLogsClock(defaultYear, currentWeek, defaultDay)
strategy := NewPerWeekStreamLogsStrategy(mockTime)
logFilePaths := strategy.getRetainedLogsFilePaths(filesystem, defaultRetentionPeriodInWeeks, testEnclaveUuid, testUserService1Uuid)
logFilePaths, err := strategy.getRetainedLogsFilePaths(filesystem, defaultRetentionPeriodInWeeks, testEnclaveUuid, testUserService1Uuid)

require.NoError(t, err)
require.Equal(t, len(expectedLogFilePaths), len(logFilePaths))
for i, filePath := range expectedLogFilePaths {
require.Equal(t, "/"+filePath, logFilePaths[i])
Expand Down Expand Up @@ -111,8 +112,57 @@ func TestGetRetainedLogsFilePathsAcrossNewYear(t *testing.T) {

mockTime := logs_clock.NewMockLogsClock(defaultYear, currentWeek, defaultDay)
strategy := NewPerWeekStreamLogsStrategy(mockTime)
logFilePaths := strategy.getRetainedLogsFilePaths(filesystem, defaultRetentionPeriodInWeeks, testEnclaveUuid, testUserService1Uuid)
logFilePaths, err := strategy.getRetainedLogsFilePaths(filesystem, defaultRetentionPeriodInWeeks, testEnclaveUuid, testUserService1Uuid)

require.NoError(t, err)
require.Equal(t, len(expectedLogFilePaths), len(logFilePaths))
for i, filePath := range expectedLogFilePaths {
require.Equal(t, "/"+filePath, logFilePaths[i])
}
}

func TestGetRetainedLogsFilePathsAcrossNewYearWith53Weeks(t *testing.T) {
// According to ISOWeek, 2015 has 53 weeks
week52filepath := getWeekFilepathStr(2015, 52)
week53filepath := getWeekFilepathStr(2015, 53)
week1filepath := getWeekFilepathStr(2016, 1)
week2filepath := getWeekFilepathStr(2016, 2)
week3filepath := getWeekFilepathStr(2016, 3)

mapFS := &fstest.MapFS{
week52filepath: {
Data: []byte{},
},
week53filepath: {
Data: []byte{},
},
week1filepath: {
Data: []byte{},
},
week2filepath: {
Data: []byte{},
},
week3filepath: {
Data: []byte{},
},
}

filesystem := volume_filesystem.NewMockedVolumeFilesystem(mapFS)
currentWeek := 3

expectedLogFilePaths := []string{
week52filepath,
week53filepath,
week1filepath,
week2filepath,
week3filepath,
}

mockTime := logs_clock.NewMockLogsClock(2016, currentWeek, 1)
strategy := NewPerWeekStreamLogsStrategy(mockTime)
logFilePaths, err := strategy.getRetainedLogsFilePaths(filesystem, defaultRetentionPeriodInWeeks, testEnclaveUuid, testUserService1Uuid)

require.NoError(t, err)
require.Equal(t, len(expectedLogFilePaths), len(logFilePaths))
for i, filePath := range expectedLogFilePaths {
require.Equal(t, "/"+filePath, logFilePaths[i])
Expand Down Expand Up @@ -149,8 +199,9 @@ func TestGetRetainedLogsFilePathsWithDiffRetentionPeriod(t *testing.T) {

mockTime := logs_clock.NewMockLogsClock(defaultYear, currentWeek, defaultDay)
strategy := NewPerWeekStreamLogsStrategy(mockTime)
logFilePaths := strategy.getRetainedLogsFilePaths(filesystem, retentionPeriod, testEnclaveUuid, testUserService1Uuid)
logFilePaths, err := strategy.getRetainedLogsFilePaths(filesystem, retentionPeriod, testEnclaveUuid, testUserService1Uuid)

require.NoError(t, err)
require.Equal(t, len(expectedLogFilePaths), len(logFilePaths))
for i, filePath := range expectedLogFilePaths {
require.Equal(t, "/"+filePath, logFilePaths[i])
Expand All @@ -159,7 +210,7 @@ func TestGetRetainedLogsFilePathsWithDiffRetentionPeriod(t *testing.T) {

func TestGetRetainedLogsFilePathsReturnsErrorIfWeeksMissing(t *testing.T) {
// ../week/enclave uuid/service uuid.json
week52filepath := getWeekFilepathStr(defaultYear, 52)
week52filepath := getWeekFilepathStr(defaultYear-1, 52)
week1filepath := getWeekFilepathStr(defaultYear, 1)
week2filepath := getWeekFilepathStr(defaultYear, 2)

Expand All @@ -175,17 +226,28 @@ func TestGetRetainedLogsFilePathsReturnsErrorIfWeeksMissing(t *testing.T) {
},
}

// should return existing file paths even though log files going all the back to retention period don't exist
expectedLogFilePaths := []string{
week52filepath,
week1filepath,
week2filepath,
}

filesystem := volume_filesystem.NewMockedVolumeFilesystem(mapFS)
currentWeek := 2

mockTime := logs_clock.NewMockLogsClock(defaultYear, currentWeek, defaultDay)
strategy := NewPerWeekStreamLogsStrategy(mockTime)
logFilePaths := strategy.getRetainedLogsFilePaths(filesystem, defaultRetentionPeriodInWeeks, testEnclaveUuid, testUserService1Uuid)
logFilePaths, err := strategy.getRetainedLogsFilePaths(filesystem, defaultRetentionPeriodInWeeks, testEnclaveUuid, testUserService1Uuid)

require.NoError(t, err)
require.Less(t, len(logFilePaths), defaultRetentionPeriodInWeeks)
for i, filePath := range expectedLogFilePaths {
require.Equal(t, "/"+filePath, logFilePaths[i])
}
}

func TestGetRetainedLogsFilePathsReturnsCorrectPathsIfWeeksMissing(t *testing.T) {
func TestGetRetainedLogsFilePathsReturnsCorrectPathsIfWeeksMissingInBetween(t *testing.T) {
// ../week/enclave uuid/service uuid.json
week52filepath := getWeekFilepathStr(defaultYear, 0)
week1filepath := getWeekFilepathStr(defaultYear, 1)
Expand All @@ -206,13 +268,49 @@ func TestGetRetainedLogsFilePathsReturnsCorrectPathsIfWeeksMissing(t *testing.T)
filesystem := volume_filesystem.NewMockedVolumeFilesystem(mapFS)
currentWeek := 3

// should only return week 3
mockTime := logs_clock.NewMockLogsClock(defaultYear, currentWeek, defaultDay)
strategy := NewPerWeekStreamLogsStrategy(mockTime)
logFilePaths := strategy.getRetainedLogsFilePaths(filesystem, defaultRetentionPeriodInWeeks, testEnclaveUuid, testUserService1Uuid)
logFilePaths, err := strategy.getRetainedLogsFilePaths(filesystem, defaultRetentionPeriodInWeeks, testEnclaveUuid, testUserService1Uuid)

require.NoError(t, err)
require.Len(t, logFilePaths, 1)
require.Equal(t, "/"+week3filepath, logFilePaths[0])
require.Equal(t, "/"+week3filepath, logFilePaths[0]) // should only return week 3 because week 2 is missing
}

func TestGetRetainedLogsFilePathsReturnsCorrectPathsIfCurrentWeekHasNoLogsYet(t *testing.T) {
// currently in week 3
currentWeek := 3
mockTime := logs_clock.NewMockLogsClock(defaultYear, currentWeek, defaultDay)

// ../week/enclave uuid/service uuid.json
week1filepath := getWeekFilepathStr(defaultYear, 1)
week2filepath := getWeekFilepathStr(defaultYear, 2)

// no logs for week current week exist yet
mapFS := &fstest.MapFS{
week1filepath: {
Data: []byte{},
},
week2filepath: {
Data: []byte{},
},
}

// should return week 1 and 2 logs, even though no logs for current week yet
expectedLogFilePaths := []string{
week1filepath,
week2filepath,
}

filesystem := volume_filesystem.NewMockedVolumeFilesystem(mapFS)
strategy := NewPerWeekStreamLogsStrategy(mockTime)
logFilePaths, err := strategy.getRetainedLogsFilePaths(filesystem, defaultRetentionPeriodInWeeks, testEnclaveUuid, testUserService1Uuid)

require.NoError(t, err)
require.Equal(t, len(expectedLogFilePaths), len(logFilePaths))
for i, filePath := range expectedLogFilePaths {
require.Equal(t, "/"+filePath, logFilePaths[i])
}
}

func TestIsWithinRetentionPeriod(t *testing.T) {
Expand Down

0 comments on commit 3905782

Please sign in to comment.