From dd0a2da55e456bfe69212cd6bcb11c4af65f19bb Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 5 Nov 2026 01:21:13 +0800 Subject: [PATCH] fix(heal): send the dry-run flag to remote bucket heal remotePeerS3Client.HealBucket sent only the bucket name and the remove flag, and HealBucketHandler built its heal options from those two values. During a dry-run bucket heal every remote node therefore ran a real one: it created the bucket directory on drives that lacked it, or removed empty bucket directories when the bucket was missing from every pool. Only the coordinating node honored DryRun. Add a "dry-run" key to the peer message and read it into HealOpts.DryRun on the receiving side, so healBucketLocal returns before MakeVol and DeleteVol on every node. The message is a string map: an older node ignores the key, and a missing key reads as false, so a mixed-version cluster behaves as before until every node is upgraded. The regression calls the real client over a test grid connection to the real handler. It fails on main, and it fails if either side of the change is removed. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Feng Ruohang --- cmd/peer-rest-server.go | 2 + cmd/peer-s3-client.go | 15 +++-- cmd/peer-s3-heal-dryrun_test.go | 107 ++++++++++++++++++++++++++++++++ cmd/peer-s3-server.go | 1 + 4 files changed, 121 insertions(+), 4 deletions(-) create mode 100644 cmd/peer-s3-heal-dryrun_test.go diff --git a/cmd/peer-rest-server.go b/cmd/peer-rest-server.go index b28c8db44..25ca056b3 100644 --- a/cmd/peer-rest-server.go +++ b/cmd/peer-rest-server.go @@ -1291,8 +1291,10 @@ func (s *peerRESTServer) HealBucketHandler(mss *grid.MSS) (np *grid.JSON[madmin. } bucketDeleted := mss.Get(peerS3BucketDeleted) == "true" + dryRun := mss.Get(peerS3BucketDryRun) == "true" res, err := healBucketLocal(context.Background(), bucket, madmin.HealOpts{ Remove: bucketDeleted, + DryRun: dryRun, }) if err != nil { return np, grid.NewRemoteErr(err) diff --git a/cmd/peer-s3-client.go b/cmd/peer-s3-client.go index 5b30f76b2..a0262ef6b 100644 --- a/cmd/peer-s3-client.go +++ b/cmd/peer-s3-client.go @@ -354,10 +354,7 @@ func (client *remotePeerS3Client) HealBucket(ctx context.Context, bucket string, return madmin.HealResultItem{}, nil } - mss := grid.NewMSSWith(map[string]string{ - peerS3Bucket: bucket, - peerS3BucketDeleted: strconv.FormatBool(opts.Remove), - }) + mss := healBucketPeerMSS(bucket, opts) ctx, cancel := context.WithTimeout(ctx, globalDriveConfig.GetMaxTimeout()) defer cancel() @@ -366,6 +363,16 @@ func (client *remotePeerS3Client) HealBucket(ctx context.Context, bucket string, return resp.ValueOrZero(), toStorageErr(err) } +// healBucketPeerMSS encodes the bucket heal options a peer applies to its +// local drives. Peers that predate the dry-run key ignore it. +func healBucketPeerMSS(bucket string, opts madmin.HealOpts) *grid.MSS { + return grid.NewMSSWith(map[string]string{ + peerS3Bucket: bucket, + peerS3BucketDeleted: strconv.FormatBool(opts.Remove), + peerS3BucketDryRun: strconv.FormatBool(opts.DryRun), + }) +} + // GetBucketInfo returns bucket stat info from a peer func (client *remotePeerS3Client) GetBucketInfo(ctx context.Context, bucket string, opts BucketOptions) (BucketInfo, error) { conn := client.gridConn() diff --git a/cmd/peer-s3-heal-dryrun_test.go b/cmd/peer-s3-heal-dryrun_test.go new file mode 100644 index 000000000..fa75ed708 --- /dev/null +++ b/cmd/peer-s3-heal-dryrun_test.go @@ -0,0 +1,107 @@ +// Copyright (c) 2026 Feng Ruohang +// +// This file is part of Silo Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "os" + "path/filepath" + "testing" + + "github.com/minio/madmin-go/v3" + "github.com/minio/minio/internal/grid" +) + +// A remote node must apply the caller's dry run to its local bucket heal. +// The request goes from remotePeerS3Client.HealBucket over a grid connection +// to the peer handler, as it does between the nodes of a deployment. +func TestPeerHealBucketDryRun(t *testing.T) { + obj, roots, err := prepareErasure(t.Context(), 4) + if err != nil { + t.Fatal(err) + } + setObjectLayer(obj) + defer removeRoots(roots) + defer obj.Shutdown(t.Context()) + + tg, err := grid.SetupTestGrid(2) + if err != nil { + t.Fatal(err) + } + defer tg.Cleanup() + if err = healBucketRPC.Register(tg.Managers[1], (&peerRESTServer{}).HealBucketHandler); err != nil { + t.Fatal(err) + } + conn := func() *grid.Connection { return tg.Managers[0].Connection(tg.Hosts[1]) } + client := &remotePeerS3Client{gridConn: conn} + + const bucket = "peer-heal-dry-run" + if err = obj.MakeBucket(t.Context(), bucket, MakeBucketOptions{}); err != nil { + t.Fatal(err) + } + dirs := make([]string, len(roots)) + for i, root := range roots { + dirs[i] = filepath.Join(root, bucket) + } + if err = os.Remove(dirs[0]); err != nil { + t.Fatal(err) + } + exists := func(dir string) bool { + _, err := os.Stat(dir) + return err == nil + } + heal := func(opts madmin.HealOpts) { + t.Helper() + if _, err := client.HealBucket(t.Context(), bucket, opts); err != nil { + t.Fatal(err) + } + } + + // Dry run must not recreate the missing bucket directory. + heal(madmin.HealOpts{DryRun: true}) + if exists(dirs[0]) { + t.Fatal("dry-run peer bucket heal created a missing bucket directory") + } + + // Dry run must not remove the empty bucket directories. + heal(madmin.HealOpts{DryRun: true, Remove: true}) + for _, dir := range dirs[1:] { + if !exists(dir) { + t.Fatalf("dry-run peer bucket heal removed %s", dir) + } + } + + // Controls. A message without the dry-run key, as sent by an older node, + // still heals: the fixture reaches MakeVol. + if _, err = healBucketRPC.Call(t.Context(), conn(), grid.NewMSSWith(map[string]string{ + peerS3Bucket: bucket, + peerS3BucketDeleted: "false", + })); err != nil { + t.Fatal(err) + } + if !exists(dirs[0]) { + t.Fatal("control: peer bucket heal did not recreate the missing directory") + } + + // A real heal with Remove reaches DeleteVol for the empty directories. + heal(madmin.HealOpts{Remove: true}) + for _, dir := range dirs { + if exists(dir) { + t.Fatalf("control: peer bucket heal kept empty directory %s", dir) + } + } +} diff --git a/cmd/peer-s3-server.go b/cmd/peer-s3-server.go index a252ee85f..295866410 100644 --- a/cmd/peer-s3-server.go +++ b/cmd/peer-s3-server.go @@ -29,6 +29,7 @@ import ( const ( peerS3Bucket = "bucket" peerS3BucketDeleted = "bucket-deleted" + peerS3BucketDryRun = "dry-run" peerS3BucketForceCreate = "force-create" peerS3BucketForceDelete = "force-delete" )