fix: golangci-lint error #1084

FrankYang0529 · 2024-04-25T06:57:01Z

Which issue(s) this PR fixes:

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

derekbit · 2024-04-25T10:51:15Z

pkg/dataconn/client.go

+			logrus.WithError(err).WithFields(logrus.Fields{
+				"seq": resp.Seq,
+				"id":  req.ID,
+			}).Error("Error removing pending operation")


Suggested change

}).Error("Error removing pending operation")

}).Warn("Failed to remove pending operation")

derekbit · 2024-04-25T10:51:48Z

pkg/dataconn/server.go

@@ -132,7 +132,9 @@ func (s *Server) write() {
 				Type: TypeClose,
 			}
 			//Best effort to notify client to close connection
-			s.wire.Write(msg)
+			if err := s.wire.Write(msg); err != nil {
+				logrus.WithError(err).Error("Failed to write")


Suggested change

logrus.WithError(err).Error("Failed to write")

logrus.WithError(err).Warn("Failed to write")

derekbit · 2024-04-25T10:53:33Z

pkg/frontend/rest/frontend.go

+	var err error
 	go func() {
-		http.ListenAndServe(listen, router)
+		err = http.ListenAndServe(listen, router)
 	}()
-	return nil
+	return err


How about

var err error go func() { if err := http.ListenAndServe(listen, router); err != nil { logrus.WithError(err).Error("Failed to ....") } }() return nil

derekbit · 2024-04-25T10:53:52Z

pkg/frontend/socket/frontend.go

+	go func() {
+		err = t.startSocketServerListen(rwu)
+	}()
+	return err


Same as https://github.com/longhorn/longhorn-engine/pull/1084/files#r1579274199

derekbit · 2024-04-25T10:54:32Z

pkg/replica/hash.go

+		logrus.WithError(err).WithFields(logrus.Fields{
+			"snapshot": t.SnapshotName,
+			"file":     fileLock.Path(),
+		}).Error("Failed to unlock the file lock")


Suggested change

}).Error("Failed to unlock the file lock")

}).Warn("Failed to unlock the file lock")

derekbit · 2024-04-25T10:55:05Z

pkg/replica/replica.go

