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

@@ -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.

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")
+	taskLogInfo := api.TaskLogInfo{
-	return e.String(http.StatusOK, fullLog)
+		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 {

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()

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.
+// TaskLogSize mocks base method.
-func (m *MockLogStorage) TaskLog(arg0, arg1 string) (string, error) {
+func (m *MockLogStorage) TaskLogSize(arg0, arg1 string) (int64, error) {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "TaskLog", arg0, arg1)
+	ret := m.ctrl.Call(m, "TaskLogSize", arg0, arg1)
-	ret0, _ := ret[0].(string)
+	ret0, _ := ret[0].(int64)
 	ret1, _ := ret[1].(error)
 	return ret0, ret1
 }
 
-// TaskLog indicates an expected call of TaskLog.
+// TaskLogSize indicates an expected call of TaskLogSize.
-func (mr *MockLogStorageMockRecorder) TaskLog(arg0, arg1 interface{}) *gomock.Call {
+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.

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.
+// TaskLogSize returns the size of the task log, in bytes.
-func (s *Storage) TaskLog(jobID, taskID string) (string, error) {
+func (s *Storage) TaskLogSize(jobID, taskID string) (int64, error) {
-	filepath := s.filepath(jobID, taskID)
+	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)

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.
+	// Check the log size, it should be the entire bigString plus what was written before that.
-	contents, err = s.TaskLog(jobID, taskID)
+	size, err = s.TaskLogSize(jobID, taskID)
 	if assert.NoError(t, err) {
-		expect := "Just a single line\nAnd another line!\n" + bigString
+		expect := int64(len("Just a single line\nAnd another line!\n" + bigString))
-		assert.Equal(t, expect, contents)
+		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),

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()

web/app/src/components/jobs/TaskDetails.vue

@@ -13,7 +13,9 @@
       <dd class="field-status-label" :class="'status-' + taskData.status">{{ taskData.status }}</dd>
 
       <dt class="field-worker" title="Assigned To">Assigned To</dt>
-      <dd><worker-link :worker="taskData.worker" /></dd>
+      <dd>
+        <worker-link :worker="taskData.worker" />
+      </dd>
 
       <template v-if="taskData.failed_by_workers.length > 0">
         <dt class="field-failed-by-workers" title="Failed by Workers">Failed by Workers</dt>
@@ -52,6 +54,9 @@
         <dd>{{ cmd.parameters }}</dd>
       </template>
     </dl>
+
+    <h3 class="sub-title">Task Log</h3>
+    <button @click="openFullLog" title="Opens the task log in a new window.">Open Full Log</button>
   </template>
 
   <div v-else class="details-no-item-selected">
@@ -61,8 +66,10 @@
 
 <script lang="js">
 import * as datetime from "@/datetime";
-import * as API from '@/manager-api';
+import { JobsApi } from '@/manager-api';
+import { backendURL } from '@/urls';
 import { apiClient } from '@/stores/api-query-count';
+import { useNotifs } from "@/stores/notifications";
 import WorkerLink from '@/components/WorkerLink.vue';
 
 export default {
@@ -73,7 +80,8 @@ export default {
   data() {
     return {
       datetime: datetime, // So that the template can access it.
-      jobsApi: new API.JobsApi(apiClient),
+      jobsApi: new JobsApi(apiClient),
+      notifs: useNotifs(),
     };
   },
   mounted() {
@@ -85,6 +93,26 @@ export default {
       return !!this.taskData && !!this.taskData.id;
     },
   },
+  methods: {
+    openFullLog() {
+      const taskUUID = this.taskData.id;
+
+      this.jobsApi.fetchTaskLogInfo(taskUUID)
+        .then((logInfo) => {
+          if (logInfo == null) {
+            this.notifs.add(`Task ${taskUUID} has no log yet`)
+            return;
+          }
+          console.log(`task ${taskUUID} log info:`, logInfo);
+
+          const url = backendURL(logInfo.url);
+          window.open(url, "_blank");
+        })
+        .catch((error) => {
+          console.log(`Error fetching task ${taskUUID} log info:`, error);
+        })
+    },
+  },
 };
 </script>
 

web/app/src/urls.js

@@ -25,3 +25,10 @@ export function ws() {
 export function api() {
   return URLs.api;
 }
+
+// Backend URLs (like task logs, SwaggerUI, etc.) should be relative to the API
+// url in order to stay working when the web development server is in use.
+export function backendURL(path) {
+  const url = new URL(path, URLs.api);
+  return url.href;
+}