From 928d943cdcfe4ba69ad233edcaa8ac4901365d17 Mon Sep 17 00:00:00 2001 From: George Blue Date: Wed, 19 Nov 2025 11:26:23 +0000 Subject: [PATCH] feat: adds the --instance-timeout flag to control timeout value --- integrationtests/upgrade_test.go | 32 +++++++++++ internal/ccapi/ccapi.go | 4 +- internal/ccapi/service_instances_test.go | 2 +- internal/ccapi/service_plans_test.go | 2 +- internal/ccapi/upgrade_instance.go | 2 +- internal/ccapi/upgrade_instance_test.go | 67 +++++++++++++++++++++++- internal/config/config.go | 3 ++ internal/config/config_test.go | 41 +++++++++++++++ internal/config/flags.go | 6 +++ internal/config/usage.go | 1 + internal/config/validations.go | 11 ++++ upgrade_all.go | 2 +- 12 files changed, 166 insertions(+), 7 deletions(-) diff --git a/integrationtests/upgrade_test.go b/integrationtests/upgrade_test.go index b2197ea..00afcb3 100644 --- a/integrationtests/upgrade_test.go +++ b/integrationtests/upgrade_test.go @@ -305,4 +305,36 @@ var _ = Describe("upgrade", func() { Expect(second.Sub(first)).To(BeNumerically("~", retryInterval, accuracy)) }) }) + + Context("instance timeout", func() { + BeforeEach(func() { + capi.AddBroker( + fakecapi.ServiceBroker{Name: brokerName}, + fakecapi.WithServiceOffering( + fakecapi.ServiceOffering{Name: "service-offering-1"}, + fakecapi.WithServicePlan( + fakecapi.ServicePlan{Name: "service-plan1", Version: "1.2.3"}, + // Instance takes 5 seconds to update, which will exceed our short timeout + fakecapi.WithServiceInstances(fakecapi.ServiceInstance{UpgradeAvailable: true, Version: "1.2.2", UpdateTime: 5 * time.Second}), + ), + ), + ) + }) + + It("respects the -instance-timeout flag", func() { + // Use a short timeout (1 second) that will be exceeded by the 5 second update time + session := cf("upgrade-all-services", brokerName, "--instance-polling-interval", "100ms", "--instance-timeout", "1s") + Eventually(session).WithTimeout(time.Minute).Should(Exit(1)) + + Expect(session.Out).To(Say("error upgrade request timeout")) + }) + + It("succeeds when timeout is long enough", func() { + // Use a timeout longer than the update time + session := cf("upgrade-all-services", brokerName, "--instance-polling-interval", "100ms", "--instance-timeout", "10s") + Eventually(session).WithTimeout(time.Minute).Should(Exit(0)) + + Expect(session.Out).To(Say("successfully upgraded 1 instances")) + }) + }) }) diff --git a/internal/ccapi/ccapi.go b/internal/ccapi/ccapi.go index 2773cfb..5ae7650 100644 --- a/internal/ccapi/ccapi.go +++ b/internal/ccapi/ccapi.go @@ -8,11 +8,13 @@ import ( type CCAPI struct { requester requester.Requester pollingInterval time.Duration + instanceTimeout time.Duration } -func NewCCAPI(req requester.Requester, pollingInterval time.Duration) CCAPI { +func NewCCAPI(req requester.Requester, pollingInterval time.Duration, instanceTimeout time.Duration) CCAPI { return CCAPI{ requester: req, pollingInterval: pollingInterval, + instanceTimeout: instanceTimeout, } } diff --git a/internal/ccapi/service_instances_test.go b/internal/ccapi/service_instances_test.go index 296c3a1..9cab4a0 100644 --- a/internal/ccapi/service_instances_test.go +++ b/internal/ccapi/service_instances_test.go @@ -24,7 +24,7 @@ var _ = Describe("GetServiceInstances", func() { fakeServer = ghttp.NewServer() DeferCleanup(fakeServer.Close) req = requester.NewRequester(fakeServer.URL(), "fake-token", false) - ccapiClient = ccapi.NewCCAPI(req, time.Millisecond) + ccapiClient = ccapi.NewCCAPI(req, time.Millisecond, 10*time.Minute) }) When("service instances exist in the given plans", func() { diff --git a/internal/ccapi/service_plans_test.go b/internal/ccapi/service_plans_test.go index a8bb8d7..a947061 100644 --- a/internal/ccapi/service_plans_test.go +++ b/internal/ccapi/service_plans_test.go @@ -25,7 +25,7 @@ var _ = Describe("GetServicePlans", func() { fakeServer = ghttp.NewServer() DeferCleanup(fakeServer.Close) req = requester.NewRequester(fakeServer.URL(), "fake-token", false) - ccapiClient = ccapi.NewCCAPI(req, time.Millisecond) + ccapiClient = ccapi.NewCCAPI(req, time.Millisecond, 10*time.Minute) }) When("Given a valid brokername", func() { diff --git a/internal/ccapi/upgrade_instance.go b/internal/ccapi/upgrade_instance.go index 86ee77b..a43d090 100644 --- a/internal/ccapi/upgrade_instance.go +++ b/internal/ccapi/upgrade_instance.go @@ -17,7 +17,7 @@ func (c CCAPI) UpgradeServiceInstance(guid, miVersion string) error { return fmt.Errorf("upgrade request error: %s", err) } - for timeout := time.After(time.Minute * 10); ; { + for timeout := time.After(c.instanceTimeout); ; { select { case <-timeout: return fmt.Errorf("error upgrade request timeout") diff --git a/internal/ccapi/upgrade_instance_test.go b/internal/ccapi/upgrade_instance_test.go index f0a61da..7ac8019 100644 --- a/internal/ccapi/upgrade_instance_test.go +++ b/internal/ccapi/upgrade_instance_test.go @@ -87,7 +87,7 @@ var _ = Describe("UpgradeServiceInstance", func() { fakeServer = ghttp.NewServer() DeferCleanup(fakeServer.Close) req = requester.NewRequester(fakeServer.URL(), "fake-token", false) - ccapiClient = ccapi.NewCCAPI(req, time.Millisecond) + ccapiClient = ccapi.NewCCAPI(req, time.Millisecond, 10*time.Minute) }) When("given an upgradeable instance", func() { @@ -217,7 +217,7 @@ var _ = Describe("UpgradeServiceInstance", func() { ), ) - ccapiClient = ccapi.NewCCAPI(req, interval) + ccapiClient = ccapi.NewCCAPI(req, interval, 10*time.Minute) const accuracy = 25 * time.Millisecond start := time.Now() @@ -229,4 +229,67 @@ var _ = Describe("UpgradeServiceInstance", func() { // big enough that we can see a change of behavior from above, but small enough that tests are still snappy Entry("moderate interval", 100*time.Millisecond), ) + + When("the upgrade times out", func() { + BeforeEach(func() { + fakeServer.AppendHandlers( + ghttp.CombineHandlers( + ghttp.VerifyHeaderKV("Authorization", "fake-token"), + ghttp.VerifyRequest("PATCH", "/v3/service_instances/test-guid"), + ghttp.VerifyBody([]byte(`{"maintenance_info":{"version":"test-mi-version"}}`)), + ghttp.RespondWith(http.StatusAccepted, ``, nil), + ), + ) + // Keep returning "in progress" indefinitely + fakeServer.RouteToHandler("GET", "/v3/service_instances/test-guid", + ghttp.CombineHandlers( + ghttp.VerifyHeaderKV("Authorization", "fake-token"), + ghttp.RespondWith(http.StatusOK, instanceUpdatingResponse, nil), + ), + ) + }) + + It("returns a timeout error", func() { + // Use a very short timeout for the test + const timeout = 10 * time.Millisecond + ccapiClient = ccapi.NewCCAPI(req, time.Millisecond, timeout) + + err := ccapiClient.UpgradeServiceInstance("test-guid", "test-mi-version") + Expect(err).To(MatchError("error upgrade request timeout")) + }) + }) + + When("a custom timeout is configured", func() { + BeforeEach(func() { + fakeServer.AppendHandlers( + ghttp.CombineHandlers( + ghttp.VerifyHeaderKV("Authorization", "fake-token"), + ghttp.VerifyRequest("PATCH", "/v3/service_instances/test-guid"), + ghttp.VerifyBody([]byte(`{"maintenance_info":{"version":"test-mi-version"}}`)), + ghttp.RespondWith(http.StatusAccepted, ``, nil), + ), + ) + // Keep returning "in progress" indefinitely + fakeServer.RouteToHandler("GET", "/v3/service_instances/test-guid", + ghttp.CombineHandlers( + ghttp.VerifyHeaderKV("Authorization", "fake-token"), + ghttp.RespondWith(http.StatusOK, instanceUpdatingResponse, nil), + ), + ) + }) + + It("respects the configured timeout", func() { + // Use a custom timeout + const customTimeout = 50 * time.Millisecond + ccapiClient = ccapi.NewCCAPI(req, time.Millisecond, customTimeout) + + const accuracy = 25 * time.Millisecond + start := time.Now() + err := ccapiClient.UpgradeServiceInstance("test-guid", "test-mi-version") + duration := time.Since(start) + + Expect(err).To(MatchError("error upgrade request timeout")) + Expect(duration).To(BeNumerically("~", customTimeout, accuracy)) + }) + }) }) diff --git a/internal/config/config.go b/internal/config/config.go index 9f7109c..922c4cf 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -29,6 +29,7 @@ type Config struct { RetryInterval time.Duration IgnoreInstanceErrors bool InstancePollingInterval time.Duration + InstanceTimeout time.Duration } // ParseConfig combines and validates data from the command line and CLIConnection object @@ -54,6 +55,7 @@ func ParseConfig(conn CLIConnection, args []string) (Config, error) { flagSet.DurationVar(&cfg.RetryInterval, retryIntervalFlag, retryIntervalDefault, retryIntervalDescription) flagSet.BoolVar(&cfg.IgnoreInstanceErrors, ignoreInstanceErrorsFlag, ignoreInstanceErrorsDefault, ignoreInstanceErrorsDescription) flagSet.DurationVar(&cfg.InstancePollingInterval, instancePollingIntervalFlag, instancePollingIntervalDefault, instancePollingIntervalDescription) + flagSet.DurationVar(&cfg.InstanceTimeout, instanceTimeoutFlag, instanceTimeoutDefault, instanceTimeoutDescription) // This ranges over a chain of functions, each of which performs a single action and may return an error. // The chain breaks at the first error received. It arguably reads better than repetitive error handling logic. @@ -82,6 +84,7 @@ func ParseConfig(conn CLIConnection, args []string) (Config, error) { func() error { return validateAttempts(cfg.Attempts) }, func() error { return validateRetryInterval(cfg.RetryInterval) }, func() error { return validateInstancePollingInterval(cfg.InstancePollingInterval) }, + func() error { return validateInstanceTimeout(cfg.InstanceTimeout) }, } { if err := s(); err != nil { return Config{}, err diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 662ffa5..c2ff85f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -3,6 +3,7 @@ package config_test import ( "fmt" "strings" + "time" "upgrade-all-services-cli-plugin/internal/config" "upgrade-all-services-cli-plugin/internal/config/configfakes" @@ -525,4 +526,44 @@ var _ = Describe("Config", func() { }) }) }) + + Describe("-instance-timeout", func() { + Context("default value", func() { + It("is set to 10 minutes", func() { + Expect(cfgErr).NotTo(HaveOccurred()) + Expect(cfg.InstanceTimeout).To(Equal(10 * time.Minute)) + }) + }) + + Context("custom value", func() { + BeforeEach(func() { + fakeArgs = append(fakeArgs, "-instance-timeout", "5m") + }) + + It("uses the custom value", func() { + Expect(cfgErr).NotTo(HaveOccurred()) + Expect(cfg.InstanceTimeout).To(Equal(5 * time.Minute)) + }) + }) + + Context("too low", func() { + BeforeEach(func() { + fakeArgs = append(fakeArgs, "-instance-timeout", "500ms") + }) + + It("returns an error", func() { + Expect(cfgErr).To(MatchError(`instance timeout must be greater or equal to 1s`)) + }) + }) + + Context("too high", func() { + BeforeEach(func() { + fakeArgs = append(fakeArgs, "-instance-timeout", "25h") + }) + + It("returns an error", func() { + Expect(cfgErr).To(MatchError(`instance timeout must be less than or equal to 24h0m0s`)) + }) + }) + }) }) diff --git a/internal/config/flags.go b/internal/config/flags.go index 4f380ce..ceccf3b 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -59,4 +59,10 @@ const ( instancePollingIntervalDescription = "polling interval for service instances during the upgrade process. Default is 10s" instancePollingIntervalMinimum = time.Millisecond instancePollingIntervalMaximum = time.Minute + + instanceTimeoutDefault = 10 * time.Minute + instanceTimeoutFlag = "instance-timeout" + instanceTimeoutDescription = "timeout for service instance upgrade operations. Default is 10m" + instanceTimeoutMinimum = time.Second + instanceTimeoutMaximum = 24 * time.Hour ) diff --git a/internal/config/usage.go b/internal/config/usage.go index 78631e5..5e39480 100644 --- a/internal/config/usage.go +++ b/internal/config/usage.go @@ -22,6 +22,7 @@ func UsageOptions() map[string]string { attemptsFlag: attemptsDescription, retryIntervalFlag: retryIntervalDescription, instancePollingIntervalFlag: instancePollingIntervalDescription, + instanceTimeoutFlag: instanceTimeoutDescription, ignoreInstanceErrorsFlag: ignoreInstanceErrorsDescription, } } diff --git a/internal/config/validations.go b/internal/config/validations.go index c5372d4..f643f6e 100644 --- a/internal/config/validations.go +++ b/internal/config/validations.go @@ -135,3 +135,14 @@ func validateInstancePollingInterval(interval time.Duration) error { return nil } } + +func validateInstanceTimeout(timeout time.Duration) error { + switch { + case timeout > instanceTimeoutMaximum: + return fmt.Errorf("instance timeout must be less than or equal to %s", instanceTimeoutMaximum) + case timeout < instanceTimeoutMinimum: + return fmt.Errorf("instance timeout must be greater or equal to %s", instanceTimeoutMinimum) + default: + return nil + } +} diff --git a/upgrade_all.go b/upgrade_all.go index fbcb7e2..2ce0da3 100644 --- a/upgrade_all.go +++ b/upgrade_all.go @@ -39,7 +39,7 @@ func upgradeAllServices(cliConnection plugin.CliConnection, args []string) int { reqr.Logger = logr } - err = upgrader.Upgrade(ccapi.NewCCAPI(reqr, cfg.InstancePollingInterval), logr, upgrader.UpgradeConfig{ + err = upgrader.Upgrade(ccapi.NewCCAPI(reqr, cfg.InstancePollingInterval, cfg.InstanceTimeout), logr, upgrader.UpgradeConfig{ BrokerName: cfg.BrokerName, ParallelUpgrades: cfg.ParallelUpgrades, Action: cfg.Action,