From 5938803f1e23297facf1c2f6d1178a90137ae5a6 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Thu, 29 Feb 2024 08:56:21 -0500 Subject: [PATCH 1/6] Remove lunar. 23.04 reached end of support on 25 January 2024. --- core/series/supportedseries_linux_test.go | 2 +- core/series/supportedseries_test.go | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/core/series/supportedseries_linux_test.go b/core/series/supportedseries_linux_test.go index 490c134268c..b733bdfa48b 100644 --- a/core/series/supportedseries_linux_test.go +++ b/core/series/supportedseries_linux_test.go @@ -80,7 +80,7 @@ func (s *SupportedSeriesLinuxSuite) TestWorkloadSeries(c *gc.C) { series, err := WorkloadSeries(time.Time{}, "", "") c.Assert(err, jc.ErrorIsNil) c.Assert(series.SortedValues(), gc.DeepEquals, []string{ - "bionic", "centos7", "centos8", "centos9", "focal", "genericlinux", "jammy", "kubernetes", "lunar", + "bionic", "centos7", "centos8", "centos9", "focal", "genericlinux", "jammy", "kubernetes", "mantic", "opensuseleap", "trusty", "win10", "win2008r2", "win2012", "win2012hv", "win2012hvr2", "win2012r2", "win2016", "win2016hv", "win2016nano", "win2019", "win7", "win8", "win81", "xenial"}) diff --git a/core/series/supportedseries_test.go b/core/series/supportedseries_test.go index 0de437dd4e6..2dda7bc5088 100644 --- a/core/series/supportedseries_test.go +++ b/core/series/supportedseries_test.go @@ -37,10 +37,10 @@ func (s *SupportedSeriesSuite) TestSeriesForTypes(c *gc.C) { c.Assert(err, jc.ErrorIsNil) ctrlSeries := info.controllerSeries() - c.Assert(ctrlSeries, jc.DeepEquals, []string{"mantic", "lunar", "jammy", "focal", "bionic", "xenial", "trusty"}) + c.Assert(ctrlSeries, jc.DeepEquals, []string{"mantic", "jammy", "focal", "bionic", "xenial", "trusty"}) wrkSeries := info.workloadSeries(false) - c.Assert(wrkSeries, jc.DeepEquals, []string{"mantic", "lunar", "jammy", "focal", "bionic", "xenial", "trusty", "centos7", "centos8", "centos9", "genericlinux", "kubernetes", "opensuseleap", "win10", "win2008r2", "win2012", "win2012hv", "win2012hvr2", "win2012r2", "win2016", "win2016hv", "win2016nano", "win2019", "win7", "win8", "win81"}) + c.Assert(wrkSeries, jc.DeepEquals, []string{"mantic", "jammy", "focal", "bionic", "xenial", "trusty", "centos7", "centos8", "centos9", "genericlinux", "kubernetes", "opensuseleap", "win10", "win2008r2", "win2012", "win2012hv", "win2012hvr2", "win2012r2", "win2016", "win2016hv", "win2016nano", "win2019", "win7", "win8", "win81"}) } func (s *SupportedSeriesSuite) TestSeriesForTypesUsingImageStream(c *gc.C) { @@ -53,10 +53,10 @@ func (s *SupportedSeriesSuite) TestSeriesForTypesUsingImageStream(c *gc.C) { c.Assert(err, jc.ErrorIsNil) ctrlSeries := info.controllerSeries() - c.Assert(ctrlSeries, jc.DeepEquals, []string{"mantic", "lunar", "jammy", "focal", "bionic", "xenial", "trusty"}) + c.Assert(ctrlSeries, jc.DeepEquals, []string{"mantic", "jammy", "focal", "bionic", "xenial", "trusty"}) wrkSeries := info.workloadSeries(false) - c.Assert(wrkSeries, jc.DeepEquals, []string{"mantic", "lunar", "jammy", "focal", "bionic", "xenial", "trusty", "centos7", "centos8", "centos9", "genericlinux", "kubernetes", "opensuseleap", "win10", "win2008r2", "win2012", "win2012hv", "win2012hvr2", "win2012r2", "win2016", "win2016hv", "win2016nano", "win2019", "win7", "win8", "win81"}) + c.Assert(wrkSeries, jc.DeepEquals, []string{"mantic", "jammy", "focal", "bionic", "xenial", "trusty", "centos7", "centos8", "centos9", "genericlinux", "kubernetes", "opensuseleap", "win10", "win2008r2", "win2012", "win2012hv", "win2012hvr2", "win2012r2", "win2016", "win2016hv", "win2016nano", "win2019", "win7", "win8", "win81"}) } func (s *SupportedSeriesSuite) TestSeriesForTypesUsingInvalidImageStream(c *gc.C) { @@ -69,10 +69,10 @@ func (s *SupportedSeriesSuite) TestSeriesForTypesUsingInvalidImageStream(c *gc.C c.Assert(err, jc.ErrorIsNil) ctrlSeries := info.controllerSeries() - c.Assert(ctrlSeries, jc.DeepEquals, []string{"mantic", "lunar", "jammy", "focal", "bionic", "xenial", "trusty"}) + c.Assert(ctrlSeries, jc.DeepEquals, []string{"mantic", "jammy", "focal", "bionic", "xenial", "trusty"}) wrkSeries := info.workloadSeries(false) - c.Assert(wrkSeries, jc.DeepEquals, []string{"mantic", "lunar", "jammy", "focal", "bionic", "xenial", "trusty", "centos7", "centos8", "centos9", "genericlinux", "kubernetes", "opensuseleap", "win10", "win2008r2", "win2012", "win2012hv", "win2012hvr2", "win2012r2", "win2016", "win2016hv", "win2016nano", "win2019", "win7", "win8", "win81"}) + c.Assert(wrkSeries, jc.DeepEquals, []string{"mantic", "jammy", "focal", "bionic", "xenial", "trusty", "centos7", "centos8", "centos9", "genericlinux", "kubernetes", "opensuseleap", "win10", "win2008r2", "win2012", "win2012hv", "win2012hvr2", "win2012r2", "win2016", "win2016hv", "win2016nano", "win2019", "win7", "win8", "win81"}) } func (s *SupportedSeriesSuite) TestSeriesForTypesUsingInvalidSeries(c *gc.C) { @@ -85,10 +85,10 @@ func (s *SupportedSeriesSuite) TestSeriesForTypesUsingInvalidSeries(c *gc.C) { c.Assert(err, jc.ErrorIsNil) ctrlSeries := info.controllerSeries() - c.Assert(ctrlSeries, jc.DeepEquals, []string{"mantic", "lunar", "jammy", "focal", "bionic", "xenial", "trusty"}) + c.Assert(ctrlSeries, jc.DeepEquals, []string{"mantic", "jammy", "focal", "bionic", "xenial", "trusty"}) wrkSeries := info.workloadSeries(false) - c.Assert(wrkSeries, jc.DeepEquals, []string{"mantic", "lunar", "jammy", "focal", "bionic", "xenial", "trusty", "centos7", "centos8", "centos9", "genericlinux", "kubernetes", "opensuseleap", "win10", "win2008r2", "win2012", "win2012hv", "win2012hvr2", "win2012r2", "win2016", "win2016hv", "win2016nano", "win2019", "win7", "win8", "win81"}) + c.Assert(wrkSeries, jc.DeepEquals, []string{"mantic", "jammy", "focal", "bionic", "xenial", "trusty", "centos7", "centos8", "centos9", "genericlinux", "kubernetes", "opensuseleap", "win10", "win2008r2", "win2012", "win2012hv", "win2012hvr2", "win2012r2", "win2016", "win2016hv", "win2016nano", "win2019", "win7", "win8", "win81"}) } var getOSFromSeriesTests = []struct { From c1888588a2d5e00643af5c5d8a67fb7f200c9d69 Mon Sep 17 00:00:00 2001 From: Harry Pidcock Date: Thu, 1 Feb 2024 11:56:30 +1000 Subject: [PATCH 2/6] Have podman build append to manifest when building images. --- make_functions.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/make_functions.sh b/make_functions.sh index 5fd71c46902..53f6cef0a3f 100755 --- a/make_functions.sh +++ b/make_functions.sh @@ -135,14 +135,16 @@ build_push_operator_image() { ${output} \ "${BUILD_DIR}" elif [[ "${OCI_BUILDER}" = "podman" ]]; then + "$DOCKER_BIN" manifest rm "$(operator_image_path)" || true + "$DOCKER_BIN" manifest create "$(operator_image_path)" "$DOCKER_BIN" build \ --jobs "4" \ -f "${WORKDIR}/Dockerfile" \ - -t "$(operator_image_path)" \ + --manifest "$(operator_image_path)" \ --platform="$build_multi_osarch" \ "${BUILD_DIR}" if [[ "$push_image" = true ]]; then - "$DOCKER_BIN" push "$(operator_image_path)" + "$DOCKER_BIN" manifest push "$(operator_image_path)" "docker://$(operator_image_path)" fi else echo "unknown OCI_BUILDER=${OCI_BUILDER} expected docker or podman" From 9160e936e39ff81c55576956046b5eab3ddbe355 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Thu, 29 Feb 2024 13:53:05 +0000 Subject: [PATCH 3/6] GOCOVERDIR causes tests to fail Some tests actually invoke the go tooling to run a test, which causes a failure as we now set juju to run with go coverage. We want to run code coverage so that we can push that information to tiobe. Unfortunately, GOCOVERDIR outputs a warning if the environment variable is not set. This warning is then caught by the strict equality output we're looking for on stdout/stderr. Note: these tests should be deprecated going forward. --- cmd/jujuc/main_test.go | 4 +++- cmd/jujud/main_test.go | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/cmd/jujuc/main_test.go b/cmd/jujuc/main_test.go index ffaa701df0a..79a16b2c8fc 100644 --- a/cmd/jujuc/main_test.go +++ b/cmd/jujuc/main_test.go @@ -106,6 +106,8 @@ func run(c *gc.C, sockPath sockets.Socket, contextId string, exit int, stdin []b fmt.Sprintf("JUJU_AGENT_SOCKET_ADDRESS=%s", sockPath.Address), fmt.Sprintf("JUJU_AGENT_SOCKET_NETWORK=%s", sockPath.Network), fmt.Sprintf("JUJU_CONTEXT_ID=%s", contextId), + // See: https://go.dev/doc/build-cover + fmt.Sprintf("GOCOVERDIR=%s", ps.Dir), // Code that imports github.com/juju/juju/testing needs to // be able to find that module at runtime (via build.Import), // so we have to preserve that env variable. @@ -225,5 +227,5 @@ func (s *HookToolMainSuite) TestStdin(c *gc.C) { c.Skip("issue 1403084: test panics on CryptAcquireContext on windows") } output := run(c, s.sockPath, "bill", 0, []byte("some standard input"), "remote") - c.Assert(output, gc.Equals, "some standard input") + c.Assert(output, jc.Contains, "some standard input") } diff --git a/cmd/jujud/main_test.go b/cmd/jujud/main_test.go index 96dc50bf65a..a391c07376b 100644 --- a/cmd/jujud/main_test.go +++ b/cmd/jujud/main_test.go @@ -158,6 +158,8 @@ func runForTest(c *gc.C, sockPath sockets.Socket, contextId string, exit int, st fmt.Sprintf("JUJU_AGENT_SOCKET_ADDRESS=%s", sockPath.Address), fmt.Sprintf("JUJU_AGENT_SOCKET_NETWORK=%s", sockPath.Network), fmt.Sprintf("JUJU_CONTEXT_ID=%s", contextId), + // See: https://go.dev/doc/build-cover + fmt.Sprintf("GOCOVERDIR=%s", ps.Dir), // Code that imports github.com/juju/juju/testing needs to // be able to find that module at runtime (via build.Import), // so we have to preserve that env variable. @@ -277,5 +279,5 @@ func (s *HookToolMainSuite) TestStdin(c *gc.C) { c.Skip("issue 1403084: test panics on CryptAcquireContext on windows") } output := runForTest(c, s.sockPath, "bill", 0, []byte("some standard input"), "remote") - c.Assert(output, gc.Equals, "some standard input") + c.Assert(output, jc.Contains, "some standard input") } From d834242e63677f46eac26cb5903843c489cb0d53 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Tue, 27 Feb 2024 10:10:41 -0500 Subject: [PATCH 4/6] Use check not assert when testing via a slice of structures. TestConfig would fail on the first minor failure rather than running all of the tests. Using check rather than assert allows for all of the failures to be surfaced at once rather than going thru them individually. --- environs/config/config_test.go | 100 +++++++++++++++++---------------- 1 file changed, 52 insertions(+), 48 deletions(-) diff --git a/environs/config/config_test.go b/environs/config/config_test.go index 39ce1d7445c..24931a9794c 100644 --- a/environs/config/config_test.go +++ b/environs/config/config_test.go @@ -657,10 +657,14 @@ func (test configTest) check(c *gc.C) { cfg, err := config.New(test.useDefaults, test.attrs) if test.err != "" { c.Check(cfg, gc.IsNil) - c.Assert(err, gc.ErrorMatches, test.err) + c.Check(err, gc.ErrorMatches, test.err) + return + } + c.Check(err, jc.ErrorIsNil, gc.Commentf("config.New failed")) + if err != nil { + // As we have a Check not an Assert so the test return } - c.Assert(err, jc.ErrorIsNil) typ, _ := test.attrs["type"].(string) // "null" has been deprecated in favour of "manual", @@ -669,104 +673,104 @@ func (test configTest) check(c *gc.C) { typ = "manual" } name, _ := test.attrs["name"].(string) - c.Assert(cfg.Type(), gc.Equals, typ) - c.Assert(cfg.Name(), gc.Equals, name) + c.Check(cfg.Type(), gc.Equals, typ) + c.Check(cfg.Name(), gc.Equals, name) agentVersion, ok := cfg.AgentVersion() if s := test.attrs["agent-version"]; s != nil { - c.Assert(ok, jc.IsTrue) - c.Assert(agentVersion, gc.Equals, version.MustParse(s.(string))) + c.Check(ok, jc.IsTrue) + c.Check(agentVersion, gc.Equals, version.MustParse(s.(string))) } else { - c.Assert(ok, jc.IsFalse) - c.Assert(agentVersion, gc.Equals, version.Zero) + c.Check(ok, jc.IsFalse) + c.Check(agentVersion, gc.Equals, version.Zero) } if expected, ok := test.attrs["uuid"]; ok { - c.Assert(cfg.UUID(), gc.Equals, expected) + c.Check(cfg.UUID(), gc.Equals, expected) } dev, _ := test.attrs["development"].(bool) - c.Assert(cfg.Development(), gc.Equals, dev) + c.Check(cfg.Development(), gc.Equals, dev) seriesAttr, _ := test.attrs["default-series"].(string) defaultSeries, ok := cfg.DefaultSeries() if seriesAttr != "" { - c.Assert(ok, jc.IsTrue) - c.Assert(defaultSeries, gc.Equals, seriesAttr) + c.Check(ok, jc.IsTrue) + c.Check(defaultSeries, gc.Equals, seriesAttr) } else { - c.Assert(ok, jc.IsFalse) - c.Assert(defaultSeries, gc.Equals, "") + c.Check(ok, jc.IsFalse) + c.Check(defaultSeries, gc.Equals, "") } if m, _ := test.attrs["firewall-mode"].(string); m != "" { - c.Assert(cfg.FirewallMode(), gc.Equals, m) + c.Check(cfg.FirewallMode(), gc.Equals, m) } if m, _ := test.attrs["default-space"].(string); m != "" { - c.Assert(cfg.DefaultSpace(), gc.Equals, m) + c.Check(cfg.DefaultSpace(), gc.Equals, m) } keys, _ := test.attrs["authorized-keys"].(string) - c.Assert(cfg.AuthorizedKeys(), gc.Equals, keys) + c.Check(cfg.AuthorizedKeys(), gc.Equals, keys) lfCfg, hasLogCfg := cfg.LogFwdSyslog() if v, ok := test.attrs["logforward-enabled"].(bool); ok { - c.Assert(hasLogCfg, jc.IsTrue) - c.Assert(lfCfg.Enabled, gc.Equals, v) + c.Check(hasLogCfg, jc.IsTrue) + c.Check(lfCfg.Enabled, gc.Equals, v) } if v, ok := test.attrs["syslog-ca-cert"].(string); v != "" { - c.Assert(hasLogCfg, jc.IsTrue) - c.Assert(lfCfg.CACert, gc.Equals, v) + c.Check(hasLogCfg, jc.IsTrue) + c.Check(lfCfg.CACert, gc.Equals, v) } else if ok { - c.Assert(hasLogCfg, jc.IsTrue) + c.Check(hasLogCfg, jc.IsTrue) c.Check(lfCfg.CACert, gc.Equals, "") } if v, ok := test.attrs["syslog-client-cert"].(string); v != "" { - c.Assert(hasLogCfg, jc.IsTrue) - c.Assert(lfCfg.ClientCert, gc.Equals, v) + c.Check(hasLogCfg, jc.IsTrue) + c.Check(lfCfg.ClientCert, gc.Equals, v) } else if ok { - c.Assert(hasLogCfg, jc.IsTrue) + c.Check(hasLogCfg, jc.IsTrue) c.Check(lfCfg.ClientCert, gc.Equals, "") } if v, ok := test.attrs["syslog-client-key"].(string); v != "" { - c.Assert(hasLogCfg, jc.IsTrue) - c.Assert(lfCfg.ClientKey, gc.Equals, v) + c.Check(hasLogCfg, jc.IsTrue) + c.Check(lfCfg.ClientKey, gc.Equals, v) } else if ok { - c.Assert(hasLogCfg, jc.IsTrue) + c.Check(hasLogCfg, jc.IsTrue) c.Check(lfCfg.ClientKey, gc.Equals, "") } if v, ok := test.attrs["ssl-hostname-verification"]; ok { - c.Assert(cfg.SSLHostnameVerification(), gc.Equals, v) + c.Check(cfg.SSLHostnameVerification(), gc.Equals, v) } if v, ok := test.attrs["provisioner-harvest-mode"]; ok { harvestMeth, err := config.ParseHarvestMode(v.(string)) - c.Assert(err, jc.ErrorIsNil) - c.Assert(cfg.ProvisionerHarvestMode(), gc.Equals, harvestMeth) + c.Check(err, jc.ErrorIsNil) + c.Check(cfg.ProvisionerHarvestMode(), gc.Equals, harvestMeth) } else { - c.Assert(cfg.ProvisionerHarvestMode(), gc.Equals, config.HarvestDestroyed) + c.Check(cfg.ProvisionerHarvestMode(), gc.Equals, config.HarvestDestroyed) } if v, ok := test.attrs["image-stream"]; ok { - c.Assert(cfg.ImageStream(), gc.Equals, v) + c.Check(cfg.ImageStream(), gc.Equals, v) } else { - c.Assert(cfg.ImageStream(), gc.Equals, "released") + c.Check(cfg.ImageStream(), gc.Equals, "released") } url, urlPresent := cfg.ImageMetadataURL() if v, _ := test.attrs["image-metadata-url"].(string); v != "" { - c.Assert(url, gc.Equals, v) - c.Assert(urlPresent, jc.IsTrue) + c.Check(url, gc.Equals, v) + c.Check(urlPresent, jc.IsTrue) } else { - c.Assert(urlPresent, jc.IsFalse) + c.Check(urlPresent, jc.IsFalse) } agentURL, urlPresent := cfg.AgentMetadataURL() expectedToolsURLValue := test.attrs["agent-metadata-url"] if urlPresent { - c.Assert(agentURL, gc.Equals, expectedToolsURLValue) + c.Check(agentURL, gc.Equals, expectedToolsURLValue) } else { - c.Assert(agentURL, gc.Equals, "") + c.Check(agentURL, gc.Equals, "") } // assertions for deprecated tools-stream attribute used with new agent-stream @@ -787,33 +791,33 @@ func (test configTest) check(c *gc.C) { containerURL, urlPresent := cfg.ContainerImageMetadataURL() if v, _ := test.attrs["container-image-metadata-url"].(string); v != "" { - c.Assert(containerURL, gc.Equals, v) - c.Assert(urlPresent, jc.IsTrue) + c.Check(containerURL, gc.Equals, v) + c.Check(urlPresent, jc.IsTrue) } else { - c.Assert(urlPresent, jc.IsFalse) + c.Check(urlPresent, jc.IsFalse) } if v, ok := test.attrs["container-image-stream"]; ok { - c.Assert(cfg.ContainerImageStream(), gc.Equals, v) + c.Check(cfg.ContainerImageStream(), gc.Equals, v) } else { - c.Assert(cfg.ContainerImageStream(), gc.Equals, "released") + c.Check(cfg.ContainerImageStream(), gc.Equals, "released") } resourceTags, cfgHasResourceTags := cfg.ResourceTags() - c.Assert(cfgHasResourceTags, jc.IsTrue) + c.Check(cfgHasResourceTags, jc.IsTrue) if tags, ok := test.attrs["resource-tags"]; ok { switch tags := tags.(type) { case []string: if len(tags) > 0 { - c.Assert(resourceTags, jc.DeepEquals, testResourceTagsMap) + c.Check(resourceTags, jc.DeepEquals, testResourceTagsMap) } case string: if tags != "" { - c.Assert(resourceTags, jc.DeepEquals, testResourceTagsMap) + c.Check(resourceTags, jc.DeepEquals, testResourceTagsMap) } } } else { - c.Assert(resourceTags, gc.HasLen, 0) + c.Check(resourceTags, gc.HasLen, 0) } xmit := cfg.TransmitVendorMetrics() From 103bd827f41899d03db915421cef3bc8231762c0 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Tue, 27 Feb 2024 10:16:39 -0500 Subject: [PATCH 5/6] Add max values to NumProvisionWorkers and NumContainerProvisionWorkers. Theoretically any user with model access could bring the controller to a standstill by setting the number of provisioner workers to an extremely large value. Set a sane value as a cap. The values are larger than the default but not back by solid data today. We do know that 16 workers for containers will cause issues with LXD on some machines. In the future we should allow the controller config to define what the max values should be. Today controller config has no linkage with model config to enable this. A question would be whether existing model config should be changed if the max is changed. --- environs/config/config.go | 43 ++++++++++++++++++++++++-- environs/config/config_test.go | 28 +++++++++++++---- state/modelconfig_test.go | 4 +-- worker/provisioner/provisioner_test.go | 2 +- 4 files changed, 66 insertions(+), 11 deletions(-) diff --git a/environs/config/config.go b/environs/config/config.go index 87dcb89348b..1ddc6f9953a 100644 --- a/environs/config/config.go +++ b/environs/config/config.go @@ -816,6 +816,14 @@ func Validate(cfg, old *Config) error { return errors.Trace(err) } + if err := cfg.validateNumProvisionWorkers(); err != nil { + return errors.Trace(err) + } + + if err := cfg.validateNumContainerProvisionWorkers(); err != nil { + return errors.Trace(err) + } + if old != nil { // Check the immutable config values. These can't change for _, attr := range immutableAttributes { @@ -1335,6 +1343,24 @@ func (c *Config) NumProvisionWorkers() int { return value } +const ( + MaxNumProvisionWorkers = 100 + MaxNumContainerProvisionWorkers = 25 +) + +// validateNumProvisionWorkers ensures the number cannot be set to +// more than 100. +// TODO: (hml) 26-Feb-2024 +// Once we can better link the controller config and the model config, +// allow the max value to be set in the controller config. +func (c *Config) validateNumProvisionWorkers() error { + value, ok := c.defined[NumProvisionWorkersKey].(int) + if ok && value > MaxNumProvisionWorkers { + return errors.Errorf("%s: must be less than %d", NumProvisionWorkersKey, MaxNumProvisionWorkers) + } + return nil +} + // NumContainerProvisionWorkers returns the number of container provisioner // workers to use. func (c *Config) NumContainerProvisionWorkers() int { @@ -1342,6 +1368,19 @@ func (c *Config) NumContainerProvisionWorkers() int { return value } +// validateNumContainerProvisionWorkers ensures the number cannot be set to +// more than 25. +// TODO: (hml) 26-Feb-2024 +// Once we can better link the controller config and the model config, +// allow the max value to be set in the controller config. +func (c *Config) validateNumContainerProvisionWorkers() error { + value, ok := c.defined[NumContainerProvisionWorkersKey].(int) + if ok && value > MaxNumContainerProvisionWorkers { + return errors.Errorf("%s: must be less than %d", NumContainerProvisionWorkersKey, MaxNumContainerProvisionWorkers) + } + return nil +} + // ImageStream returns the simplestreams stream // used to identify which image ids to search // when starting an instance. @@ -1355,7 +1394,7 @@ func (c *Config) ImageStream() string { // AgentStream returns the simplestreams stream // used to identify which tools to use when -// when bootstrapping or upgrading an environment. +// bootstrapping or upgrading an environment. func (c *Config) AgentStream() string { v, _ := c.defined[AgentStreamKey].(string) if v != "" { @@ -1376,7 +1415,7 @@ func (c *Config) ContainerImageStream() string { // GUIStream returns the simplestreams stream // used to identify which gui to use when -// when fetching a gui tarball. +// fetching a gui tarball. func (c *Config) GUIStream() string { v, _ := c.defined[GUIStreamKey].(string) if v != "" { diff --git a/environs/config/config_test.go b/environs/config/config_test.go index 24931a9794c..338a3a0b25a 100644 --- a/environs/config/config_test.go +++ b/environs/config/config_test.go @@ -347,15 +347,31 @@ var configTests = []configTest{ }), err: `provisioner-harvest-mode: expected one of \[all none unknown destroyed], got "yes please"`, }, { - about: fmt.Sprintf( - "%s: %d", - "num-provision-workers", - 42, - ), + about: fmt.Sprintf("num-provision-workers: 42"), + useDefaults: config.UseDefaults, + attrs: minimalConfigAttrs.Merge(testing.Attrs{ + "num-provision-workers": 42, + }), + }, { + about: fmt.Sprintf("num-provision-workers: over max"), + useDefaults: config.UseDefaults, + attrs: minimalConfigAttrs.Merge(testing.Attrs{ + "num-provision-workers": 101, + }), + err: `num-provision-workers: must be less than 100`, + }, { + about: fmt.Sprintf("num-container-provision-workers: 17"), + useDefaults: config.UseDefaults, + attrs: minimalConfigAttrs.Merge(testing.Attrs{ + "num-container-provision-workers": 17, + }), + }, { + about: fmt.Sprintf("num-container-provision-workers: over max"), useDefaults: config.UseDefaults, attrs: minimalConfigAttrs.Merge(testing.Attrs{ - "num-provision-workers": "42", + "num-container-provision-workers": 26, }), + err: `num-container-provision-workers: must be less than 25`, }, { about: "default image stream", useDefaults: config.UseDefaults, diff --git a/state/modelconfig_test.go b/state/modelconfig_test.go index a31203c63e6..bd03f8f4364 100644 --- a/state/modelconfig_test.go +++ b/state/modelconfig_test.go @@ -458,7 +458,7 @@ func (s *ModelConfigSourceSuite) TestUpdateModelConfigDefaults(c *gc.C) { attrs = map[string]interface{}{ "apt-mirror": "http://different-mirror", - "num-provision-workers": 666, + "num-provision-workers": 66, } err = s.State.UpdateModelConfigDefaultValues(attrs, []string{"http-proxy", "https-proxy"}, nil) c.Assert(err, jc.ErrorIsNil) @@ -487,7 +487,7 @@ func (s *ModelConfigSourceSuite) TestUpdateModelConfigDefaults(c *gc.C) { Value: "dummy-proxy", }}} expectedValues["num-provision-workers"] = config.AttributeDefaultValues{ - Controller: 666, + Controller: 66, Default: 16, } c.Assert(cfg, jc.DeepEquals, expectedValues) diff --git a/worker/provisioner/provisioner_test.go b/worker/provisioner/provisioner_test.go index 1d8c2565d78..72671957b10 100644 --- a/worker/provisioner/provisioner_test.go +++ b/worker/provisioner/provisioner_test.go @@ -118,7 +118,7 @@ func (s *CommonProvisionerSuite) assertProvisionerObservesConfigChangesWorkerCou // Switch to reaping on All machines. attrs := map[string]interface{}{} if container { - attrs[config.NumContainerProvisionWorkersKey] = 42 + attrs[config.NumContainerProvisionWorkersKey] = 10 } else { attrs[config.NumProvisionWorkersKey] = 42 } From 88ac91362412a054395b1458ec197bc1d5af8d05 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Wed, 28 Feb 2024 13:55:37 -0500 Subject: [PATCH 6/6] Ensure using Check doesn't lead to nil pointer panic. In configTest.check, ensure we don't do a check on a nil value after the method call has failed. 95% of the methods return a string, bool or int so this isn't a huge problem. Be aware when adding to the test. --- environs/config/config_test.go | 11 ++++++----- worker/provisioner/provisioner_test.go | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/environs/config/config_test.go b/environs/config/config_test.go index 338a3a0b25a..21f1d85b482 100644 --- a/environs/config/config_test.go +++ b/environs/config/config_test.go @@ -676,9 +676,9 @@ func (test configTest) check(c *gc.C) { c.Check(err, gc.ErrorMatches, test.err) return } - c.Check(err, jc.ErrorIsNil, gc.Commentf("config.New failed")) - if err != nil { - // As we have a Check not an Assert so the test + if !c.Check(err, jc.ErrorIsNil, gc.Commentf("config.New failed")) { + // As we have a Check not an Assert so the test should not + // continue from here as it will result in a nil pointer panic. return } @@ -730,8 +730,9 @@ func (test configTest) check(c *gc.C) { lfCfg, hasLogCfg := cfg.LogFwdSyslog() if v, ok := test.attrs["logforward-enabled"].(bool); ok { - c.Check(hasLogCfg, jc.IsTrue) - c.Check(lfCfg.Enabled, gc.Equals, v) + if c.Check(hasLogCfg, jc.IsTrue) { + c.Check(lfCfg.Enabled, gc.Equals, v) + } } if v, ok := test.attrs["syslog-ca-cert"].(string); v != "" { c.Check(hasLogCfg, jc.IsTrue) diff --git a/worker/provisioner/provisioner_test.go b/worker/provisioner/provisioner_test.go index 72671957b10..de14b36186a 100644 --- a/worker/provisioner/provisioner_test.go +++ b/worker/provisioner/provisioner_test.go @@ -136,7 +136,7 @@ func (s *CommonProvisionerSuite) assertProvisionerObservesConfigChangesWorkerCou select { case newCfg := <-cfgObserver: if container { - if newCfg.NumContainerProvisionWorkers() == 42 { + if newCfg.NumContainerProvisionWorkers() == 10 { return } received = append(received, newCfg.NumContainerProvisionWorkers())