Fix delays and iterim fix for the partial fix in #3502 (#3511)

This patch uses a technique where in a retryable storage
before object layer initialization has a higher delay
and waits for longer period upto 4 times with time unit
of seconds.

And uses another set of configuration after the disks
have been formatted, i.e use a lower retry backoff rate
and retrying only once per 5 millisecond.

Network IO error count is reduced to a lower value i.e 256
before we reject the disk completely. This is done so that
combination of retry logic and total error count roughly
come to around 2.5secs which is when we basically take the
disk offline completely.

NOTE: This patch doesn't fix the issue of what if the disk
is completely dead and comes back again after the initialization.
Such a mutating state requires a change in our startup sequence
which will be done subsequently. This is an interim fix to alleviate
users from these issues.
This commit is contained in:
Harshavardhana 2016-12-30 17:08:02 -08:00 committed by GitHub
parent dd68cdd802
commit 8562b22823
9 changed files with 191 additions and 68 deletions

View File

@ -22,6 +22,10 @@ import (
"time" "time"
) )
// Attempt to retry only this many number of times before
// giving up on the remote RPC entirely.
const globalAuthRPCRetryThreshold = 1
// GenericReply represents any generic RPC reply. // GenericReply represents any generic RPC reply.
type GenericReply struct{} type GenericReply struct{}
@ -165,7 +169,7 @@ func (authClient *AuthRPCClient) Call(serviceMethod string, args interface {
authClient.Close() authClient.Close()
// No need to return error until the retry count threshold has reached. // No need to return error until the retry count threshold has reached.
if i < globalMaxAuthRPCRetryThreshold { if i < globalAuthRPCRetryThreshold {
continue continue
} }
} }

View File

