Skip to content

Fix job log resolution to skip legacy logs in favour of normal/new ones #10769

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

Merged
williammartin merged 4 commits into from
Apr 11, 2025
Merged

Fix job log resolution to skip legacy logs in favour of normal/new ones #10769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
940bd10
Prefer normal job run log file over legacy one
babakks Apr 11, 2025
2d21b46
Test normal job run log is preferred over legacy one
babakks Apr 11, 2025
b652861
Add `$` anchor to log file regexps
babakks Apr 11, 2025
f337ce9
Explain job log resolution reason
babakks Apr 11, 2025
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
Binary file modified pkg/cmd/run/view/fixtures/run_log.zip
View file
Open in desktop
Binary file not shown.
45 changes: 30 additions & 15 deletions pkg/cmd/run/view/view.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,15 +551,25 @@ func getJobNameForLogFilename(name string) string {
return sanitizedJobName
}

// A job run log file is a top-level .txt file whose name starts with an ordinal
// number; e.g., "0_jobname.txt".
func jobLogFilenameRegexp(job shared.Job) *regexp.Regexp {
sanitizedJobName := getJobNameForLogFilename(job.Name)
re := fmt.Sprintf(`^-?\d+_%s\.txt`, regexp.QuoteMeta(sanitizedJobName))
re := fmt.Sprintf(`^\d+_%s\.txt$`, regexp.QuoteMeta(sanitizedJobName))
return regexp.MustCompile(re)
}

// A legacy job run log file is a top-level .txt file whose name starts with a
Comment thread
babakks marked this conversation as resolved.
// negative number which is the ID of the run; e.g., "-2147483648_jobname.txt".
func legacyJobLogFilenameRegexp(job shared.Job) *regexp.Regexp {
sanitizedJobName := getJobNameForLogFilename(job.Name)
re := fmt.Sprintf(`^-\d+_%s\.txt$`, regexp.QuoteMeta(sanitizedJobName))
return regexp.MustCompile(re)
}

func stepLogFilenameRegexp(job shared.Job, step shared.Step) *regexp.Regexp {
sanitizedJobName := getJobNameForLogFilename(job.Name)
re := fmt.Sprintf(`^%s\/%d_.*\.txt`, regexp.QuoteMeta(sanitizedJobName), step.Number)
re := fmt.Sprintf(`^%s\/%d_.*\.txt$`, regexp.QuoteMeta(sanitizedJobName), step.Number)
return regexp.MustCompile(re)
}

Expand Down Expand Up @@ -658,24 +668,29 @@ func truncateAsUTF16(str string, max int) string {
// where the ID can apparently be negative.
func attachRunLog(rlz *zip.Reader, jobs []shared.Job) {
for i, job := range jobs {
re := jobLogFilenameRegexp(job)
for _, file := range rlz.File {
if re.MatchString(file.Name) {
jobs[i].Log = file
break
}
// As a highest priority, we try to use the step logs first. We have seen zips that surprisingly contain
// step logs, normal job logs and legacy job logs. In this case, both job logs would be ignored. We have
// never seen a zip containing both job logs and no step logs, however, it may be possible. In that case
// let's prioritise the normal log over the legacy one.
jobLog := matchFileInZIPArchive(rlz, jobLogFilenameRegexp(job))
if jobLog == nil {
jobLog = matchFileInZIPArchive(rlz, legacyJobLogFilenameRegexp(job))
}
jobs[i].Log = jobLog
Comment on lines +675 to +679
Copy link
Copy Markdown
Member

@williammartin williammartin Apr 11, 2025

Choose a reason for hiding this comment

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

Not for this PR but when jobLog == nil after both attempts, this really feels like we should try to do:

#7643

Very annoying that things are silently not there.


for j, step := range job.Steps {
re := stepLogFilenameRegexp(job, step)
for _, file := range rlz.File {
if re.MatchString(file.Name) {
jobs[i].Steps[j].Log = file
break
}
}
jobs[i].Steps[j].Log = matchFileInZIPArchive(rlz, stepLogFilenameRegexp(job, step))
}
}
}

func matchFileInZIPArchive(zr *zip.Reader, re *regexp.Regexp) *zip.File {
for _, file := range zr.File {
if re.MatchString(file.Name) {
return file
}
}
return nil
}

func displayRunLog(w io.Writer, jobs []shared.Job, failed bool) error {
Expand Down
14 changes: 12 additions & 2 deletions pkg/cmd/run/view/view_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2039,8 +2039,9 @@ func TestViewRun(t *testing.T) {
// ├── 2_cool job with no step logs.txt
// ├── 3_sad job with no step logs.txt
// ├── -9999999999_legacy cool job with no step logs.txt
// └── -9999999999_legacy sad job with no step logs.txt

// ├── -9999999999_legacy sad job with no step logs.txt
// ├── 4_cool job with both legacy and new logs.txt
// └── -9999999999_cool job with both legacy and new logs.txt
func Test_attachRunLog(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -2149,6 +2150,15 @@ func Test_attachRunLog(t *testing.T) {
wantJobFilename: "-9999999999_legacy cool job with no step logs.txt",
wantStepMatch: false,
},
{
name: "matching job name with both normal and legacy filename",
job: shared.Job{
Name: "cool job with both legacy and new logs",
},
wantJobMatch: true,
wantJobFilename: "4_cool job with both legacy and new logs.txt",
wantStepMatch: false,
},
{
name: "one job name is a suffix of another",
job: shared.Job{
Expand Down
Loading