From f9b4a8d6e8092796b4fea5a802e6b5172eb6b240 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Fri, 19 Jan 2024 18:27:58 -0800 Subject: [PATCH] improve server update behavior by re-using memory properly (#18831) --- cmd/admin-handlers.go | 6 +++--- cmd/notification.go | 5 +++-- cmd/peer-rest-client.go | 25 +++++++------------------ cmd/peer-rest-common.go | 11 +++++++---- cmd/peer-rest-server.go | 19 +++++++++++++------ cmd/update.go | 24 +++++++++--------------- 6 files changed, 42 insertions(+), 48 deletions(-) diff --git a/cmd/admin-handlers.go b/cmd/admin-handlers.go index 671ad1989..46c546928 100644 --- a/cmd/admin-handlers.go +++ b/cmd/admin-handlers.go @@ -149,7 +149,7 @@ func (a adminAPIHandlers) ServerUpdateHandler(w http.ResponseWriter, r *http.Req } // Download Binary Once - reader, err := downloadBinary(u, mode) + bin, err := downloadBinary(u, mode) if err != nil { logger.LogIf(ctx, fmt.Errorf("server update failed with %w", err)) writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) @@ -157,7 +157,7 @@ func (a adminAPIHandlers) ServerUpdateHandler(w http.ResponseWriter, r *http.Req } // Push binary to other servers - for _, nerr := range globalNotificationSys.VerifyBinary(ctx, u, sha256Sum, releaseInfo, reader) { + for _, nerr := range globalNotificationSys.VerifyBinary(ctx, u, sha256Sum, releaseInfo, bin) { if nerr.Err != nil { err := AdminError{ Code: AdminUpdateApplyFailure, @@ -171,7 +171,7 @@ func (a adminAPIHandlers) ServerUpdateHandler(w http.ResponseWriter, r *http.Req } } - err = verifyBinary(u, sha256Sum, releaseInfo, mode, reader) + err = verifyBinary(u, sha256Sum, releaseInfo, mode, bytes.NewReader(bin)) if err != nil { logger.LogIf(ctx, fmt.Errorf("server update failed with %w", err)) writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) diff --git a/cmd/notification.go b/cmd/notification.go index 37ed4d72e..bd3e430f0 100644 --- a/cmd/notification.go +++ b/cmd/notification.go @@ -18,6 +18,7 @@ package cmd import ( + "bytes" "context" "errors" "fmt" @@ -338,7 +339,7 @@ func (sys *NotificationSys) DownloadProfilingData(ctx context.Context, writer io } // VerifyBinary - asks remote peers to verify the checksum -func (sys *NotificationSys) VerifyBinary(ctx context.Context, u *url.URL, sha256Sum []byte, releaseInfo string, reader []byte) []NotificationPeerErr { +func (sys *NotificationSys) VerifyBinary(ctx context.Context, u *url.URL, sha256Sum []byte, releaseInfo string, bin []byte) []NotificationPeerErr { ng := WithNPeers(len(sys.peerClients)) for idx, client := range sys.peerClients { if client == nil { @@ -346,7 +347,7 @@ func (sys *NotificationSys) VerifyBinary(ctx context.Context, u *url.URL, sha256 } client := client ng.Go(ctx, func() error { - return client.VerifyBinary(ctx, u, sha256Sum, releaseInfo, reader) + return client.VerifyBinary(ctx, u, sha256Sum, releaseInfo, bytes.NewReader(bin)) }, idx, *client.host) } return ng.Wait() diff --git a/cmd/peer-rest-client.go b/cmd/peer-rest-client.go index 7b1839a88..8bd195f6b 100644 --- a/cmd/peer-rest-client.go +++ b/cmd/peer-rest-client.go @@ -21,6 +21,7 @@ import ( "bytes" "context" "encoding/gob" + "encoding/hex" "errors" "fmt" "io" @@ -475,26 +476,14 @@ func (client *peerRESTClient) LoadGroup(group string) error { return nil } -type binaryInfo struct { - URL *url.URL - Sha256Sum []byte - ReleaseInfo string - BinaryFile []byte -} - // VerifyBinary - sends verify binary message to remote peers. -func (client *peerRESTClient) VerifyBinary(ctx context.Context, u *url.URL, sha256Sum []byte, releaseInfo string, readerInput []byte) error { +func (client *peerRESTClient) VerifyBinary(ctx context.Context, u *url.URL, sha256Sum []byte, releaseInfo string, reader io.Reader) error { values := make(url.Values) - var reader bytes.Buffer - if err := gob.NewEncoder(&reader).Encode(binaryInfo{ - URL: u, - Sha256Sum: sha256Sum, - ReleaseInfo: releaseInfo, - BinaryFile: readerInput, - }); err != nil { - return err - } - respBody, err := client.callWithContext(ctx, peerRESTMethodDownloadBinary, values, &reader, -1) + values.Set(peerRESTURL, u.String()) + values.Set(peerRESTSha256Sum, hex.EncodeToString(sha256Sum)) + values.Set(peerRESTReleaseInfo, releaseInfo) + + respBody, err := client.callWithContext(ctx, peerRESTMethodVerifyBinary, values, reader, -1) if err != nil { return err } diff --git a/cmd/peer-rest-common.go b/cmd/peer-rest-common.go index fcfc1eee3..4c5030023 100644 --- a/cmd/peer-rest-common.go +++ b/cmd/peer-rest-common.go @@ -1,4 +1,4 @@ -// Copyright (c) 2015-2021 MinIO, Inc. +// Copyright (c) 2015-2024 MinIO, Inc. // // This file is part of MinIO Object Storage stack // @@ -18,8 +18,7 @@ package cmd const ( - peerRESTVersion = "v35" // Add new service restart behavior - + peerRESTVersion = "v36" // Rewrite VerifyBinaryHandler() peerRESTVersionPrefix = SlashSeparator + peerRESTVersion peerRESTPrefix = minioReservedBucketPath + "/peer" peerRESTPath = peerRESTPrefix + peerRESTVersionPrefix @@ -42,7 +41,7 @@ const ( peerRESTMethodLoadBucketMetadata = "/loadbucketmetadata" peerRESTMethodGetBucketStats = "/getbucketstats" peerRESTMethodGetAllBucketStats = "/getallbucketstats" - peerRESTMethodDownloadBinary = "/downloadbinary" + peerRESTMethodVerifyBinary = "/verifybinary" peerRESTMethodCommitBinary = "/commitbinary" peerRESTMethodSignalService = "/signalservice" peerRESTMethodBackgroundHealStatus = "/backgroundhealstatus" @@ -111,6 +110,10 @@ const ( peerRESTDryRun = "dry-run" peerRESTForce = "force" + peerRESTURL = "url" + peerRESTSha256Sum = "sha256sum" + peerRESTReleaseInfo = "releaseinfo" + peerRESTListenBucket = "bucket" peerRESTListenPrefix = "prefix" peerRESTListenSuffix = "suffix" diff --git a/cmd/peer-rest-server.go b/cmd/peer-rest-server.go index 49debe959..26a561f52 100644 --- a/cmd/peer-rest-server.go +++ b/cmd/peer-rest-server.go @@ -20,11 +20,13 @@ package cmd import ( "context" "encoding/gob" + "encoding/hex" "encoding/json" "errors" "fmt" "io" "net/http" + "net/url" "strconv" "strings" "sync/atomic" @@ -824,8 +826,8 @@ func (s *peerRESTServer) GetLocalDiskIDs(w http.ResponseWriter, r *http.Request) logger.LogIf(ctx, gob.NewEncoder(w).Encode(ids)) } -// DownloadBinary - updates the current server. -func (s *peerRESTServer) DownloadBinaryHandler(w http.ResponseWriter, r *http.Request) { +// VerifyBinary - verifies the downloaded binary is in-tact +func (s *peerRESTServer) VerifyBinaryHandler(w http.ResponseWriter, r *http.Request) { if !s.IsValid(w, r) { s.writeErrorResponse(w, errors.New("Invalid request")) return @@ -836,14 +838,19 @@ func (s *peerRESTServer) DownloadBinaryHandler(w http.ResponseWriter, r *http.Re return } - var info binaryInfo - err := gob.NewDecoder(r.Body).Decode(&info) + u, err := url.Parse(r.Form.Get(peerRESTURL)) if err != nil { s.writeErrorResponse(w, err) return } + sha256Sum, err := hex.DecodeString(r.Form.Get(peerRESTSha256Sum)) + if err != nil { + s.writeErrorResponse(w, err) + return + } + releaseInfo := r.Form.Get(peerRESTReleaseInfo) - if err = verifyBinary(info.URL, info.Sha256Sum, info.ReleaseInfo, getMinioMode(), info.BinaryFile); err != nil { + if err = verifyBinary(u, sha256Sum, releaseInfo, getMinioMode(), r.Body); err != nil { s.writeErrorResponse(w, err) return } @@ -1524,7 +1531,7 @@ func registerPeerRESTHandlers(router *mux.Router) { subrouter.Methods(http.MethodPost).Path(peerRESTVersionPrefix + peerRESTMethodLoadBucketMetadata).HandlerFunc(h(server.LoadBucketMetadataHandler)).Queries(restQueries(peerRESTBucket)...) subrouter.Methods(http.MethodPost).Path(peerRESTVersionPrefix + peerRESTMethodGetBucketStats).HandlerFunc(h(server.GetBucketStatsHandler)).Queries(restQueries(peerRESTBucket)...) subrouter.Methods(http.MethodPost).Path(peerRESTVersionPrefix + peerRESTMethodSignalService).HandlerFunc(h(server.SignalServiceHandler)).Queries(restQueries(peerRESTSignal)...) - subrouter.Methods(http.MethodPost).Path(peerRESTVersionPrefix + peerRESTMethodDownloadBinary).HandlerFunc(h(server.DownloadBinaryHandler)) + subrouter.Methods(http.MethodPost).Path(peerRESTVersionPrefix + peerRESTMethodVerifyBinary).HandlerFunc(h(server.VerifyBinaryHandler)).Queries(restQueries(peerRESTURL, peerRESTSha256Sum, peerRESTReleaseInfo)...) subrouter.Methods(http.MethodPost).Path(peerRESTVersionPrefix + peerRESTMethodCommitBinary).HandlerFunc(h(server.CommitBinaryHandler)) subrouter.Methods(http.MethodPost).Path(peerRESTVersionPrefix + peerRESTMethodDeletePolicy).HandlerFunc(h(server.DeletePolicyHandler)).Queries(restQueries(peerRESTPolicy)...) subrouter.Methods(http.MethodPost).Path(peerRESTVersionPrefix + peerRESTMethodLoadPolicy).HandlerFunc(h(server.LoadPolicyHandler)).Queries(restQueries(peerRESTPolicy)...) diff --git a/cmd/update.go b/cmd/update.go index 2fb54a15a..45191c03f 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -19,7 +19,6 @@ package cmd import ( "bufio" - "bytes" "crypto" "crypto/tls" "encoding/hex" @@ -508,10 +507,10 @@ func getUpdateReaderFromURL(u *url.URL, transport http.RoundTripper, mode string return resp.Body, nil } -var updateInProgress uint32 +var updateInProgress atomic.Uint32 // Function to get the reader from an architecture -func downloadBinary(u *url.URL, mode string) (readerReturn []byte, err error) { +func downloadBinary(u *url.URL, mode string) (bin []byte, err error) { transport := getUpdateTransport(30 * time.Second) var reader io.ReadCloser if u.Scheme == "https" || u.Scheme == "http" { @@ -523,13 +522,8 @@ func downloadBinary(u *url.URL, mode string) (readerReturn []byte, err error) { return nil, fmt.Errorf("unsupported protocol scheme: %s", u.Scheme) } defer xhttp.DrainBody(reader) - // convert a Reader to bytes - binaryFile, err := io.ReadAll(reader) - if err != nil { - return nil, err - } - return binaryFile, nil + return io.ReadAll(reader) } const ( @@ -537,11 +531,11 @@ const ( defaultMinisignPubkey = "RWTx5Zr1tiHQLwG9keckT0c45M3AGeHD6IvimQHpyRywVWGbP1aVSGav" ) -func verifyBinary(u *url.URL, sha256Sum []byte, releaseInfo, mode string, reader []byte) (err error) { - if !atomic.CompareAndSwapUint32(&updateInProgress, 0, 1) { +func verifyBinary(u *url.URL, sha256Sum []byte, releaseInfo, mode string, reader io.Reader) (err error) { + if !updateInProgress.CompareAndSwap(0, 1) { return errors.New("update already in progress") } - defer atomic.StoreUint32(&updateInProgress, 0) + defer updateInProgress.Store(0) transport := getUpdateTransport(30 * time.Second) opts := selfupdate.Options{ @@ -571,7 +565,7 @@ func verifyBinary(u *url.URL, sha256Sum []byte, releaseInfo, mode string, reader opts.Verifier = v } - if err = selfupdate.PrepareAndCheckBinary(bytes.NewReader(reader), opts); err != nil { + if err = selfupdate.PrepareAndCheckBinary(reader, opts); err != nil { var pathErr *os.PathError if errors.As(err, &pathErr) { return AdminError{ @@ -592,10 +586,10 @@ func verifyBinary(u *url.URL, sha256Sum []byte, releaseInfo, mode string, reader } func commitBinary() (err error) { - if !atomic.CompareAndSwapUint32(&updateInProgress, 0, 1) { + if !updateInProgress.CompareAndSwap(0, 1) { return errors.New("update already in progress") } - defer atomic.StoreUint32(&updateInProgress, 0) + defer updateInProgress.Store(0) opts := selfupdate.Options{}