@ -20,6 +20,7 @@ import (
"bytes" "bytes"
"path/filepath" "path/filepath"
"testing" "testing"
"time"
) )
// TestNewFS - tests initialization of all input disks // TestNewFS - tests initialization of all input disks
@ -86,7 +87,12 @@ func TestNewFS(t *testing.T) {
if err != errInvalidArgument { if err != errInvalidArgument {
t.Errorf("Expecting error invalid argument, got %s", err) t.Errorf("Expecting error invalid argument, got %s", err)
} }
_, err = newFSObjects(&retryStorage{xlStorageDisks[0]}) _, err = newFSObjects(&retryStorage{
remoteStorage: xlStorageDisks[0],
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
})
if err != nil { if err != nil {
errMsg := "Unable to recognize backend format, Disk is not in FS format." errMsg := "Unable to recognize backend format, Disk is not in FS format."
if err.Error() == errMsg { if err.Error() == errMsg {

View File

@ -89,14 +89,6 @@ var (
// List of admin peers. // List of admin peers.
globalAdminPeers = adminPeers{} globalAdminPeers = adminPeers{}
// Attempt to retry only this many number of times before
// giving up on the remote disk entirely.
globalMaxStorageRetryThreshold = 3
// Attempt to retry only this many number of times before
// giving up on the remote RPC entirely.
globalMaxAuthRPCRetryThreshold = 1
// Add new variable global values here. // Add new variable global values here.
) )

View File

@ -311,16 +311,40 @@ func waitForFormatDisks(firstDisk bool, endpoints []*url.URL, storageDisks []Sto
if storageDisks == nil { if storageDisks == nil {
return nil, errInvalidArgument return nil, errInvalidArgument
} }
// Retryable disks before formatting, we need to have a larger
// retry window so that we wait enough amount of time before
// the disks come online.
retryDisks := make([]StorageAPI, len(storageDisks))
for i, storage := range storageDisks {
retryDisks[i] = &retryStorage{
remoteStorage: storage,
maxRetryAttempts: globalStorageInitRetryThreshold,
retryUnit: time.Second,
retryCap: time.Second * 30, // 30 seconds.
}
}
// Start retry loop retrying until disks are formatted properly, until we have reached // Start retry loop retrying until disks are formatted properly, until we have reached
// a conditional quorum of formatted disks. // a conditional quorum of formatted disks.
err = retryFormattingDisks(firstDisk, endpoints, storageDisks) err = retryFormattingDisks(firstDisk, endpoints, retryDisks)
if err != nil { if err != nil {
return nil, err return nil, err
} }
// Initialize the disk into a formatted disks wrapper. // Initialize the disk into a formatted disks wrapper.
formattedDisks = make([]StorageAPI, len(storageDisks)) formattedDisks = make([]StorageAPI, len(storageDisks))
for i, storage := range storageDisks { for i, storage := range storageDisks {
formattedDisks[i] = &retryStorage{storage} // After formatting is done we need a smaller time
// window and lower retry value before formatting.
formattedDisks[i] = &retryStorage{
remoteStorage: storage,
maxRetryAttempts: globalStorageRetryThreshold,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 5, // 5 milliseconds.
}
} }
// Success.
return formattedDisks, nil return formattedDisks, nil
} }

View File

@ -22,12 +22,25 @@ import (
"github.com/minio/minio/pkg/disk" "github.com/minio/minio/pkg/disk"
) )
const (
// Attempt to retry only this many number of times before
// giving up on the remote disk entirely during initialization.
globalStorageInitRetryThreshold = 4
// Attempt to retry only this many number of times before
// giving up on the remote disk entirely after initialization.
globalStorageRetryThreshold = 1
)
// Retry storage is an instance of StorageAPI which // Retry storage is an instance of StorageAPI which
// additionally verifies upon network shutdown if the // additionally verifies upon network shutdown if the
// underlying storage is available and is really // underlying storage is available and is really
// formatted. // formatted.
type retryStorage struct { type retryStorage struct {
remoteStorage StorageAPI remoteStorage StorageAPI
maxRetryAttempts int
retryUnit time.Duration
retryCap time.Duration
} }
// String representation of remoteStorage. // String representation of remoteStorage.
@ -209,13 +222,13 @@ func (f retryStorage) reInit() (err error) {
doneCh := make(chan struct{}) doneCh := make(chan struct{})
defer close(doneCh) defer close(doneCh)
for i := range newRetryTimer(time.Second, time.Second*30, MaxJitter, doneCh) { for i := range newRetryTimer(f.retryUnit, f.retryCap, MaxJitter, doneCh) {
// Initialize and make a new login attempt. // Initialize and make a new login attempt.
err = f.remoteStorage.Init() err = f.remoteStorage.Init()
if err != nil { if err != nil {
// No need to return error until the retry count // No need to return error until the retry count
// threshold has reached. // threshold has reached.
if i < globalMaxStorageRetryThreshold { if i < f.maxRetryAttempts {
continue continue
} }
return err return err
@ -226,7 +239,7 @@ func (f retryStorage) reInit() (err error) {
if err != nil { if err != nil {
// No need to return error until the retry count // No need to return error until the retry count
// threshold has reached. // threshold has reached.
if i < globalMaxStorageRetryThreshold { if i < f.maxRetryAttempts {
continue continue
} }
return err return err

View File

@ -20,6 +20,7 @@ import (
"bytes" "bytes"
"reflect" "reflect"
"testing" "testing"
"time"
) )
// Tests retry storage. // Tests retry storage.
@ -39,9 +40,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
// Validate all the conditions for retrying calls. // Validate all the conditions for retrying calls.
@ -52,9 +58,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -77,9 +88,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -97,9 +113,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -114,9 +135,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -131,9 +157,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -151,9 +182,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -168,9 +204,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -185,9 +226,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -202,9 +248,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -223,9 +274,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -248,9 +304,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -268,9 +329,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -285,9 +351,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {
@ -306,9 +377,14 @@ func TestRetryStorage(t *testing.T) {
if !ok { if !ok {
t.Fatal("storage disk is not *retryStorage type") t.Fatal("storage disk is not *retryStorage type")
} }
storageDisks[i] = &retryStorage{newNaughtyDisk(retryDisk, map[int]error{ storageDisks[i] = &retryStorage{
1: errDiskNotFound, remoteStorage: newNaughtyDisk(retryDisk, map[int]error{
}, nil)} 1: errDiskNotFound,
}, nil),
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}
} }
for _, disk := range storageDisks { for _, disk := range storageDisks {

View File

@ -90,7 +90,7 @@ func newRetryTimer(unit time.Duration, cap time.Duration, jitter float64, doneCh
go func() { go func() {
defer close(attemptCh) defer close(attemptCh)
nextBackoff := 1 nextBackoff := 0
for { for {
select { select {
// Attempts starts. // Attempts starts.

View File

@ -241,8 +241,14 @@ func (n *networkStorage) String() string {
return n.netAddr + ":" + n.netPath return n.netAddr + ":" + n.netPath
} }
// maximum allowed network IOError. // Network IO error count is kept at 256 with some simple
const maxAllowedNetworkIOError = 1024 // math. Before we reject the disk completely. The combination
// of retry logic and total error count roughly comes around
// 2.5secs ( 2 * 5 * time.Millisecond * 256) which is when we
// basically take the disk offline completely. This is considered
// sufficient time tradeoff to avoid large delays in-terms of
// incoming i/o.
const maxAllowedNetworkIOError = 256 // maximum allowed network IOError.
// Initializes the remote RPC connection by attempting a login attempt. // Initializes the remote RPC connection by attempting a login attempt.
func (n *networkStorage) Init() (err error) { func (n *networkStorage) Init() (err error) {

View File

@ -65,9 +65,6 @@ func init() {
// Enable caching. // Enable caching.
setMaxMemory() setMaxMemory()
// Tests don't need to retry.
globalMaxStorageRetryThreshold = 1
} }
func prepareFS() (ObjectLayer, string, error) { func prepareFS() (ObjectLayer, string, error) {
@ -1656,7 +1653,12 @@ func prepareNErroredDisks(storageDisks []StorageAPI, offline int, err error, t *
} }
for i := 0; i < offline; i++ { for i := 0; i < offline; i++ {
storageDisks[i] = &naughtyDisk{disk: &retryStorage{storageDisks[i]}, defaultErr: err} storageDisks[i] = &naughtyDisk{disk: &retryStorage{
remoteStorage: storageDisks[i],
maxRetryAttempts: 1,
retryUnit: time.Millisecond,
retryCap: time.Millisecond * 10,
}, defaultErr: err}
} }
return storageDisks return storageDisks
} }