Merge pull request #18842 from ghouscht/defrag-fixes-backport-3.5

[3.5] fix(defrag): handle errors during defrag

Author: ahrtr

server/mvcc/backend/backend.go

@@ -477,22 +477,21 @@ func (b *backend) defrag() error {
 	b.readTx.Lock()
 	defer b.readTx.Unlock()
 
-	b.batchTx.unsafeCommit(true)
-
-	b.batchTx.tx = nil
-
 	// Create a temporary file to ensure we start with a clean slate.
 	// Snapshotter.cleanupSnapdir cleans up any of these that are found during startup.
 	dir := filepath.Dir(b.db.Path())
 	temp, err := ioutil.TempFile(dir, "db.tmp.*")
 	if err != nil {
 		return err
 	}
+
 	options := bolt.Options{}
 	if boltOpenOptions != nil {
 		options = *boltOpenOptions
 	}
 	options.OpenFile = func(_ string, _ int, _ os.FileMode) (file *os.File, err error) {
+		// gofail: var defragOpenFileError string
+		// return nil, fmt.Errorf(defragOpenFileError)
 		return temp, nil
 	}
 	// Don't load tmp db into memory regardless of opening options
@@ -515,13 +514,23 @@ func (b *backend) defrag() error {
 			zap.String("current-db-size-in-use", humanize.Bytes(uint64(sizeInUse1))),
 		)
 	}
+
+	// Commit/stop and then reset current transactions (including the readTx)
+	b.batchTx.unsafeCommit(true)
+	b.batchTx.tx = nil
+
 	// gofail: var defragBeforeCopy struct{}
 	err = defragdb(b.db, tmpdb, defragLimit)
 	if err != nil {
 		tmpdb.Close()
 		if rmErr := os.RemoveAll(tmpdb.Path()); rmErr != nil {
 			b.lg.Error("failed to remove db.tmp after defragmentation completed", zap.Error(rmErr))
 		}
+
+		// restore the bbolt transactions if defragmentation fails
+		b.batchTx.tx = b.unsafeBegin(true)
+		b.readTx.tx = b.unsafeBegin(false)
+
 		return err
 	}
 
@@ -574,6 +583,9 @@ func (b *backend) defrag() error {
 }
 
 func defragdb(odb, tmpdb *bolt.DB, limit int) error {
+	// gofail: var defragdbFail string
+	// return fmt.Errorf(defragdbFail)
+
 	// open a tx on tmpdb for writes
 	tmptx, err := tmpdb.Begin(true)
 	if err != nil {

tests/e2e/defrag_no_space_test.go

@@ -0,0 +1,74 @@
+// Copyright 2024 The etcd Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package e2e
+
+import (
+	"context"
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+
+	"go.etcd.io/etcd/tests/v3/framework/e2e"
+)
+
+func TestDefragNoSpace(t *testing.T) {
+	tests := []struct {
+		name      string
+		failpoint string
+		err       string
+	}{
+		{
+			name:      "no space (#18810) - can't open/create new bbolt db",
+			failpoint: "defragOpenFileError",
+			err:       "no space",
+		},
+		{
+			name:      "defragdb failure",
+			failpoint: "defragdbFail",
+			err:       "some random error",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			e2e.BeforeTest(t)
+
+			clus, err := e2e.NewEtcdProcessCluster(t,
+				&e2e.EtcdProcessClusterConfig{
+					ClusterSize:   1,
+					LogLevel:      "debug",
+					GoFailEnabled: true,
+				},
+			)
+			require.NoError(t, err)
+			t.Cleanup(func() { clus.Stop() })
+
+			member := clus.Procs[0]
+			etcdctl := member.Etcdctl(e2e.ClientNonTLS, false, false)
+
+			require.NoError(t, member.Failpoints().SetupHTTP(context.Background(), tc.failpoint, fmt.Sprintf(`return("%s")`, tc.err)))
+			require.ErrorContains(t, etcdctl.Defragment(time.Minute), tc.err)
+
+			// Make sure etcd continues to run even after the failed defrag attempt
+			require.NoError(t, etcdctl.Put("foo", "bar"))
+			value, err := etcdctl.Get("foo")
+			require.NoError(t, err)
+			require.Len(t, value.Kvs, 1)
+			require.Equal(t, "bar", string(value.Kvs[0].Value))
+		})
+	}
+}