Add sufficient deadlines and countermeasures to handle hung node scenario (#19688)

Signed-off-by: Shubhendu Ram Tripathi <shubhendu@minio.io>
Signed-off-by: Harshavardhana <harsha@minio.io>

internal/config/drive/drive.go

@@ -25,15 +25,18 @@ import (
 	"github.com/minio/pkg/v2/env"
 )
 
+// Drive specific timeout environment variables
 const (
-	envMaxDriveTimeout = "MINIO_DRIVE_MAX_TIMEOUT"
+	EnvMaxDriveTimeout       = "MINIO_DRIVE_MAX_TIMEOUT"
+	EnvMaxDriveTimeoutLegacy = "_MINIO_DRIVE_MAX_TIMEOUT"
+	EnvMaxDiskTimeoutLegacy  = "_MINIO_DISK_MAX_TIMEOUT"
 )
 
 // DefaultKVS - default KVS for drive
 var DefaultKVS = config.KVS{
 	config.KV{
 		Key:   MaxTimeout,
-		Value: "",
+		Value: "30s",
 	},
 }
 
@@ -53,8 +56,13 @@ func (c *Config) Update(new Config) error {
 	return nil
 }
 
-// GetMaxTimeout - returns the max timeout value.
+// GetMaxTimeout - returns the per call drive operation timeout
 func (c *Config) GetMaxTimeout() time.Duration {
+	return c.GetOPTimeout()
+}
+
+// GetOPTimeout - returns the per call drive operation timeout
+func (c *Config) GetOPTimeout() time.Duration {
 	configLk.RLock()
 	defer configLk.RUnlock()
 
@@ -71,35 +79,32 @@ func LookupConfig(kvs config.KVS) (cfg Config, err error) {
 	}
 
 	// if not set. Get default value from environment
-	d := env.Get(envMaxDriveTimeout, kvs.GetWithDefault(MaxTimeout, DefaultKVS))
+	d := env.Get(EnvMaxDriveTimeout, env.Get(EnvMaxDriveTimeoutLegacy, env.Get(EnvMaxDiskTimeoutLegacy, kvs.GetWithDefault(MaxTimeout, DefaultKVS))))
 	if d == "" {
-		d = env.Get("_MINIO_DRIVE_MAX_TIMEOUT", "")
-		if d == "" {
-			d = env.Get("_MINIO_DISK_MAX_TIMEOUT", "")
-		}
-	}
-
-	dur, _ := time.ParseDuration(d)
-	if dur < time.Second {
 		cfg.MaxTimeout = 30 * time.Second
 	} else {
-		cfg.MaxTimeout = getMaxTimeout(dur)
+		dur, _ := time.ParseDuration(d)
+		if dur < time.Second {
+			cfg.MaxTimeout = 30 * time.Second
+		} else {
+			cfg.MaxTimeout = getMaxTimeout(dur)
+		}
 	}
 	return cfg, err
 }
 
 func getMaxTimeout(t time.Duration) time.Duration {
-	if t < time.Second {
-		// get default value
-		d := env.Get("_MINIO_DRIVE_MAX_TIMEOUT", "")
-		if d == "" {
-			d = env.Get("_MINIO_DISK_MAX_TIMEOUT", "")
-		}
-		dur, _ := time.ParseDuration(d)
-		if dur < time.Second {
-			return 30 * time.Second
-		}
-		return dur
+	if t > time.Second {
+		return t
 	}
-	return t
+	// get default value
+	d := env.Get(EnvMaxDriveTimeoutLegacy, env.Get(EnvMaxDiskTimeoutLegacy, ""))
+	if d == "" {
+		return 30 * time.Second
+	}
+	dur, _ := time.ParseDuration(d)
+	if dur < time.Second {
+		return 30 * time.Second
+	}
+	return dur
 }

internal/config/drive/help.go

@@ -22,12 +22,13 @@ import "github.com/minio/minio/internal/config"
 var (
 	// MaxTimeout is the max timeout for drive
 	MaxTimeout = "max_timeout"
+
 	// HelpDrive is help for drive
 	HelpDrive = config.HelpKVS{
 		config.HelpKV{
 			Key:         MaxTimeout,
 			Type:        "string",
-			Description: "set per call max_timeout for the drive, defaults to 2 minutes",
+			Description: "set per call max_timeout for the drive, defaults to 30 seconds",
 			Optional:    true,
 		},
 	}

internal/deadlineconn/deadlineconn.go

@@ -33,13 +33,13 @@ type DeadlineConn struct {
 // Sets read deadline
 func (c *DeadlineConn) setReadDeadline() {
 	if c.readDeadline > 0 {
-		c.SetReadDeadline(time.Now().UTC().Add(c.readDeadline))
+		c.Conn.SetReadDeadline(time.Now().UTC().Add(c.readDeadline))
 	}
 }
 
 func (c *DeadlineConn) setWriteDeadline() {
 	if c.writeDeadline > 0 {
-		c.SetWriteDeadline(time.Now().UTC().Add(c.writeDeadline))
+		c.Conn.SetWriteDeadline(time.Now().UTC().Add(c.writeDeadline))
 	}
 }
 

internal/grid/connection.go

@@ -42,6 +42,7 @@ import (
 	xioutil "github.com/minio/minio/internal/ioutil"
 	"github.com/minio/minio/internal/logger"
 	"github.com/minio/minio/internal/pubsub"
+	xnet "github.com/minio/pkg/v2/net"
 	"github.com/puzpuzpuz/xsync/v3"
 	"github.com/tinylib/msgp/msgp"
 	"github.com/zeebo/xxh3"
@@ -677,9 +678,8 @@ func (c *Connection) connect() {
 				return
 			}
 			if gotState != StateConnecting {
-				// Don't print error on first attempt,
-				// and after that only once per hour.
-				gridLogOnceIf(c.ctx, fmt.Errorf("grid: %s connecting to %s: %w (%T) Sleeping %v (%v)", c.Local, toDial, err, err, sleep, gotState), toDial)
+				// Don't print error on first attempt, and after that only once per hour.
+				gridLogOnceIf(c.ctx, fmt.Errorf("grid: %s re-connecting to %s: %w (%T) Sleeping %v (%v)", c.Local, toDial, err, err, sleep, gotState), toDial)
 			}
 			c.updateState(StateConnectionError)
 			time.Sleep(sleep)
@@ -972,7 +972,9 @@ func (c *Connection) handleMessages(ctx context.Context, conn net.Conn) {
 			msg, err = readDataInto(msg, conn, c.side, ws.OpBinary)
 			if err != nil {
 				cancel(ErrDisconnected)
-				gridLogIfNot(ctx, fmt.Errorf("ws read: %w", err), net.ErrClosed, io.EOF)
+				if !xnet.IsNetworkOrHostDown(err, true) {
+					gridLogIfNot(ctx, fmt.Errorf("ws read: %w", err), net.ErrClosed, io.EOF)
+				}
 				return
 			}
 			if c.incomingBytes != nil {
@@ -983,7 +985,9 @@ func (c *Connection) handleMessages(ctx context.Context, conn net.Conn) {
 			var m message
 			subID, remain, err := m.parse(msg)
 			if err != nil {
-				gridLogIf(ctx, fmt.Errorf("ws parse package: %w", err))
+				if !xnet.IsNetworkOrHostDown(err, true) {
+					gridLogIf(ctx, fmt.Errorf("ws parse package: %w", err))
+				}
 				cancel(ErrDisconnected)
 				return
 			}
@@ -1004,7 +1008,9 @@ func (c *Connection) handleMessages(ctx context.Context, conn net.Conn) {
 				var next []byte
 				next, remain, err = msgp.ReadBytesZC(remain)
 				if err != nil {
-					gridLogIf(ctx, fmt.Errorf("ws read merged: %w", err))
+					if !xnet.IsNetworkOrHostDown(err, true) {
+						gridLogIf(ctx, fmt.Errorf("ws read merged: %w", err))
+					}
 					cancel(ErrDisconnected)
 					return
 				}
@@ -1012,7 +1018,9 @@ func (c *Connection) handleMessages(ctx context.Context, conn net.Conn) {
 				m.Payload = nil
 				subID, _, err = m.parse(next)
 				if err != nil {
-					gridLogIf(ctx, fmt.Errorf("ws parse merged: %w", err))
+					if !xnet.IsNetworkOrHostDown(err, true) {
+						gridLogIf(ctx, fmt.Errorf("ws parse merged: %w", err))
+					}
 					cancel(ErrDisconnected)
 					return
 				}
@@ -1119,18 +1127,24 @@ func (c *Connection) handleMessages(ctx context.Context, conn net.Conn) {
 			buf.Reset()
 			err := wsw.writeMessage(&buf, c.side, ws.OpBinary, toSend)
 			if err != nil {
-				gridLogIf(ctx, fmt.Errorf("ws writeMessage: %w", err))
+				if !xnet.IsNetworkOrHostDown(err, true) {
+					gridLogIf(ctx, fmt.Errorf("ws writeMessage: %w", err))
+				}
 				return
 			}
 			PutByteBuffer(toSend)
+
 			err = conn.SetWriteDeadline(time.Now().Add(connWriteTimeout))
 			if err != nil {
 				gridLogIf(ctx, fmt.Errorf("conn.SetWriteDeadline: %w", err))
 				return
 			}
+
 			_, err = buf.WriteTo(conn)
 			if err != nil {
-				gridLogIf(ctx, fmt.Errorf("ws write: %w", err))
+				if !xnet.IsNetworkOrHostDown(err, true) {
+					gridLogIf(ctx, fmt.Errorf("ws write: %w", err))
+				}
 				return
 			}
 			continue
@@ -1163,18 +1177,24 @@ func (c *Connection) handleMessages(ctx context.Context, conn net.Conn) {
 		buf.Reset()
 		err = wsw.writeMessage(&buf, c.side, ws.OpBinary, toSend)
 		if err != nil {
-			gridLogIf(ctx, fmt.Errorf("ws writeMessage: %w", err))
+			if !xnet.IsNetworkOrHostDown(err, true) {
+				gridLogIf(ctx, fmt.Errorf("ws writeMessage: %w", err))
+			}
 			return
 		}
-		// buf is our local buffer, so we can reuse it.
+
 		err = conn.SetWriteDeadline(time.Now().Add(connWriteTimeout))
 		if err != nil {
 			gridLogIf(ctx, fmt.Errorf("conn.SetWriteDeadline: %w", err))
 			return
 		}
+
+		// buf is our local buffer, so we can reuse it.
 		_, err = buf.WriteTo(conn)
 		if err != nil {
-			gridLogIf(ctx, fmt.Errorf("ws write: %w", err))
+			if !xnet.IsNetworkOrHostDown(err, true) {
+				gridLogIf(ctx, fmt.Errorf("ws write: %w", err))
+			}
 			return
 		}
 

internal/http/dial_linux.go

@@ -26,6 +26,7 @@ import (
 	"syscall"
 	"time"
 
+	"github.com/minio/minio/internal/deadlineconn"
 	"golang.org/x/sys/unix"
 )
 
@@ -39,8 +40,8 @@ func setTCPParametersFn(opts TCPOptions) func(network, address string, c syscall
 
 			_ = unix.SetsockoptInt(fd, unix.SOL_SOCKET, unix.SO_REUSEPORT, 1)
 
+			// Enable custom socket send/recv buffers.
 			if opts.SendBufSize > 0 {
-				// Enable big buffers
 				_ = unix.SetsockoptInt(fd, unix.SOL_SOCKET, unix.SO_SNDBUF, opts.SendBufSize)
 			}
 
@@ -84,7 +85,9 @@ func setTCPParametersFn(opts TCPOptions) func(network, address string, c syscall
 			//    https://blog.cloudflare.com/when-tcp-sockets-refuse-to-die/
 			// This is a sensitive configuration, it is better to set it to high values, > 60 secs since it can
 			// affect clients reading data with a very slow pace  (disappropriate with socket buffer sizes)
-			_ = syscall.SetsockoptInt(fd, syscall.IPPROTO_TCP, unix.TCP_USER_TIMEOUT, opts.UserTimeout)
+			if opts.UserTimeout > 0 {
+				_ = syscall.SetsockoptInt(fd, syscall.IPPROTO_TCP, unix.TCP_USER_TIMEOUT, opts.UserTimeout)
+			}
 
 			if opts.Interface != "" {
 				if h, _, err := net.SplitHostPort(address); err == nil {
@@ -111,6 +114,16 @@ func NewInternodeDialContext(dialTimeout time.Duration, opts TCPOptions) DialCon
 			Timeout: dialTimeout,
 			Control: setTCPParametersFn(opts),
 		}
-		return dialer.DialContext(ctx, network, addr)
+		conn, err := dialer.DialContext(ctx, network, addr)
+		if err != nil {
+			return nil, err
+		}
+		if opts.DriveOPTimeout != nil {
+			// Read deadlines are sufficient for now as per various
+			// scenarios of hung node detection, we may add Write deadlines
+			// if needed later on.
+			return deadlineconn.New(conn).WithReadDeadline(opts.DriveOPTimeout()), nil
+		}
+		return conn, nil
 	}
 }

internal/http/listener.go

@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"net"
 	"syscall"
+	"time"
 )
 
 type acceptResult struct {
@@ -117,13 +118,27 @@ func (listener *httpListener) Addrs() (addrs []net.Addr) {
 
 // TCPOptions specify customizable TCP optimizations on raw socket
 type TCPOptions struct {
-	UserTimeout int              // this value is expected to be in milliseconds
+	UserTimeout int // this value is expected to be in milliseconds
+
+	// When the net.Conn is a remote drive this value is honored, we close the connection to remote peer proactively.
+	DriveOPTimeout func() time.Duration
+
 	SendBufSize int              // SO_SNDBUF size for the socket connection, NOTE: this sets server and client connection
 	RecvBufSize int              // SO_RECVBUF size for the socket connection, NOTE: this sets server and client connection
 	Interface   string           // This is a VRF device passed via `--interface` flag
 	Trace       func(msg string) // Trace when starting.
 }
 
+// ForWebsocket returns TCPOptions valid for websocket net.Conn
+func (t TCPOptions) ForWebsocket() TCPOptions {
+	return TCPOptions{
+		UserTimeout: t.UserTimeout,
+		Interface:   t.Interface,
+		SendBufSize: t.SendBufSize,
+		RecvBufSize: t.RecvBufSize,
+	}
+}
+
 // newHTTPListener - creates new httpListener object which is interface compatible to net.Listener.
 // httpListener is capable to
 // * listen to multiple addresses

internal/ioutil/ioutil.go

@@ -97,13 +97,6 @@ type ioret[V any] struct {
 	err error
 }
 
-// DeadlineWriter deadline writer with timeout
-type DeadlineWriter struct {
-	io.WriteCloser
-	timeout time.Duration
-	err     error
-}
-
 // WithDeadline will execute a function with a deadline and return a value of a given type.
 // If the deadline/context passes before the function finishes executing,
 // the zero value and the context error is returned.
@@ -145,21 +138,17 @@ func NewDeadlineWorker(timeout time.Duration) *DeadlineWorker {
 // channel so that the work function can attempt to exit gracefully.
 // Multiple calls to Run will run independently of each other.
 func (d *DeadlineWorker) Run(work func() error) error {
-	c := make(chan ioret[struct{}], 1)
-	t := time.NewTimer(d.timeout)
-	go func() {
-		c <- ioret[struct{}]{val: struct{}{}, err: work()}
-	}()
+	_, err := WithDeadline[struct{}](context.Background(), d.timeout, func(ctx context.Context) (struct{}, error) {
+		return struct{}{}, work()
+	})
+	return err
+}
 
-	select {
-	case r := <-c:
-		if !t.Stop() {
-			<-t.C
-		}
-		return r.err
-	case <-t.C:
-		return context.DeadlineExceeded
-	}
+// DeadlineWriter deadline writer with timeout
+type DeadlineWriter struct {
+	io.WriteCloser
+	timeout time.Duration
+	err     error
 }
 
 // NewDeadlineWriter wraps a writer to make it respect given deadline

internal/ioutil/ioutil_test.go

@@ -41,6 +41,26 @@ func (w *sleepWriter) Close() error {
 	return nil
 }
 
+func TestDeadlineWorker(t *testing.T) {
+	work := NewDeadlineWorker(500 * time.Millisecond)
+
+	err := work.Run(func() error {
+		time.Sleep(600 * time.Millisecond)
+		return nil
+	})
+	if err != context.DeadlineExceeded {
+		t.Error("DeadlineWorker shouldn't be successful - should return context.DeadlineExceeded")
+	}
+
+	err = work.Run(func() error {
+		time.Sleep(450 * time.Millisecond)
+		return nil
+	})
+	if err != nil {
+		t.Error("DeadlineWorker should succeed")
+	}
+}
+
 func TestDeadlineWriter(t *testing.T) {
 	w := NewDeadlineWriter(&sleepWriter{timeout: 500 * time.Millisecond}, 450*time.Millisecond)
 	_, err := w.Write([]byte("1"))