From 726129446dfb653de72b0cd63cfc61ab4e31e79b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Sat, 16 Jul 2022 12:55:41 +0200 Subject: [PATCH] T99730: Allow access to full task log The web interface has a button that opens the task log in a new window. This might need some restyling ;-) --- internal/manager/api_impl/interfaces.go | 3 +- internal/manager/api_impl/jobs.go | 37 ++++++++++-- internal/manager/api_impl/jobs_test.go | 60 +++++++++++++++++++ .../api_impl/mocks/api_impl_mock.gen.go | 28 ++++++--- internal/manager/task_logs/task_logs.go | 22 +++---- internal/manager/task_logs/task_logs_test.go | 18 ++++-- internal/worker/mocks/client.gen.go | 40 ++++++------- web/app/src/components/jobs/TaskDetails.vue | 54 +++++++++++++---- web/app/src/urls.js | 7 +++ 9 files changed, 205 insertions(+), 64 deletions(-) diff --git a/internal/manager/api_impl/interfaces.go b/internal/manager/api_impl/interfaces.go index 16e12a4a..6d99e6be 100644 --- a/internal/manager/api_impl/interfaces.go +++ b/internal/manager/api_impl/interfaces.go @@ -123,7 +123,8 @@ type LogStorage interface { WriteTimestamped(logger zerolog.Logger, jobID, taskID string, logText string) error RotateFile(logger zerolog.Logger, jobID, taskID string) Tail(jobID, taskID string) (string, error) - TaskLog(jobID, taskID string) (string, error) + TaskLogSize(jobID, taskID string) (int64, error) + Filepath(jobID, taskID string) string } // LastRendered processes the "last rendered" images. diff --git a/internal/manager/api_impl/jobs.go b/internal/manager/api_impl/jobs.go index af258bf8..648b6116 100644 --- a/internal/manager/api_impl/jobs.go +++ b/internal/manager/api_impl/jobs.go @@ -5,6 +5,7 @@ package api_impl import ( "errors" "fmt" + "math" "net/http" "os" "path" @@ -17,6 +18,7 @@ import ( "git.blender.org/flamenco/internal/manager/webupdates" "git.blender.org/flamenco/internal/uuid" "git.blender.org/flamenco/pkg/api" + "git.blender.org/flamenco/pkg/crosspath" ) // JobFilesURLPrefix is the URL prefix that the Flamenco API expects to serve @@ -215,13 +217,13 @@ func (f *Flamenco) SetTaskStatus(e echo.Context, taskID string) error { return e.NoContent(http.StatusNoContent) } -func (f *Flamenco) FetchTaskLog(e echo.Context, taskID string) error { +func (f *Flamenco) FetchTaskLogInfo(e echo.Context, taskID string) error { logger := requestLogger(e) ctx := e.Request().Context() logger = logger.With().Str("task", taskID).Logger() if !uuid.IsValid(taskID) { - logger.Warn().Msg("fetchTaskLog: bad task ID ") + logger.Warn().Msg("FetchTaskLogInfo: bad task ID ") return sendAPIError(e, http.StatusBadRequest, "bad task ID") } @@ -235,7 +237,7 @@ func (f *Flamenco) FetchTaskLog(e echo.Context, taskID string) error { } logger = logger.With().Str("job", dbTask.Job.UUID).Logger() - fullLog, err := f.logStorage.TaskLog(dbTask.Job.UUID, taskID) + size, err := f.logStorage.TaskLogSize(dbTask.Job.UUID, taskID) if err != nil { if errors.Is(err, os.ErrNotExist) { logger.Debug().Msg("task log unavailable, task has no log on disk") @@ -245,13 +247,36 @@ func (f *Flamenco) FetchTaskLog(e echo.Context, taskID string) error { return sendAPIError(e, http.StatusInternalServerError, "error fetching task log: %v", err) } - if fullLog == "" { + if size == 0 { logger.Debug().Msg("task log unavailable, on-disk task log is empty") return e.NoContent(http.StatusNoContent) } + if size > math.MaxInt { + // The OpenAPI definition just has type "integer", which translates to an + // 'int' in Go. + logger.Warn(). + Int64("size", size). + Int("cappedSize", math.MaxInt). + Msg("Task log is larger than can be stored in an int, capping the reported size. The log can still be entirely downloaded.") + size = math.MaxInt + } - logger.Debug().Msg("fetched task log") - return e.String(http.StatusOK, fullLog) + taskLogInfo := api.TaskLogInfo{ + TaskId: taskID, + JobId: dbTask.Job.UUID, + Size: int(size), + } + + fullLogPath := f.logStorage.Filepath(dbTask.Job.UUID, taskID) + relPath, err := f.localStorage.RelPath(fullLogPath) + if err != nil { + logger.Error().Err(err).Msg("task log is outside the manager storage, cannot construct its URL for download") + } else { + taskLogInfo.Url = path.Join(JobFilesURLPrefix, crosspath.ToSlash(relPath)) + } + + logger.Debug().Msg("fetched task log info") + return e.JSON(http.StatusOK, &taskLogInfo) } func (f *Flamenco) FetchTaskLogTail(e echo.Context, taskID string) error { diff --git a/internal/manager/api_impl/jobs_test.go b/internal/manager/api_impl/jobs_test.go index a1b7bcdf..567c88c6 100644 --- a/internal/manager/api_impl/jobs_test.go +++ b/internal/manager/api_impl/jobs_test.go @@ -315,6 +315,66 @@ func TestFetchTaskLogTail(t *testing.T) { assertResponseNoContent(t, echoCtx) } +func TestFetchTaskLogInfo(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + mf := newMockedFlamenco(mockCtrl) + + jobID := "18a9b096-d77e-438c-9be2-74397038298b" + taskID := "2e020eee-20f8-4e95-8dcf-65f7dfc3ebab" + dbJob := persistence.Job{ + UUID: jobID, + Name: "test job", + Status: api.JobStatusActive, + Settings: persistence.StringInterfaceMap{}, + Metadata: persistence.StringStringMap{}, + } + dbTask := persistence.Task{ + UUID: taskID, + Job: &dbJob, + Name: "test task", + } + mf.persistence.EXPECT(). + FetchTask(gomock.Any(), taskID). + Return(&dbTask, nil). + AnyTimes() + + // The task can be found, but has no on-disk task log. + // This should not cause any error, but instead be returned as "no content". + mf.logStorage.EXPECT().TaskLogSize(jobID, taskID). + Return(int64(0), fmt.Errorf("wrapped error: %w", os.ErrNotExist)) + + echoCtx := mf.prepareMockedRequest(nil) + err := mf.flamenco.FetchTaskLogInfo(echoCtx, taskID) + assert.NoError(t, err) + assertResponseNoContent(t, echoCtx) + + // Check that a 204 No Content is also returned when the task log file on disk exists, but is empty. + mf.logStorage.EXPECT().TaskLogSize(jobID, taskID). + Return(int64(0), fmt.Errorf("wrapped error: %w", os.ErrNotExist)) + + echoCtx = mf.prepareMockedRequest(nil) + err = mf.flamenco.FetchTaskLogInfo(echoCtx, taskID) + assert.NoError(t, err) + assertResponseNoContent(t, echoCtx) + + // Check that otherwise we actually get the info. + mf.logStorage.EXPECT().TaskLogSize(jobID, taskID).Return(int64(47), nil) + mf.logStorage.EXPECT().Filepath(jobID, taskID).Return("/path/to/job-x/test-y.txt") + mf.localStorage.EXPECT().RelPath("/path/to/job-x/test-y.txt").Return("job-x/test-y.txt", nil) + + echoCtx = mf.prepareMockedRequest(nil) + err = mf.flamenco.FetchTaskLogInfo(echoCtx, taskID) + assert.NoError(t, err) + assertResponseJSON(t, echoCtx, http.StatusOK, api.TaskLogInfo{ + JobId: jobID, + TaskId: taskID, + Size: 47, + Url: "/job-files/job-x/test-y.txt", + }) +} + func TestFetchJobLastRenderedInfo(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() diff --git a/internal/manager/api_impl/mocks/api_impl_mock.gen.go b/internal/manager/api_impl/mocks/api_impl_mock.gen.go index 3e817913..5082fca0 100644 --- a/internal/manager/api_impl/mocks/api_impl_mock.gen.go +++ b/internal/manager/api_impl/mocks/api_impl_mock.gen.go @@ -593,6 +593,20 @@ func (m *MockLogStorage) EXPECT() *MockLogStorageMockRecorder { return m.recorder } +// Filepath mocks base method. +func (m *MockLogStorage) Filepath(arg0, arg1 string) string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Filepath", arg0, arg1) + ret0, _ := ret[0].(string) + return ret0 +} + +// Filepath indicates an expected call of Filepath. +func (mr *MockLogStorageMockRecorder) Filepath(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Filepath", reflect.TypeOf((*MockLogStorage)(nil).Filepath), arg0, arg1) +} + // RotateFile mocks base method. func (m *MockLogStorage) RotateFile(arg0 zerolog.Logger, arg1, arg2 string) { m.ctrl.T.Helper() @@ -620,19 +634,19 @@ func (mr *MockLogStorageMockRecorder) Tail(arg0, arg1 interface{}) *gomock.Call return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Tail", reflect.TypeOf((*MockLogStorage)(nil).Tail), arg0, arg1) } -// TaskLog mocks base method. -func (m *MockLogStorage) TaskLog(arg0, arg1 string) (string, error) { +// TaskLogSize mocks base method. +func (m *MockLogStorage) TaskLogSize(arg0, arg1 string) (int64, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "TaskLog", arg0, arg1) - ret0, _ := ret[0].(string) + ret := m.ctrl.Call(m, "TaskLogSize", arg0, arg1) + ret0, _ := ret[0].(int64) ret1, _ := ret[1].(error) return ret0, ret1 } -// TaskLog indicates an expected call of TaskLog. -func (mr *MockLogStorageMockRecorder) TaskLog(arg0, arg1 interface{}) *gomock.Call { +// TaskLogSize indicates an expected call of TaskLogSize. +func (mr *MockLogStorageMockRecorder) TaskLogSize(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TaskLog", reflect.TypeOf((*MockLogStorage)(nil).TaskLog), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TaskLogSize", reflect.TypeOf((*MockLogStorage)(nil).TaskLogSize), arg0, arg1) } // Write mocks base method. diff --git a/internal/manager/task_logs/task_logs.go b/internal/manager/task_logs/task_logs.go index 52817a51..6e08894d 100644 --- a/internal/manager/task_logs/task_logs.go +++ b/internal/manager/task_logs/task_logs.go @@ -94,7 +94,7 @@ func (s *Storage) writeToDisk(logger zerolog.Logger, jobID, taskID string, logTe s.taskLock(taskID) defer s.taskUnlock(taskID) - filepath := s.filepath(jobID, taskID) + filepath := s.Filepath(jobID, taskID) logger = logger.With().Str("filepath", filepath).Logger() if err := os.MkdirAll(path.Dir(filepath), 0755); err != nil { @@ -135,7 +135,7 @@ func (s *Storage) writeToDisk(logger zerolog.Logger, jobID, taskID string, logTe // RotateFile rotates the task's log file, ignoring (but logging) any errors that occur. func (s *Storage) RotateFile(logger zerolog.Logger, jobID, taskID string) { - logpath := s.filepath(jobID, taskID) + logpath := s.Filepath(jobID, taskID) logger = logger.With().Str("logpath", logpath).Logger() s.taskLock(taskID) @@ -148,34 +148,34 @@ func (s *Storage) RotateFile(logger zerolog.Logger, jobID, taskID string) { } } -// filepath returns the file path suitable to write a log file. +// Filepath returns the file path suitable to write a log file. // Note that this intentionally shares the behaviour of `pathForJob()` in // `internal/manager/local_storage/local_storage.go`; it is intended that the // file handling code in this source file is migrated to use the `local_storage` // package at some point. -func (s *Storage) filepath(jobID, taskID string) string { +func (s *Storage) Filepath(jobID, taskID string) string { dirpath := s.localStorage.ForJob(jobID) filename := fmt.Sprintf("task-%v.txt", taskID) return path.Join(dirpath, filename) } -// TaskLog reads the entire log file. -func (s *Storage) TaskLog(jobID, taskID string) (string, error) { - filepath := s.filepath(jobID, taskID) +// TaskLogSize returns the size of the task log, in bytes. +func (s *Storage) TaskLogSize(jobID, taskID string) (int64, error) { + filepath := s.Filepath(jobID, taskID) s.taskLock(taskID) defer s.taskUnlock(taskID) - buffer, err := os.ReadFile(filepath) + stat, err := os.Stat(filepath) if err != nil { - return "", fmt.Errorf("reading log file of job %q task %q: %w", jobID, taskID, err) + return 0, fmt.Errorf("unable to access log file of job %q task %q: %w", jobID, taskID, err) } - return string(buffer), nil + return stat.Size(), nil } // Tail reads the final few lines of a task log. func (s *Storage) Tail(jobID, taskID string) (string, error) { - filepath := s.filepath(jobID, taskID) + filepath := s.Filepath(jobID, taskID) s.taskLock(taskID) defer s.taskUnlock(taskID) diff --git a/internal/manager/task_logs/task_logs_test.go b/internal/manager/task_logs/task_logs_test.go index 9296ba74..81399d95 100644 --- a/internal/manager/task_logs/task_logs_test.go +++ b/internal/manager/task_logs/task_logs_test.go @@ -74,7 +74,7 @@ func TestLogRotation(t *testing.T) { assert.True(t, errors.Is(err, fs.ErrNotExist)) } -func TestLogTailAndFullLog(t *testing.T) { +func TestLogTailAndSize(t *testing.T) { s, finish, mocks := taskLogsTestFixtures(t) defer finish() @@ -86,10 +86,16 @@ func TestLogTailAndFullLog(t *testing.T) { mocks.broadcaster.EXPECT().BroadcastTaskLogUpdate(gomock.Any()).Times(3) mocks.localStorage.EXPECT().ForJob(jobID).Return(jobDir).AnyTimes() + // Check tail & size of non-existent log file. contents, err := s.Tail(jobID, taskID) assert.ErrorIs(t, err, os.ErrNotExist) assert.Equal(t, "", contents) + size, err := s.TaskLogSize(jobID, taskID) + assert.ErrorIs(t, err, fs.ErrNotExist) + assert.Equal(t, int64(0), size) + + // Test a single line. err = s.Write(zerolog.Nop(), jobID, taskID, "Just a single line") assert.NoError(t, err) contents, err = s.Tail(jobID, taskID) @@ -110,11 +116,11 @@ func TestLogTailAndFullLog(t *testing.T) { err = s.Write(zerolog.Nop(), jobID, taskID, bigString) assert.NoError(t, err) - // Check the full log, it should be the entire bigString plus what was written before that. - contents, err = s.TaskLog(jobID, taskID) + // Check the log size, it should be the entire bigString plus what was written before that. + size, err = s.TaskLogSize(jobID, taskID) if assert.NoError(t, err) { - expect := "Just a single line\nAnd another line!\n" + bigString - assert.Equal(t, expect, contents) + expect := int64(len("Just a single line\nAnd another line!\n" + bigString)) + assert.Equal(t, expect, size) } // Check the tail, it should only be the few last lines of bigString. @@ -184,7 +190,7 @@ func TestLogWritingParallel(t *testing.T) { // Test that the final log contains 1000 lines of of 100 characters, without // any run getting interrupted by another one. - contents, err := os.ReadFile(s.filepath(jobID, taskID)) + contents, err := os.ReadFile(s.Filepath(jobID, taskID)) assert.NoError(t, err) lines := strings.Split(string(contents), "\n") assert.Equal(t, numGoroutines+1, len(lines), diff --git a/internal/worker/mocks/client.gen.go b/internal/worker/mocks/client.gen.go index 70070b07..76d718da 100644 --- a/internal/worker/mocks/client.gen.go +++ b/internal/worker/mocks/client.gen.go @@ -216,6 +216,26 @@ func (mr *MockFlamencoClientMockRecorder) FetchJobWithResponse(arg0, arg1 interf return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchJobWithResponse", reflect.TypeOf((*MockFlamencoClient)(nil).FetchJobWithResponse), varargs...) } +// FetchTaskLogInfoWithResponse mocks base method. +func (m *MockFlamencoClient) FetchTaskLogInfoWithResponse(arg0 context.Context, arg1 string, arg2 ...api.RequestEditorFn) (*api.FetchTaskLogInfoResponse, error) { + m.ctrl.T.Helper() + varargs := []interface{}{arg0, arg1} + for _, a := range arg2 { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "FetchTaskLogInfoWithResponse", varargs...) + ret0, _ := ret[0].(*api.FetchTaskLogInfoResponse) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FetchTaskLogInfoWithResponse indicates an expected call of FetchTaskLogInfoWithResponse. +func (mr *MockFlamencoClientMockRecorder) FetchTaskLogInfoWithResponse(arg0, arg1 interface{}, arg2 ...interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]interface{}{arg0, arg1}, arg2...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchTaskLogInfoWithResponse", reflect.TypeOf((*MockFlamencoClient)(nil).FetchTaskLogInfoWithResponse), varargs...) +} + // FetchTaskLogTailWithResponse mocks base method. func (m *MockFlamencoClient) FetchTaskLogTailWithResponse(arg0 context.Context, arg1 string, arg2 ...api.RequestEditorFn) (*api.FetchTaskLogTailResponse, error) { m.ctrl.T.Helper() @@ -236,26 +256,6 @@ func (mr *MockFlamencoClientMockRecorder) FetchTaskLogTailWithResponse(arg0, arg return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchTaskLogTailWithResponse", reflect.TypeOf((*MockFlamencoClient)(nil).FetchTaskLogTailWithResponse), varargs...) } -// FetchTaskLogWithResponse mocks base method. -func (m *MockFlamencoClient) FetchTaskLogWithResponse(arg0 context.Context, arg1 string, arg2 ...api.RequestEditorFn) (*api.FetchTaskLogResponse, error) { - m.ctrl.T.Helper() - varargs := []interface{}{arg0, arg1} - for _, a := range arg2 { - varargs = append(varargs, a) - } - ret := m.ctrl.Call(m, "FetchTaskLogWithResponse", varargs...) - ret0, _ := ret[0].(*api.FetchTaskLogResponse) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// FetchTaskLogWithResponse indicates an expected call of FetchTaskLogWithResponse. -func (mr *MockFlamencoClientMockRecorder) FetchTaskLogWithResponse(arg0, arg1 interface{}, arg2 ...interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - varargs := append([]interface{}{arg0, arg1}, arg2...) - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchTaskLogWithResponse", reflect.TypeOf((*MockFlamencoClient)(nil).FetchTaskLogWithResponse), varargs...) -} - // FetchTaskWithResponse mocks base method. func (m *MockFlamencoClient) FetchTaskWithResponse(arg0 context.Context, arg1 string, arg2 ...api.RequestEditorFn) (*api.FetchTaskResponse, error) { m.ctrl.T.Helper() diff --git a/web/app/src/components/jobs/TaskDetails.vue b/web/app/src/components/jobs/TaskDetails.vue index 60ec080d..73db3f98 100644 --- a/web/app/src/components/jobs/TaskDetails.vue +++ b/web/app/src/components/jobs/TaskDetails.vue @@ -13,20 +13,22 @@
{{ taskData.status }}
Assigned To
-
+
+ +
Priority
@@ -52,6 +54,9 @@
{{ cmd.parameters }}
+ +

Task Log

+
@@ -61,19 +66,22 @@