@@ -911,7 +915,7 @@ func (r *Replica) createDisk(name string, userCreated bool, created string, labe
 	rollbackFuncList := []func() error{}
 	defer func() {
 		if err == nil {
-			r.rmDisk(oldHead)
+			err = r.rmDisk(oldHead)


add a warning message

derekbit · 2024-04-25T10:55:28Z

pkg/replica/rest/router.go

@@ -37,7 +38,9 @@ func NewRouter(s *Server) *mux.Router {
 	f := HandleError

 	router.Methods("GET").Path("/ping").Handler(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
-		rw.Write([]byte("pong"))
+		if _, err := rw.Write([]byte("pong")); err != nil {
+			logrus.WithError(err).Error("Failed to write response")


Suggested change

logrus.WithError(err).Error("Failed to write response")

logrus.WithError(err).Warn("Failed to write response")

derekbit · 2024-04-25T10:55:45Z

pkg/replica/rpc/dataserver.go

@@ -58,7 +58,9 @@ func (s *DataServer) listenAndServeTCP() error {

 		go func(conn net.Conn) {
 			server := dataconn.NewServer(conn, s.s)
-			server.Handle()
+			if err = server.Handle(); err != nil {
+				logrus.WithError(err).Error("failed to handle data server")


Suggested change

logrus.WithError(err).Error("failed to handle data server")

logrus.WithError(err).Warn("failed to handle data server")

derekbit · 2024-04-25T10:55:55Z

pkg/replica/rpc/dataserver.go

@@ -83,7 +85,9 @@ func (s *DataServer) listenAndServeUNIX() error {
 		logrus.Infof("New connection from: %v", conn.RemoteAddr())
 		go func(conn net.Conn) {
 			server := dataconn.NewServer(conn, s.s)
-			server.Handle()
+			if err = server.Handle(); err != nil {
+				logrus.WithError(err).Error("failed to handle data server")


Suggested change

logrus.WithError(err).Error("failed to handle data server")

logrus.WithError(err).Warn("failed to handle data server")

derekbit · 2024-04-25T10:56:12Z

pkg/sync/rpc/server.go

-	go s.completeBackupRestore()
+	go func() {
+		if completeErr := s.completeBackupRestore(); completeErr != nil {
+			logrus.WithError(completeErr).Error("Failed to complete backup restore")


Suggested change

logrus.WithError(completeErr).Error("Failed to complete backup restore")

logrus.WithError(completeErr).Warn("Failed to complete backup restore")

derekbit · 2024-04-25T10:56:23Z

pkg/sync/rpc/server.go

-	go s.purgeSnapshots()
+	go func() {
+		if purgeErr := s.purgeSnapshots(); purgeErr != nil {
+			logrus.WithError(purgeErr).Error("Failed to purge snapshots")


Suggested change

logrus.WithError(purgeErr).Error("Failed to purge snapshots")

logrus.WithError(purgeErr).Warn("Failed to purge snapshots")

derekbit · 2024-04-25T10:57:09Z

pkg/sync/rpc/server.go

+		if unlockErr := fileLock.Unlock(); unlockErr != nil {
+			logrus.WithError(unlockErr).WithFields(logrus.Fields{
+				"file": fileLock.Path(),
+			}).Error("Failed to unlock file")


Suggested change

}).Error("Failed to unlock file")

}).Warn("Failed to unlock file")

If we don't error out, using warning level message might be fine. WDYT?

derekbit · 2024-04-29T02:54:18Z

pkg/replica/hash.go

+			if err = SetSnapshotHashInfoToChecksumFile(t.SnapshotName, &xattrType.SnapshotHashInfo{
 				Method:            defaultHashMethod,
 				Checksum:          checksum,
 				ChangeTime:        changeTime,
 				LastHashedAt:      lastHashedAt,
 				SilentlyCorrupted: silentlyCorrupted,
-			})
+			}); err != nil {
+				logrus.WithError(err).Errorf("failed to set snapshot %s hash info to checksum file", t.SnapshotName)
+				t.State = ProgressStateError
+				t.Error = err.Error()
+				return
+			}

-			remain, err := t.isChangeTimeRemain(changeTime)
+			var remain bool
+			remain, err = t.isChangeTimeRemain(changeTime)
 			if !remain {
 				if err == nil {
 					err = fmt.Errorf("snapshot %v modification time is changed", t.SnapshotName)
 				}
 				// Do the best to delete the useless checksum file.
 				// The deletion failure is acceptable, because the mismatching timestamps
 				// will trigger the rehash in the next hash request.
-				DeleteSnapshotHashInfoChecksumFile(t.SnapshotName)
+				if deleteErr := DeleteSnapshotHashInfoChecksumFile(t.SnapshotName); deleteErr != nil {
+					logrus.WithError(deleteErr).Errorf("failed to delete snapshot %v hash info checksum file", t.SnapshotName)
+					t.State = ProgressStateError
+					t.Error = deleteErr.Error()
+					return
+				}


I'm thinking if we need to error out here.
The operations are best-effort.
Can we switch them to warning level messages?

Do you mean we can also remove t.Error = err.Error() for these two errors and only need to log them?

Yes, just keep logrus.WithError(deleteErr).Errorf("failed to delete snapshot %v hash info checksum file", t.SnapshotName)

Updated it. Thanks.

derekbit · 2024-04-29T02:55:51Z

Ping @shuo-wu, @PhanLe1010 and @ejweber for reviewing the changes in the data path.

FrankYang0529 · 2024-05-02T08:19:57Z

Hi @shuo-wu, @PhanLe1010 and @ejweber, may you help me review this PR? I need it, so I can keep working on longhorn/longhorn#8039. Thank you.

ejweber

Sorry for the delay @FrankYang0529. A couple of considerations.

ejweber · 2024-05-02T21:54:40Z

pkg/replica/replica.go

-			r.rmDisk(name)
+			if err := r.rmDisk(name); err != nil {
+				return err
+			}


This used to be best effort, but now we fail if we cannot complete the rmDisk. Unless we have a specific reason not to, I think we should maintain the existing behavior and use a warning.

ejweber · 2024-05-02T21:55:44Z

pkg/replica/replica.go

-			r.rmDisk(name)
+			if err := r.rmDisk(name); err != nil {
+				return err
+			}
 		}
 	}



Do you know why the below lines aren't flagged? It seems we ignore an error that os.Remove can return.

I'm not sure about this one. It's weird. I add logging warning message for os.Remove.

ejweber · 2024-05-02T22:36:46Z

app/cmd/controller.go

-	control.StartGRPCServer()
+	if err = control.StartGRPCServer(); err != nil {
+		return err
+	}


I think this change does not allow us to execute control.Shutdown in the event of a failure in control.StartGRPCServer. We return instead of entering control.WaitForShutdown, which would normally wait for the controller to intercept a signal and execute the registered control.Shutdown function. I think it's likely @shuo-wu intended to fall through to control.WaitForShutdown, even if control.StartGRPCServer failed (though it's weird we return, but do not log a couple of errors). Do you agree?

It looks like it would be ok to either:

Maintain existing behavior by logging warnings in control.StartGRPCServer instead of allowing it to return an error, or

Maintain existing behavior by logging a warning if control.StartGRPCServer returns an error, or

Directly execute control.Shutdown if control.StartGRPCServer returns an error.

Signed-off-by: PoAn Yang <poan.yang@suse.com>

FrankYang0529 · 2024-05-03T01:57:26Z

Hi @ejweber, thanks for the review. I change returning error to logging warning message.

ejweber

Thanks for the changes. LGTM.

derekbit

LGTM

FrankYang0529 · 2024-05-13T06:59:22Z

@mergify backport v1.6.x

mergify · 2024-05-13T06:59:38Z

backport v1.6.x

✅ Backports have been created

#1111 fix: golangci-lint error (backport #1084) has been created for branch v1.6.x

FrankYang0529 · 2024-06-14T02:37:52Z

@mergify backport v1.6.x

mergify · 2024-06-14T02:37:55Z

backport v1.6.x

✅ Backports have been created

#1111 fix: golangci-lint error (backport #1084) has been created for branch v1.6.x

FrankYang0529 requested review from innobead and derekbit April 25, 2024 08:03

derekbit reviewed Apr 25, 2024

View reviewed changes

FrankYang0529 force-pushed the LH-7448 branch from 55d34d1 to 04176ee Compare April 29, 2024 01:59

derekbit reviewed Apr 29, 2024

View reviewed changes

derekbit requested a review from shuo-wu April 29, 2024 02:55

derekbit requested review from PhanLe1010 and ejweber April 29, 2024 02:56

FrankYang0529 force-pushed the LH-7448 branch from 04176ee to e00aa3f Compare April 29, 2024 06:00

ejweber requested changes May 2, 2024

View reviewed changes

fix: golangci-lint error

3455e50

Signed-off-by: PoAn Yang <poan.yang@suse.com>

FrankYang0529 force-pushed the LH-7448 branch from e00aa3f to 3455e50 Compare May 3, 2024 01:55

ejweber approved these changes May 3, 2024

View reviewed changes

derekbit approved these changes May 3, 2024

View reviewed changes

mergify bot merged commit 737bf77 into longhorn:master May 3, 2024
6 checks passed

FrankYang0529 deleted the LH-7448 branch May 6, 2024 01:58

mergify bot mentioned this pull request May 13, 2024

fix: golangci-lint error (backport #1084) #1111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: golangci-lint error #1084

fix: golangci-lint error #1084

FrankYang0529 commented Apr 25, 2024

derekbit Apr 25, 2024 •

edited

Loading

derekbit Apr 25, 2024

derekbit Apr 25, 2024

derekbit Apr 25, 2024

derekbit Apr 25, 2024

derekbit Apr 25, 2024

derekbit Apr 25, 2024

derekbit Apr 25, 2024

derekbit Apr 25, 2024

derekbit Apr 25, 2024

derekbit Apr 25, 2024

derekbit Apr 25, 2024

derekbit Apr 29, 2024

FrankYang0529 Apr 29, 2024

derekbit Apr 29, 2024

FrankYang0529 Apr 29, 2024

derekbit commented Apr 29, 2024

FrankYang0529 commented May 2, 2024

ejweber left a comment

ejweber May 2, 2024

ejweber May 2, 2024

FrankYang0529 May 3, 2024

ejweber May 2, 2024

FrankYang0529 commented May 3, 2024

ejweber left a comment

derekbit left a comment

FrankYang0529 commented May 13, 2024

mergify bot commented May 13, 2024 •

edited

Loading

FrankYang0529 commented Jun 14, 2024

mergify bot commented Jun 14, 2024

	}).Error("Error removing pending operation")
	}).Warn("Failed to remove pending operation")

	logrus.WithError(err).Error("Failed to write")
	logrus.WithError(err).Warn("Failed to write")

	}).Error("Failed to unlock the file lock")
	}).Warn("Failed to unlock the file lock")

	logrus.WithError(err).Error("failed to handle data server")
	logrus.WithError(err).Warn("failed to handle data server")

	logrus.WithError(completeErr).Error("Failed to complete backup restore")
	logrus.WithError(completeErr).Warn("Failed to complete backup restore")

	logrus.WithError(purgeErr).Error("Failed to purge snapshots")
	logrus.WithError(purgeErr).Warn("Failed to purge snapshots")

	}).Error("Failed to unlock file")
	}).Warn("Failed to unlock file")

fix: golangci-lint error #1084

fix: golangci-lint error #1084

Conversation

FrankYang0529 commented Apr 25, 2024

Which issue(s) this PR fixes:

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

derekbit Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbit commented Apr 29, 2024

FrankYang0529 commented May 2, 2024

ejweber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankYang0529 commented May 3, 2024

ejweber left a comment

Choose a reason for hiding this comment

derekbit left a comment

Choose a reason for hiding this comment

FrankYang0529 commented May 13, 2024

mergify bot commented May 13, 2024 • edited Loading

✅ Backports have been created

FrankYang0529 commented Jun 14, 2024

mergify bot commented Jun 14, 2024

✅ Backports have been created

derekbit Apr 25, 2024 •

edited

Loading

mergify bot commented May 13, 2024 •

edited

Loading