From 93fba1d5ab3ac841377655a9da20237ed2ab43f6 Mon Sep 17 00:00:00 2001
From: aoiasd <45024769+aoiasd@users.noreply.github.com>
Date: Tue, 17 Dec 2024 19:16:44 +0800
Subject: [PATCH] fix: access log remove new log not the old and skip rotate
 empty log (#38309)

relate: https://github.com/milvus-io/milvus/issues/38293

Signed-off-by: aoiasd <zhicheng.yue@zilliz.com>
---
 internal/proxy/accesslog/global_test.go |  4 +++-
 internal/proxy/accesslog/writer.go      |  7 +++++--
 internal/proxy/accesslog/writer_test.go | 26 +++++++++++++++++++++----
 3 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/internal/proxy/accesslog/global_test.go b/internal/proxy/accesslog/global_test.go
index 6a1715e792265..e90265c2f621c 100644
--- a/internal/proxy/accesslog/global_test.go
+++ b/internal/proxy/accesslog/global_test.go
@@ -25,6 +25,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/metadata"
 	"google.golang.org/grpc/peer"
@@ -94,7 +95,7 @@ func TestAccessLogger_DynamicEnable(t *testing.T) {
 	ok := _globalL.Write(accessInfo)
 	assert.False(t, ok)
 
-	etcdCli, _ := etcd.GetEtcdClient(
+	etcdCli, err := etcd.GetEtcdClient(
 		Params.EtcdCfg.UseEmbedEtcd.GetAsBool(),
 		Params.EtcdCfg.EtcdUseSSL.GetAsBool(),
 		Params.EtcdCfg.Endpoints.GetAsStrings(),
@@ -102,6 +103,7 @@ func TestAccessLogger_DynamicEnable(t *testing.T) {
 		Params.EtcdCfg.EtcdTLSKey.GetValue(),
 		Params.EtcdCfg.EtcdTLSCACert.GetValue(),
 		Params.EtcdCfg.EtcdTLSMinVersion.GetValue())
+	require.NoError(t, err)
 
 	// enable access log
 	ctx, cancel := context.WithCancel(context.Background())
diff --git a/internal/proxy/accesslog/writer.go b/internal/proxy/accesslog/writer.go
index 5aad0acd6df3d..6b5da438349b3 100644
--- a/internal/proxy/accesslog/writer.go
+++ b/internal/proxy/accesslog/writer.go
@@ -247,6 +247,10 @@ func (l *RotateWriter) Rotate() error {
 }
 
 func (l *RotateWriter) rotate() error {
+	if l.size == 0 {
+		return nil
+	}
+
 	if err := l.closeFile(); err != nil {
 		return err
 	}
@@ -330,7 +334,7 @@ func (l *RotateWriter) millRunOnce() error {
 	}
 
 	if l.maxBackups >= 0 && l.maxBackups < len(files) {
-		for _, f := range files[l.maxBackups:] {
+		for _, f := range files[:len(files)-l.maxBackups] {
 			errRemove := os.Remove(path.Join(l.dir(), f.fileName))
 			if err == nil && errRemove != nil {
 				err = errRemove
@@ -436,7 +440,6 @@ func (l *RotateWriter) oldLogFiles() ([]logInfo, error) {
 		}
 		if t, err := timeFromName(f.Name(), prefix, ext); err == nil {
 			logFiles = append(logFiles, logInfo{t, f.Name()})
-			continue
 		}
 	}
 
diff --git a/internal/proxy/accesslog/writer_test.go b/internal/proxy/accesslog/writer_test.go
index 36cb768d96e94..45d5425802f75 100644
--- a/internal/proxy/accesslog/writer_test.go
+++ b/internal/proxy/accesslog/writer_test.go
@@ -27,6 +27,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/milvus-io/milvus/pkg/util/paramtable"
 )
@@ -112,28 +113,41 @@ func TestRotateWriter_SizeRotate(t *testing.T) {
 	Params.Save(Params.ProxyCfg.AccessLog.RemotePath.Key, "access_log/")
 	Params.Save(Params.ProxyCfg.AccessLog.MaxSize.Key, "1")
 	defer os.RemoveAll(testPath)
+	fileNum := 5
 
 	logger, err := NewRotateWriter(&Params.ProxyCfg.AccessLog, &Params.MinioCfg)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	defer logger.handler.Clean()
 	defer logger.Close()
 
+	// return error when write text large than file maxsize
 	num := 1024 * 1024
 	text := getText(num + 1)
 	_, err = logger.Write(text)
 	assert.Error(t, err)
 
-	for i := 1; i <= 2; i++ {
+	for i := 1; i <= fileNum+1; i++ {
 		text = getText(num)
 		n, err := logger.Write(text)
 		assert.Equal(t, num, n)
 		assert.NoError(t, err)
 	}
 
+	// assert minio files
 	time.Sleep(time.Duration(1) * time.Second)
-	logfiles, err := logger.handler.listAll()
+	remoteFiles, err := logger.handler.listAll()
 	assert.NoError(t, err)
-	assert.Equal(t, 1, len(logfiles))
+	assert.Equal(t, fileNum, len(remoteFiles))
+
+	// assert local sealed files num
+	localFields, err := logger.oldLogFiles()
+	assert.NoError(t, err)
+	assert.Equal(t, fileNum, len(localFields))
+
+	// old files should in order
+	for i := 0; i < fileNum-1; i++ {
+		assert.True(t, localFields[i].timestamp.Before(localFields[i+1].timestamp))
+	}
 }
 
 func TestRotateWriter_LocalRetention(t *testing.T) {
@@ -150,9 +164,13 @@ func TestRotateWriter_LocalRetention(t *testing.T) {
 	assert.NoError(t, err)
 	defer logger.Close()
 
+	// write two sealed files
+	logger.Write([]byte("Test"))
 	logger.Rotate()
+	logger.Write([]byte("Test"))
 	logger.Rotate()
 	time.Sleep(time.Duration(1) * time.Second)
+
 	logFiles, err := logger.oldLogFiles()
 	assert.NoError(t, err)
 	assert.Equal(t, 1, len(logFiles))