From 238dbc03470672732939fe1646ab881891cdbd41 Mon Sep 17 00:00:00 2001 From: peimingming Date: Wed, 28 Nov 2018 14:49:57 +0800 Subject: [PATCH] Add UT and review comments and issue fix (#6144) Signed-off-by: peimingming --- src/common/dao/joblog.go | 2 +- src/common/dao/joblog_test.go | 44 +++++++++++++ src/common/models/joblog.go | 2 - src/jobservice/job/impl/context.go | 2 +- src/jobservice/logger/backend/db_logger.go | 12 ++-- .../logger/backend/db_logger_test.go | 63 +++++++++++++++++++ src/jobservice/logger/entry.go | 15 ++++- src/jobservice/logger/factory.go | 4 +- src/jobservice/logger/getter/db_getter.go | 1 + src/jobservice/logger/known_loggers.go | 27 +++++++- src/jobservice/logger/known_loggers_test.go | 24 +++++++ src/jobservice/logger/sweeper/db_sweeper.go | 17 +++-- src/jobservice/pool/redis_job_wrapper.go | 5 +- 13 files changed, 199 insertions(+), 19 deletions(-) create mode 100644 src/common/dao/joblog_test.go create mode 100644 src/jobservice/logger/known_loggers_test.go diff --git a/src/common/dao/joblog.go b/src/common/dao/joblog.go index 5eb73f229..7fb0882ea 100644 --- a/src/common/dao/joblog.go +++ b/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" ) diff --git a/src/common/dao/joblog_test.go b/src/common/dao/joblog_test.go new file mode 100644 index 000000000..42d9ff851 --- /dev/null +++ b/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) +} diff --git a/src/common/models/joblog.go b/src/common/models/joblog.go index 5efbc810f..b004621ee 100644 --- a/src/common/models/joblog.go +++ b/src/common/models/joblog.go @@ -19,5 +19,3 @@ type JobLog struct { func (a *JobLog) TableName() string { return JobLogTable } - - diff --git a/src/jobservice/job/impl/context.go b/src/jobservice/job/impl/context.go index 0f7dbdb91..c47f69167 100644 --- a/src/jobservice/job/impl/context.go +++ b/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 ( diff --git a/src/jobservice/logger/backend/db_logger.go b/src/jobservice/logger/backend/db_logger.go index 3d8d82127..6887e4f82 100644 --- a/src/jobservice/logger/backend/db_logger.go +++ b/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, - bw: bw, - buffer: buffer, - key: key, + backendLogger: backendLogger, + bw: bw, + buffer: buffer, + key: key, }, nil } diff --git a/src/jobservice/logger/backend/db_logger_test.go b/src/jobservice/logger/backend/db_logger_test.go index 248094365..7b47aa48e 100644 --- a/src/jobservice/logger/backend/db_logger_test.go +++ b/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) +} diff --git a/src/jobservice/logger/entry.go b/src/jobservice/logger/entry.go index 32f700183..64c54e93a 100644 --- a/src/jobservice/logger/entry.go +++ b/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 } diff --git a/src/jobservice/logger/factory.go b/src/jobservice/logger/factory.go index 36391d372..3e16ad72f 100644 --- a/src/jobservice/logger/factory.go +++ b/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 - depth int + level, key string + depth int ) for _, op := range options { switch op.Field() { diff --git a/src/jobservice/logger/getter/db_getter.go b/src/jobservice/logger/getter/db_getter.go index e4be8f46e..42f5aa4e5 100644 --- a/src/jobservice/logger/getter/db_getter.go +++ b/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 { } diff --git a/src/jobservice/logger/known_loggers.go b/src/jobservice/logger/known_loggers.go index 7480a5476..4aae1f238 100644 --- a/src/jobservice/logger/known_loggers.go +++ b/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 +} diff --git a/src/jobservice/logger/known_loggers_test.go b/src/jobservice/logger/known_loggers_test.go new file mode 100644 index 000000000..9cf4d722c --- /dev/null +++ b/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)) +} diff --git a/src/jobservice/logger/sweeper/db_sweeper.go b/src/jobservice/logger/sweeper/db_sweeper.go index 8808b622f..763d531b6 100644 --- a/src/jobservice/logger/sweeper/db_sweeper.go +++ b/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 -} \ No newline at end of file +} diff --git a/src/jobservice/pool/redis_job_wrapper.go b/src/jobservice/pool/redis_job_wrapper.go index 052561816..2584f5567 100644 --- a/src/jobservice/pool/redis_job_wrapper.go +++ b/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) + } } }()