From c0ddd24dde7980cda6d550f32b5beb7fc517f821 Mon Sep 17 00:00:00 2001 From: Brian Picciano Date: Wed, 1 Jan 2025 13:07:35 +0100 Subject: [PATCH] Don't restart garage on config change --- go/daemon/children/children.go | 22 +++++------ go/daemon/children/garage.go | 12 +++--- go/garage/garagesrv/tpl.go | 38 ++++++++----------- .../garage-dont-restart-on-peer-change.md | 11 ------ tasks/bugs/garage-remove-then-re-add-alloc.md | 14 +++++++ 5 files changed, 44 insertions(+), 53 deletions(-) delete mode 100644 tasks/bugs/garage-dont-restart-on-peer-change.md create mode 100644 tasks/bugs/garage-remove-then-re-add-alloc.md diff --git a/go/daemon/children/children.go b/go/daemon/children/children.go index 2d567a4..7452e54 100644 --- a/go/daemon/children/children.go +++ b/go/daemon/children/children.go @@ -205,7 +205,7 @@ func (c *Children) reloadGarage( return nil } - var anyChanged bool + var anyCreated bool for _, alloc := range allocs { var ( procName = garagePmuxProcName(alloc) @@ -216,7 +216,7 @@ func (c *Children) reloadGarage( ) ) - childConfigPath, changed, err := garageWriteChildConfig( + childConfigPath, err := garageWriteChildConfig( ctx, c.logger, c.garageRPCSecret, @@ -227,26 +227,22 @@ func (c *Children) reloadGarage( ) if err != nil { return fmt.Errorf("writing child config file for alloc %+v: %w", alloc, err) - } else if !changed { - c.logger.Info(ctx, "No changes to garage config file") + } + + if _, ok := c.garageProcs[procName]; ok { + c.logger.Info(ctx, "Garage instance already exists") continue } - anyChanged = true + anyCreated = true - if proc, ok := c.garageProcs[procName]; ok { - c.logger.Info(ctx, "garage config has changed, restarting process") - proc.Restart() - continue - } - - c.logger.Info(ctx, "garage config has been added, creating process") + c.logger.Info(ctx, "Garage config has been added, creating process") c.garageProcs[procName] = garagePmuxProc( ctx, c.logger, c.binDirPath, procName, childConfigPath, ) } - if anyChanged { + if anyCreated { if err := waitForGarage( ctx, c.logger, diff --git a/go/daemon/children/garage.go b/go/daemon/children/garage.go index cc29598..cf6f04d 100644 --- a/go/daemon/children/garage.go +++ b/go/daemon/children/garage.go @@ -71,7 +71,7 @@ func garageWriteChildConfig( hostBootstrap bootstrap.Bootstrap, alloc daecommon.ConfigStorageAllocation, ) ( - string, bool, error, + string, error, ) { var ( thisHost = hostBootstrap.ThisHost() @@ -94,10 +94,10 @@ func garageWriteChildConfig( dbEngine, err := garagesrv.GetDBEngine(alloc.MetaPath) if err != nil { - return "", false, fmt.Errorf("getting alloc db engine: %w", err) + return "", fmt.Errorf("getting alloc db engine: %w", err) } - changed, err := garagesrv.WriteGarageTomlFile( + err = garagesrv.WriteGarageTomlFile( ctx, logger, garageTomlPath, @@ -115,12 +115,12 @@ func garageWriteChildConfig( ) if err != nil { - return "", false, fmt.Errorf( + return "", fmt.Errorf( "creating garage.toml file at %q: %w", garageTomlPath, err, ) } - return garageTomlPath, changed, nil + return garageTomlPath, nil } func garagePmuxProcName(alloc daecommon.ConfigStorageAllocation) string { @@ -164,7 +164,7 @@ func garagePmuxProcs( } for _, alloc := range allocs { - childConfigPath, _, err := garageWriteChildConfig( + childConfigPath, err := garageWriteChildConfig( ctx, logger, rpcSecret, runtimeDirPath, adminToken, diff --git a/go/garage/garagesrv/tpl.go b/go/garage/garagesrv/tpl.go index 1d3e23b..687a291 100644 --- a/go/garage/garagesrv/tpl.go +++ b/go/garage/garagesrv/tpl.go @@ -1,15 +1,15 @@ package garagesrv import ( - "cmp" + "bytes" "context" + "fmt" "io" - "slices" + "os" "strconv" "text/template" "isle/garage" - "isle/toolkit" "dev.mediocregopher.com/mediocre-go-lib.git/mlog" ) @@ -58,26 +58,18 @@ func RenderGarageToml(into io.Writer, data GarageTomlData) error { } // WriteGarageTomlFile renders a garage.toml using the given data to a new file -// at the given path, returning true if the file changed or didn't -// previously exist. +// at the given path. func WriteGarageTomlFile( - ctx context.Context, - logger *mlog.Logger, - path string, - data GarageTomlData, -) ( - bool, error, -) { - slices.SortFunc(data.BootstrapPeers, func(i, j garage.RemoteNode) int { - return cmp.Or( - cmp.Compare(i.IP, j.IP), - cmp.Compare(i.RPCPort, j.RPCPort), - ) - }) + ctx context.Context, logger *mlog.Logger, path string, data GarageTomlData, +) error { + buf := new(bytes.Buffer) + if err := garageTomlTpl.Execute(buf, data); err != nil { + return fmt.Errorf("executing template: %w", err) + } - return toolkit.WriteFileCheckChanged( - ctx, logger, path, 0600, func(w io.Writer) error { - return garageTomlTpl.Execute(w, data) - }, - ) + if err := os.WriteFile(path, buf.Bytes(), 0600); err != nil { + return fmt.Errorf("writing file: %w", err) + } + + return nil } diff --git a/tasks/bugs/garage-dont-restart-on-peer-change.md b/tasks/bugs/garage-dont-restart-on-peer-change.md deleted file mode 100644 index b307685..0000000 --- a/tasks/bugs/garage-dont-restart-on-peer-change.md +++ /dev/null @@ -1,11 +0,0 @@ ---- -type: task ---- - -# Don't Restart Garage on Peer Change - -When the garage configuration has changed, it is not necessary to issue a -restart to the garage processes if only the bootstrap peers have changed. Those -are only necessary on startup of garage. - -Implementing this should speed up integration tests a bit. diff --git a/tasks/bugs/garage-remove-then-re-add-alloc.md b/tasks/bugs/garage-remove-then-re-add-alloc.md new file mode 100644 index 0000000..9fca654 --- /dev/null +++ b/tasks/bugs/garage-remove-then-re-add-alloc.md @@ -0,0 +1,14 @@ +--- +type: task +after: + - ./garage-apply-layout-before-stopping-instance.md +--- + +I think there is currently a bug related to re-adding a storage allocation on an +RPC port of a garage instance which was previously used: + +- Step 1) Remove a storage allocation using RPC port N +- Step 2) Add a new allocation using RPC port N, but with a new data/meta dir + +I believe in this case garage will go back to using the old data/meta dir, and +possibly even re-use the old pubkey.