Add UT and review comments and issue fix (#6144)

Signed-off-by: peimingming <peimingming@corp.netease.com>

src/common/dao/joblog.go

@@ -1,8 +1,8 @@
 package dao
 
 import (
-	"github.com/goharbor/harbor/src/common/models"
 	"github.com/astaxie/beego/orm"
+	"github.com/goharbor/harbor/src/common/models"
 	"time"
 )
 

src/common/dao/joblog_test.go

@@ -0,0 +1,44 @@
+package dao
+
+import (
+	"testing"
+
+	"github.com/goharbor/harbor/src/common/models"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"time"
+)
+
+func TestMethodsOfJobLog(t *testing.T) {
+	uuid := "uuid_for_unit_test"
+	now := time.Now()
+	content := "content for unit text"
+	jobLog := &models.JobLog{
+		UUID:         uuid,
+		CreationTime: now,
+		Content:      content,
+	}
+
+	// create
+	_, err := CreateOrUpdateJobLog(jobLog)
+	require.Nil(t, err)
+
+	// update
+	updateContent := "content for unit text update"
+	jobLog.Content = updateContent
+	_, err = CreateOrUpdateJobLog(jobLog)
+	require.Nil(t, err)
+
+	// get
+	log, err := GetJobLog(uuid)
+	require.Nil(t, err)
+	assert.Equal(t, now.Second(), log.CreationTime.Second())
+	assert.Equal(t, updateContent, log.Content)
+	assert.Equal(t, jobLog.LogID, log.LogID)
+
+	// delete
+	count, err := DeleteJobLogsBefore(time.Now().Add(time.Duration(time.Minute)))
+	require.Nil(t, err)
+	assert.Equal(t, int64(1), count)
+}

src/common/models/joblog.go

@@ -19,5 +19,3 @@ type JobLog struct {
 func (a *JobLog) TableName() string {
 	return JobLogTable
 }
-
-

src/jobservice/job/impl/context.go

@@ -30,8 +30,8 @@ import (
 	"github.com/goharbor/harbor/src/jobservice/env"
 	"github.com/goharbor/harbor/src/jobservice/job"
 	"github.com/goharbor/harbor/src/jobservice/logger"
-	jmodel "github.com/goharbor/harbor/src/jobservice/models"
 	"github.com/goharbor/harbor/src/jobservice/logger/sweeper"
+	jmodel "github.com/goharbor/harbor/src/jobservice/models"
 )
 
 const (

src/jobservice/logger/backend/db_logger.go

@@ -1,11 +1,11 @@
 package backend
 
 import (
-	"github.com/goharbor/harbor/src/common/utils/log"
 	"bufio"
 	"bytes"
-	"github.com/goharbor/harbor/src/common/models"
 	"github.com/goharbor/harbor/src/common/dao"
+	"github.com/goharbor/harbor/src/common/models"
+	"github.com/goharbor/harbor/src/common/utils/log"
 )
 
 // DBLogger is an implementation of logger.Interface.
@@ -27,10 +27,10 @@ func NewDBLogger(key string, level string, depth int) (*DBLogger, error) {
 	backendLogger := log.New(bw, log.NewTextFormatter(), logLevel, depth)
 
 	return &DBLogger{
-		backendLogger:  backendLogger,
+		backendLogger: backendLogger,
-		bw:             bw,
+		bw:            bw,
-		buffer:         buffer,
+		buffer:        buffer,
-		key:            key,
+		key:           key,
 	}, nil
 }
 

src/jobservice/logger/backend/db_logger_test.go

@@ -1 +1,64 @@
 package backend
+
+import (
+	"github.com/goharbor/harbor/src/common/dao"
+	"github.com/goharbor/harbor/src/common/utils/log"
+	"github.com/goharbor/harbor/src/jobservice/logger/getter"
+	"github.com/goharbor/harbor/src/jobservice/logger/sweeper"
+	"github.com/stretchr/testify/require"
+	"os"
+	"testing"
+)
+
+func TestMain(m *testing.M) {
+
+	// databases := []string{"mysql", "sqlite"}
+	databases := []string{"postgresql"}
+	for _, database := range databases {
+		log.Infof("run test cases for database: %s", database)
+
+		result := 1
+		switch database {
+		case "postgresql":
+			dao.PrepareTestForPostgresSQL()
+		default:
+			log.Fatalf("invalid database: %s", database)
+		}
+
+		result = m.Run()
+
+		if result != 0 {
+			os.Exit(result)
+		}
+	}
+
+}
+
+// Test DB logger
+func TestDBLogger(t *testing.T) {
+	uuid := "uuid_for_unit_test"
+	l, err := NewDBLogger(uuid, "DEBUG", 4)
+	require.Nil(t, err)
+
+	l.Debug("JobLog Debug: TestDBLogger")
+	l.Info("JobLog Info: TestDBLogger")
+	l.Warning("JobLog Warning: TestDBLogger")
+	l.Error("JobLog Error: TestDBLogger")
+	l.Debugf("JobLog Debugf: %s", "TestDBLogger")
+	l.Infof("JobLog Infof: %s", "TestDBLogger")
+	l.Warningf("JobLog Warningf: %s", "TestDBLogger")
+	l.Errorf("JobLog Errorf: %s", "TestDBLogger")
+
+	l.Close()
+
+	dbGetter := getter.NewDBGetter()
+	ll, err := dbGetter.Retrieve(uuid)
+	require.Nil(t, err)
+	log.Infof("get logger %s", ll)
+
+	sweeper.PrepareDBSweep()
+	dbSweeper := sweeper.NewDBSweeper(-1)
+	count, err := dbSweeper.Sweep()
+	require.Nil(t, err)
+	require.Equal(t, 1, count)
+}

src/jobservice/logger/entry.go

@@ -1,5 +1,7 @@
 package logger
 
+import "fmt"
+
 // Entry provides unique interfaces on top of multiple logger backends.
 // Entry also implements @Interface.
 type Entry struct {
@@ -85,10 +87,21 @@ func (e *Entry) Fatalf(format string, v ...interface{}) {
 
 // Close logger
 func (e *Entry) Close() error {
+	var errMsg string
 	for _, l := range e.loggers {
 		if closer, ok := l.(Closer); ok {
-			closer.Close()
+			err := closer.Close()
+			if err != nil {
+				if errMsg == "" {
+					errMsg = fmt.Sprintf("logger: %s, err: %s", GetLoggerName(l), err)
+				} else {
+					errMsg = fmt.Sprintf("%s; logger: %s, err: %s", errMsg, GetLoggerName(l), err)
+				}
+			}
 		}
 	}
+	if errMsg != "" {
+		return fmt.Errorf(errMsg)
+	}
 	return nil
 }

src/jobservice/logger/factory.go

@@ -66,8 +66,8 @@ func StdFactory(options ...OptionItem) (Interface, error) {
 // DBFactory is factory of file logger
 func DBFactory(options ...OptionItem) (Interface, error) {
 	var (
-		level, key    string
+		level, key string
-		depth         int
+		depth      int
 	)
 	for _, op := range options {
 		switch op.Field() {

src/jobservice/logger/getter/db_getter.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"github.com/goharbor/harbor/src/common/dao"
 )
+
 // DBGetter is responsible for retrieving DB log data
 type DBGetter struct {
 }

src/jobservice/logger/known_loggers.go

@@ -1,6 +1,10 @@
 package logger
 
-import "strings"
+import (
+	"github.com/goharbor/harbor/src/jobservice/logger/backend"
+	"reflect"
+	"strings"
+)
 
 const (
 	// LoggerNameFile is unique name of the file logger.
@@ -83,3 +87,24 @@ func IsKnownLevel(level string) bool {
 
 	return false
 }
+
+// GetLoggerName return a logger name by Interface
+func GetLoggerName(l Interface) string {
+	var name string
+	if l == nil {
+		return name
+	}
+
+	switch l.(type) {
+	case *backend.DBLogger:
+		name = LoggerNameDB
+	case *backend.StdOutputLogger:
+		name = LoggerNameStdOutput
+	case *backend.FileLogger:
+		name = LoggerNameFile
+	default:
+		name = reflect.TypeOf(l).String()
+	}
+
+	return name
+}

src/jobservice/logger/known_loggers_test.go

@@ -0,0 +1,24 @@
+package logger
+
+import (
+	"github.com/goharbor/harbor/src/jobservice/logger/backend"
+	"github.com/stretchr/testify/require"
+	"os"
+	"path"
+	"testing"
+)
+
+// Test GetLoggerName
+func TestGetLoggerName(t *testing.T) {
+	uuid := "uuid_for_unit_test"
+	l, err := backend.NewDBLogger(uuid, "DEBUG", 4)
+	require.Nil(t, err)
+	require.Equal(t, LoggerNameDB, GetLoggerName(l))
+
+	stdLog := backend.NewStdOutputLogger("DEBUG", backend.StdErr, 4)
+	require.Equal(t, LoggerNameStdOutput, GetLoggerName(stdLog))
+
+	fileLog, err := backend.NewFileLogger("DEBUG", path.Join(os.TempDir(), "TestFileLogger.log"), 4)
+	require.Nil(t, err)
+	require.Equal(t, LoggerNameFile, GetLoggerName(fileLog))
+}

src/jobservice/logger/sweeper/db_sweeper.go

@@ -1,12 +1,13 @@
 package sweeper
 
 import (
-	"time"
 	"fmt"
 	"github.com/goharbor/harbor/src/common/dao"
+	"time"
 )
 
 var dbInit = make(chan int, 1)
+var isDBInit = false
 
 // DBSweeper is used to sweep the DB logs
 type DBSweeper struct {
@@ -23,7 +24,7 @@ func NewDBSweeper(duration int) *DBSweeper {
 // Sweep logs
 func (dbs *DBSweeper) Sweep() (int, error) {
 	// DB initialization not completed, waiting
-	<-dbInit
+	WaitingDBInit()
 
 	// Start to sweep logs
 	before := time.Now().Add(time.Duration(dbs.duration) * oneDay * -1)
@@ -41,8 +42,16 @@ func (dbs *DBSweeper) Duration() int {
 	return dbs.duration
 }
 
-// prepare sweeping
+// WaitingDBInit waiting DB init
+func WaitingDBInit() {
+	if !isDBInit {
+		<-dbInit
+	}
+}
+
+// PrepareDBSweep invoked after DB init
 func PrepareDBSweep() error {
+	isDBInit = true
 	dbInit <- 1
 	return nil
-}
+}

src/jobservice/pool/redis_job_wrapper.go

@@ -107,7 +107,10 @@ func (rj *RedisJob) Run(j *work.Job) error {
 	defer func() {
 		// Close open io stream first
 		if closer, ok := execContext.GetLogger().(logger.Closer); ok {
-			closer.Close()
+			err := closer.Close()
+			if err != nil {
+				logger.Errorf("Close job logger failed: %s", err)
+			}
 		}
 	}()