CASMHMS-6294: Context cancel fixes

Cray-HPE · Dec 9, 2024 · 1ec8df4 · 1ec8df4
1 parent bb4b4e7
commit 1ec8df4
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Resolved various scaling/resource issues
+- Upgraded Go to 1.23
+- Updated go-retryablehttp to 0.7.7
+- Fixed docker compose build issues
 
 ## [2.29.0] - 2024-09-18
 

diff --git a/pkg/service-reservations/interface.go b/pkg/service-reservations/interface.go
@@ -1,6 +1,6 @@
 // MIT License
 //
-// (C) Copyright [2020-2023] Hewlett Packard Enterprise Development LP
+// (C) Copyright [2020-2024] Hewlett Packard Enterprise Development LP
 //
 // Permission is hereby granted, free of charge, to any person obtaining a
 // copy of this software and associated documentation files (the "Software"),
@@ -29,16 +29,17 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"net/http"
 	"net/url"
 	"sync"
 	"time"
 
+	base "github.com/Cray-HPE/hms-base/v2"
+	"github.com/Cray-HPE/hms-smd/v2/pkg/sm"
 	"github.com/hashicorp/go-retryablehttp"
 	"github.com/sirupsen/logrus"
-	"github.com/Cray-HPE/hms-base/v2"
-	"github.com/Cray-HPE/hms-smd/v2/pkg/sm"
 )
 
 const HSM_DEFAULT_RESERVATION_PATH = "/hsm/v2/locks/service/reservations"
@@ -47,6 +48,26 @@ const DEFAULT_TERM_MINUTES = 1
 const DefaultExpirationWindow = 30
 const DefaultSleepTime = 10
 
+// Response bodies should always be drained and closed, else we leak resources
+// and fail to reuse network connections.
+func DrainAndCloseResponseBody(resp *http.Response) {
+	if resp != nil && resp.Body != nil {
+			_, _ = io.Copy(io.Discard, resp.Body) // ok even if already drained
+			resp.Body.Close()                     // ok even if already closed
+	}
+}
+
+func DrainAndCloseResponseBodyAndCancelContext(resp *http.Response, ctxCancel context.CancelFunc) {
+	// Must always drain and close response body first
+	DrainAndCloseResponseBody(resp)
+
+	// Call context cancel function, if supplied.  This must always be done
+	// after draining and closing the response body
+	if ctxCancel != nil {
+			ctxCancel()
+	}
+}
+
 //TODO - Future Enhancements
 // * ALLOW both rigid an flexible processing -> not sure rigid release makes sense... so really just flexible on the AQUIRE
 // * alter CHECK, RELEASE functions to cope with flexibility -> return an array of tuples: something like xname, bool, error, etc
@@ -280,10 +301,11 @@ func (i *Production) doRenew(reservationKeys []Key, flex bool) (ReservationRelea
 	}
 	base.SetHTTPUserAgent(newRequest, serviceName)
 
-	reqContext, _ := context.WithTimeout(context.Background(), time.Second*40)
+	reqContext, reqCtxCancel := context.WithTimeout(context.Background(), time.Second*40)
 	req, err := retryablehttp.FromRequest(newRequest)
 	req = req.WithContext(reqContext)
 	if err != nil {
+		reqCtxCancel()
 		i.logger.WithField("error", err).Error("doRenew() - END")
 		return response, err
 	}
@@ -292,13 +314,13 @@ func (i *Production) doRenew(reservationKeys []Key, flex bool) (ReservationRelea
 
 	//make request
 	resp, err := i.httpClient.Do(req)
+	defer DrainAndCloseResponseBodyAndCancelContext(resp, reqCtxCancel)
 	if err != nil {
 		i.logger.WithField("error", err).Error("doRenew() - END")
 		return response, err
 	}
 
 	//process response
-	defer resp.Body.Close()
 	body, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
 		i.logger.WithField("error", err).Error("doRenew() - END")
@@ -351,10 +373,11 @@ func (i *Production) Aquire(xnames []string) error {
 	}
 	base.SetHTTPUserAgent(newRequest,serviceName)
 
-	reqContext, _ := context.WithTimeout(context.Background(), time.Second*40)
+	reqContext, reqCtxCancel := context.WithTimeout(context.Background(), time.Second*40)
 	req, err := retryablehttp.FromRequest(newRequest)
 	req = req.WithContext(reqContext)
 	if err != nil {
+		reqCtxCancel()
 		i.logger.WithField("error", err).Error("Aquire() - END")
 		return err
 	}
@@ -363,13 +386,13 @@ func (i *Production) Aquire(xnames []string) error {
 
 	//make request
 	resp, err := i.httpClient.Do(req)
+	defer DrainAndCloseResponseBodyAndCancelContext(resp, reqCtxCancel)
 	if err != nil {
 		i.logger.WithField("error", err).Error("Aquire() - END")
 		return err
 	}
 
 	//process response
-	defer resp.Body.Close()
 	body, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
 		i.logger.WithField("error", err).Error("Aquire() - END")
@@ -447,10 +470,11 @@ func (i *Production) FlexAquire(xnames []string) (ReservationCreateResponse,erro
 	}
 	base.SetHTTPUserAgent(newRequest,serviceName)
 
-	reqContext, _ := context.WithTimeout(context.Background(), time.Second*40)
+	reqContext, reqCtxCancel := context.WithTimeout(context.Background(), time.Second*40)
 	req, err := retryablehttp.FromRequest(newRequest)
 	req = req.WithContext(reqContext)
 	if err != nil {
+		reqCtxCancel()
 		i.logger.WithField("error", err).Error("SoftAquire() - END")
 		return resResponse,err
 	}
@@ -459,13 +483,13 @@ func (i *Production) FlexAquire(xnames []string) (ReservationCreateResponse,erro
 
 	//make request
 	resp, err := i.httpClient.Do(req)
+	defer DrainAndCloseResponseBodyAndCancelContext(resp, reqCtxCancel)
 	if err != nil {
 		i.logger.WithField("error", err).Error("SoftAquire() - END")
 		return resResponse,err
 	}
 
 	//process response
-	defer resp.Body.Close()
 	body, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
 		i.logger.WithField("error", err).Error("SoftAquire() - END")
@@ -632,10 +656,11 @@ func (i *Production) Release(xnames []string) error {
 	}
 	base.SetHTTPUserAgent(newRequest,serviceName)
 
-	reqContext, _ := context.WithTimeout(context.Background(), time.Second*40)
+	reqContext, reqCtxCancel := context.WithTimeout(context.Background(), time.Second*40)
 	req, err := retryablehttp.FromRequest(newRequest)
 	req = req.WithContext(reqContext)
 	if err != nil {
+		reqCtxCancel()
 		i.logger.WithField("error", err).Error("Release() - END")
 		return err
 	}
@@ -644,13 +669,13 @@ func (i *Production) Release(xnames []string) error {
 
 	//make request
 	resp, err := i.httpClient.Do(req)
+	defer DrainAndCloseResponseBodyAndCancelContext(resp, reqCtxCancel)
 	if err != nil {
 		i.logger.WithField("error", err).Error("Release() - END")
 		return err
 	}
 
 	//process response
-	defer resp.Body.Close()
 	body, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
 		i.logger.WithField("error", err).Error("Release() - END")
@@ -749,10 +774,11 @@ func (i *Production) FlexRelease(xnames []string) (ReservationReleaseRenewRespon
 	}
 	base.SetHTTPUserAgent(newRequest,serviceName)
 
-	reqContext, _ := context.WithTimeout(context.Background(), time.Second*40)
+	reqContext, reqCtxCancel := context.WithTimeout(context.Background(), time.Second*40)
 	req, err := retryablehttp.FromRequest(newRequest)
 	req = req.WithContext(reqContext)
 	if err != nil {
+		reqCtxCancel()
 		i.logger.WithField("error", err).Error("SoftRelease() - END")
 		return retData,err
 	}
@@ -761,13 +787,13 @@ func (i *Production) FlexRelease(xnames []string) (ReservationReleaseRenewRespon
 
 	//make request
 	resp, err := i.httpClient.Do(req)
+	defer DrainAndCloseResponseBodyAndCancelContext(resp, reqCtxCancel)
 	if err != nil {
 		i.logger.WithField("error", err).Error("SoftRelease() - END")
 		return retData,err
 	}
 
 	//process response
-	defer resp.Body.Close()
 	body, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
 		i.logger.WithField("error", err).Error("SoftRelease() - END")
@@ -851,10 +877,11 @@ func (i *Production) update() {
 	}
 	base.SetHTTPUserAgent(newRequest,serviceName)
 
-	reqContext, _ := context.WithTimeout(context.Background(), time.Second*40)
+	reqContext, reqCtxCancel := context.WithTimeout(context.Background(), time.Second*40)
 	req, err := retryablehttp.FromRequest(newRequest)
 	req = req.WithContext(reqContext)
 	if err != nil {
+		reqCtxCancel()
 		i.logger.WithField("error", err).Error("update() - END")
 		return
 	}
@@ -863,13 +890,13 @@ func (i *Production) update() {
 
 	//make request
 	resp, err := i.httpClient.Do(req)
+	defer DrainAndCloseResponseBodyAndCancelContext(resp, reqCtxCancel)
 	if err != nil {
 		i.logger.WithField("error", err).Error("update() - END")
 		return
 	}
 
 	//process response
-	defer resp.Body.Close()
 	body, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
 		i.logger.WithField("error", err).Error("update() - END")
@@ -954,10 +981,11 @@ func (i *Production) ValidateDeputyKeys(keys []Key) (ReservationCheckResponse,er
 			err)
 	}
 	req, err := retryablehttp.FromRequest(newRequest)
-	reqContext,_ := context.WithTimeout(context.Background(), 40*time.Second)
+	reqContext, reqCtxCancel := context.WithTimeout(context.Background(), 40*time.Second)
 	req = req.WithContext(reqContext)
 
 	rsp,rsperr := i.httpClient.Do(req)
+	defer DrainAndCloseResponseBodyAndCancelContext(rsp, reqCtxCancel)
 	if (rsperr != nil) {
 		return retData,fmt.Errorf("Error sending http request for deputy key check: %v",
 			rsperr)