From 1c70db28e05013824a25061a210551c3249ea3e4 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Thu, 30 Nov 2023 17:47:25 +0000 Subject: [PATCH 1/7] Implement agent binary strategy We only ever need to upload the agent binary once in the workflow. This allows us to do that by calling out to the binary uploader. The worker will prevent any other dependency engine entities starting and thus we're at liberty to perform the required steps to seed the controller. --- internal/bootstrap/agentbinary.go | 78 +++++++++++++++++++++++++++++++ worker/bootstrap/manifold.go | 68 +++++++++++++++++++++------ worker/bootstrap/manifold_test.go | 8 +++- worker/bootstrap/worker.go | 63 +++++++++++++++++++++---- worker/bootstrap/worker_test.go | 9 +++- 5 files changed, 200 insertions(+), 26 deletions(-) create mode 100644 internal/bootstrap/agentbinary.go diff --git a/internal/bootstrap/agentbinary.go b/internal/bootstrap/agentbinary.go new file mode 100644 index 00000000000..1bc44d8e70b --- /dev/null +++ b/internal/bootstrap/agentbinary.go @@ -0,0 +1,78 @@ +// Copyright 2023 Canonical Ltd. +// Licensed under the LGPLv3, see LICENCE file for details. + +package bootstrap + +import ( + "bytes" + "context" + "fmt" + "io" + "os" + "path/filepath" + + "github.com/juju/errors" + "github.com/juju/version/v2" + + agenttools "github.com/juju/juju/agent/tools" + "github.com/juju/juju/controller" + "github.com/juju/juju/core/arch" + coreos "github.com/juju/juju/core/os" + "github.com/juju/juju/state/binarystorage" + jujuversion "github.com/juju/juju/version" +) + +// Logger represents the logging methods called. +type Logger interface { + Errorf(message string, args ...any) + Warningf(message string, args ...any) + Infof(message string, args ...any) + Debugf(message string, args ...any) +} + +const ( + // AgentCompressedBinaryName is the name of the agent binary. + AgentCompressedBinaryName = "tools.tar.gz" +) + +// AgentBinaryStorage is the interface that is used to store the agent binary. +type AgentBinaryStorage interface { + // Add adds the agent binary to the storage. + Add(context.Context, io.Reader, binarystorage.Metadata) error +} + +// PopulateAgentBinary is the function that is used to populate the agent +// binary at bootstrap. +func PopulateAgentBinary(ctx context.Context, dataDir string, storage AgentBinaryStorage, cfg controller.Config, logger Logger) error { + current := version.Binary{ + Number: jujuversion.Current, + Arch: arch.HostArch(), + Release: coreos.HostOSTypeName(), + } + + agentTools, err := agenttools.ReadTools(dataDir, current) + if err != nil { + return fmt.Errorf("cannot read tools: %w", err) + } + + data, err := os.ReadFile(filepath.Join( + agenttools.SharedToolsDir(dataDir, current), + AgentCompressedBinaryName, + )) + if err != nil { + return errors.Trace(err) + } + + metadata := binarystorage.Metadata{ + Version: agentTools.Version.String(), + Size: agentTools.Size, + SHA256: agentTools.SHA256, + } + + logger.Debugf("Adding agent binary: %v", agentTools.Version) + if err := storage.Add(ctx, bytes.NewReader(data), metadata); err != nil { + return errors.Trace(err) + } + + return nil +} diff --git a/worker/bootstrap/manifold.go b/worker/bootstrap/manifold.go index 4edf7fb0e06..6911c87fd7c 100644 --- a/worker/bootstrap/manifold.go +++ b/worker/bootstrap/manifold.go @@ -4,14 +4,21 @@ package bootstrap import ( + "context" + "io" + "github.com/juju/errors" "github.com/juju/worker/v3" "github.com/juju/worker/v3/dependency" "github.com/juju/juju/agent" + "github.com/juju/juju/controller" "github.com/juju/juju/core/application" "github.com/juju/juju/core/objectstore" + "github.com/juju/juju/internal/bootstrap" + "github.com/juju/juju/internal/servicefactory" st "github.com/juju/juju/state" + "github.com/juju/juju/state/binarystorage" "github.com/juju/juju/worker/common" "github.com/juju/juju/worker/gate" "github.com/juju/juju/worker/state" @@ -25,8 +32,22 @@ type Logger interface { Debugf(message string, args ...any) } +// BinaryAgentStorageService is the interface that is used to get the storage +// for the agent binary. +type BinaryAgentStorageService interface { + AgentBinaryStorage(objectstore.ObjectStore) (BinaryAgentStorage, error) +} + +// BinaryAgentStorage is the interface that is used to store the agent binary. +type BinaryAgentStorage interface { + // Add adds the agent binary to the storage. + Add(context.Context, io.Reader, binarystorage.Metadata) error + // Close closes the storage. + Close() error +} + // AgentBinaryBootstrapFunc is the function that is used to populate the tools. -type AgentBinaryBootstrapFunc func() error +type AgentBinaryBootstrapFunc func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, controller.Config, Logger) error // RequiresBootstrapFunc is the function that is used to check if the controller // application exists. @@ -34,10 +55,11 @@ type RequiresBootstrapFunc func(appService ApplicationService) (bool, error) // ManifoldConfig defines the configuration for the trace manifold. type ManifoldConfig struct { - AgentName string - StateName string - ObjectStoreName string - BootstrapGateName string + AgentName string + StateName string + ObjectStoreName string + BootstrapGateName string + ServiceFactoryName string Logger Logger AgentBinarySeeder AgentBinaryBootstrapFunc @@ -58,6 +80,9 @@ func (cfg ManifoldConfig) Validate() error { if cfg.BootstrapGateName == "" { return errors.NotValidf("empty BootstrapGateName") } + if cfg.ServiceFactoryName == "" { + return errors.NotValidf("empty ServiceFactoryName") + } if cfg.Logger == nil { return errors.NotValidf("nil Logger") } @@ -75,6 +100,7 @@ func Manifold(config ManifoldConfig) dependency.Manifold { config.StateName, config.ObjectStoreName, config.BootstrapGateName, + config.ServiceFactoryName, }, Start: func(context dependency.Context) (worker.Worker, error) { if err := config.Validate(); err != nil { @@ -120,13 +146,20 @@ func Manifold(config ManifoldConfig) dependency.Manifold { return nil, errors.Trace(err) } + var controllerServiceFactory servicefactory.ControllerServiceFactory + if err := context.Get(config.ServiceFactoryName, &controllerServiceFactory); err != nil { + _ = stTracker.Done() + return nil, errors.Trace(err) + } + w, err := NewWorker(WorkerConfig{ - Agent: a, - ObjectStore: objectStore, - State: st, - BootstrapUnlocker: bootstrapUnlocker, - AgentBinarySeeder: config.AgentBinarySeeder, - Logger: config.Logger, + Agent: a, + ObjectStore: objectStore, + ControllerConfigService: controllerServiceFactory.ControllerConfig(), + State: st, + BootstrapUnlocker: bootstrapUnlocker, + AgentBinarySeeder: config.AgentBinarySeeder, + Logger: config.Logger, }) if err != nil { _ = stTracker.Done() @@ -142,14 +175,21 @@ func Manifold(config ManifoldConfig) dependency.Manifold { // CAASAgentBinarySeeder is the function that is used to populate the tools // for CAAS. -func CAASAgentBinarySeeder() error { +func CAASAgentBinarySeeder(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, controller.Config, Logger) error { + // CAAS doesn't need to populate the tools. return nil } // IAASAgentBinarySeeder is the function that is used to populate the tools // for IAAS. -func IAASAgentBinarySeeder() error { - return nil +func IAASAgentBinarySeeder(ctx context.Context, dataDir string, storageService BinaryAgentStorageService, objectStore objectstore.ObjectStore, controllerConfig controller.Config, logger Logger) error { + storage, err := storageService.AgentBinaryStorage(objectStore) + if err != nil { + return errors.Trace(err) + } + defer storage.Close() + + return bootstrap.PopulateAgentBinary(ctx, dataDir, storage, controllerConfig, logger) } // ApplicationService is the interface that is used to check if an application diff --git a/worker/bootstrap/manifold_test.go b/worker/bootstrap/manifold_test.go index 528227d0ee7..0da4895c0e2 100644 --- a/worker/bootstrap/manifold_test.go +++ b/worker/bootstrap/manifold_test.go @@ -4,12 +4,16 @@ package bootstrap import ( + "context" + "github.com/juju/errors" jc "github.com/juju/testing/checkers" "github.com/juju/worker/v3/dependency" dependencytesting "github.com/juju/worker/v3/dependency/testing" gc "gopkg.in/check.v1" + controller "github.com/juju/juju/controller" + "github.com/juju/juju/core/objectstore" state "github.com/juju/juju/state" ) @@ -43,7 +47,9 @@ func (s *manifoldSuite) getConfig() ManifoldConfig { StateName: "state", BootstrapGateName: "bootstrap-gate", Logger: s.logger, - AgentBinarySeeder: func() error { return nil }, + AgentBinarySeeder: func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, controller.Config, Logger) error { + return nil + }, RequiresBootstrap: func(appService ApplicationService) (bool, error) { return false, nil }, } } diff --git a/worker/bootstrap/worker.go b/worker/bootstrap/worker.go index 7485bde4d8d..c8f6ca0020c 100644 --- a/worker/bootstrap/worker.go +++ b/worker/bootstrap/worker.go @@ -4,10 +4,14 @@ package bootstrap import ( + "context" + "fmt" + "github.com/juju/errors" "gopkg.in/tomb.v2" "github.com/juju/juju/agent" + "github.com/juju/juju/controller" "github.com/juju/juju/core/objectstore" "github.com/juju/juju/state" "github.com/juju/juju/worker/gate" @@ -18,13 +22,20 @@ const ( stateStarted = "started" ) +// ControllerConfigService is the interface that is used to get the +// controller configuration. +type ControllerConfigService interface { + ControllerConfig(context.Context) (controller.Config, error) +} + // WorkerConfig encapsulates the configuration options for the // bootstrap worker. type WorkerConfig struct { - Agent agent.Agent - ObjectStore objectstore.ObjectStore - BootstrapUnlocker gate.Unlocker - AgentBinarySeeder AgentBinaryBootstrapFunc + Agent agent.Agent + ObjectStore objectstore.ObjectStore + ControllerConfigService ControllerConfigService + BootstrapUnlocker gate.Unlocker + AgentBinarySeeder AgentBinaryBootstrapFunc // Deprecated: This is only here, until we can remove the state layer. State *state.State @@ -40,6 +51,9 @@ func (c *WorkerConfig) Validate() error { if c.ObjectStore == nil { return errors.NotValidf("nil ObjectStore") } + if c.ControllerConfigService == nil { + return errors.NotValidf("nil ControllerConfigService") + } if c.BootstrapUnlocker == nil { return errors.NotValidf("nil BootstrapUnlocker") } @@ -95,11 +109,23 @@ func (w *bootstrapWorker) loop() error { // Report the initial started state. w.reportInternalState(stateStarted) - // Perform the bootstrap. - // 1. Read bootstrap-params - // 2. Access the object store - // 3. Populate tools - // 4. Deploy the controller charm using state + ctx, cancel := w.scopedContext() + defer cancel() + + agentConfig := w.cfg.Agent.CurrentConfig() + dataDir := agentConfig.DataDir() + + controllerConfig, err := w.cfg.ControllerConfigService.ControllerConfig(ctx) + if err != nil { + return fmt.Errorf("failed to get controller config: %v", err) + } + + // Agent binary seeder will populate the tools for the agent. + agentStorage := agentStorageShim{State: w.cfg.State} + if err := w.cfg.AgentBinarySeeder(ctx, dataDir, agentStorage, w.cfg.ObjectStore, controllerConfig, w.cfg.Logger); err != nil { + return errors.Trace(err) + } + w.cfg.BootstrapUnlocker.Unlock() return nil } @@ -111,3 +137,22 @@ func (w *bootstrapWorker) reportInternalState(state string) { default: } } + +// scopedContext returns a context that is in the scope of the worker lifetime. +// It returns a cancellable context that is cancelled when the action has +// completed. +func (w *bootstrapWorker) scopedContext() (context.Context, context.CancelFunc) { + ctx, cancel := context.WithCancel(context.Background()) + return w.tomb.Context(ctx), cancel +} + +type agentStorageShim struct { + State *state.State +} + +// AgentBinaryStorage returns the interface for the BinaryAgentStorage. +// This is currently a shim wrapper around the tools storage. That will be +// renamed once we re-implement the tools storage in dqlite. +func (s agentStorageShim) AgentBinaryStorage(objectStore objectstore.ObjectStore) (BinaryAgentStorage, error) { + return s.State.ToolsStorage(objectStore) +} diff --git a/worker/bootstrap/worker_test.go b/worker/bootstrap/worker_test.go index ac82dbff048..9d5d66e71a8 100644 --- a/worker/bootstrap/worker_test.go +++ b/worker/bootstrap/worker_test.go @@ -4,6 +4,7 @@ package bootstrap import ( + "context" time "time" jc "github.com/juju/testing/checkers" @@ -12,6 +13,8 @@ import ( gomock "go.uber.org/mock/gomock" gc "gopkg.in/check.v1" + controller "github.com/juju/juju/controller" + "github.com/juju/juju/core/objectstore" "github.com/juju/juju/state" "github.com/juju/juju/testing" ) @@ -42,8 +45,10 @@ func (s *workerSuite) newWorker(c *gc.C) worker.Worker { Agent: s.agent, ObjectStore: s.objectStore, BootstrapUnlocker: s.bootstrapUnlocker, - AgentBinarySeeder: func() error { return nil }, - State: &state.State{}, + AgentBinarySeeder: func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, controller.Config, Logger) error { + return nil + }, + State: &state.State{}, }, s.states) c.Assert(err, jc.ErrorIsNil) return w From 85bf066ea56240df40cd8b86fd8b0fd2e9011c30 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Fri, 1 Dec 2023 13:59:35 +0000 Subject: [PATCH 2/7] Removes the bootstrap prepareTools Moves the bootstrap prepareTools that was previously run by the cmd/agent/bootstrap state command, to the bootstrap worker. The strategy is actually in the internal package, which gives us freedom to implement the strategy based on the dependencies. The uploading of the tools only happens on in IAAS models. There is no if isCAAS check, it's up to the dependency engine to give us the right dependencies. The code path becomes branchless on that dependency. The userdataconfig removed the tools binary after it ran, this was problematic as the worker isn't guaranteed to have run with in that window. Removing that code and pushing it into the strategy now ensures it correctly cleans up it's own concern. If the upload fails, the tools file will still be there, allowing us to retry later. Lastly, the big major change is to run the bootstrap worker only if the bootstrap-params file is available. Only once the bootstrap worker has completed do we remove that file. This should allow us to perform retries at a later time if we want to make bootstrapping more reliable. --- agent/tools/toolsdir.go | 4 +- cmd/jujud/agent/bootstrap.go | 61 -------- cmd/jujud/agent/bootstrap_test.go | 45 ------ cmd/jujud/agent/machine/manifolds.go | 31 ++-- internal/bootstrap/agentbinary.go | 23 ++- internal/bootstrap/agentbinary_test.go | 166 ++++++++++++++++++++++ internal/bootstrap/bootstrap_mock_test.go | 51 +++++++ internal/bootstrap/package_test.go | 38 +++++ internal/cloudconfig/userdatacfg_test.go | 6 +- internal/cloudconfig/userdatacfg_unix.go | 6 - worker/bootstrap/bootstrap_mock_test.go | 90 ++++++++++++ worker/bootstrap/manifold.go | 131 ++++++++--------- worker/bootstrap/manifold_test.go | 55 ++++--- worker/bootstrap/package_test.go | 23 ++- worker/bootstrap/worker.go | 39 +++-- worker/bootstrap/worker_test.go | 36 ++++- worker/objectstore/manifold.go | 2 +- 17 files changed, 562 insertions(+), 245 deletions(-) create mode 100644 internal/bootstrap/agentbinary_test.go create mode 100644 internal/bootstrap/bootstrap_mock_test.go create mode 100644 internal/bootstrap/package_test.go create mode 100644 worker/bootstrap/bootstrap_mock_test.go diff --git a/agent/tools/toolsdir.go b/agent/tools/toolsdir.go index 61122b312bb..dbac570cbcb 100644 --- a/agent/tools/toolsdir.go +++ b/agent/tools/toolsdir.go @@ -162,11 +162,11 @@ func ReadTools(dataDir string, vers version.Binary) (*coretools.Tools, error) { dir := SharedToolsDir(dataDir, vers) toolsData, err := os.ReadFile(path.Join(dir, toolsFile)) if err != nil { - return nil, fmt.Errorf("cannot read agent metadata in directory %v: %v", dir, err) + return nil, fmt.Errorf("cannot read agent metadata in directory %v: %w", dir, err) } var tools coretools.Tools if err := json.Unmarshal(toolsData, &tools); err != nil { - return nil, fmt.Errorf("invalid agent metadata in directory %q: %v", dir, err) + return nil, fmt.Errorf("invalid agent metadata in directory %q: %w", dir, err) } return &tools, nil } diff --git a/cmd/jujud/agent/bootstrap.go b/cmd/jujud/agent/bootstrap.go index c79180e1334..ae8c0b7a8e1 100644 --- a/cmd/jujud/agent/bootstrap.go +++ b/cmd/jujud/agent/bootstrap.go @@ -4,8 +4,6 @@ package agent import ( - "bytes" - "context" stdcontext "context" "fmt" "io" @@ -22,12 +20,10 @@ import ( "github.com/juju/loggo" "github.com/juju/names/v4" "github.com/juju/utils/v3/ssh" - "github.com/juju/version/v2" "github.com/juju/juju/agent" "github.com/juju/juju/agent/agentbootstrap" agentconfig "github.com/juju/juju/agent/config" - agenttools "github.com/juju/juju/agent/tools" "github.com/juju/juju/caas" k8sprovider "github.com/juju/juju/caas/kubernetes/provider" k8sconstants "github.com/juju/juju/caas/kubernetes/provider/constants" @@ -55,7 +51,6 @@ import ( "github.com/juju/juju/internal/objectstore" "github.com/juju/juju/internal/tools" "github.com/juju/juju/state" - "github.com/juju/juju/state/binarystorage" "github.com/juju/juju/state/cloudimagemetadata" "github.com/juju/juju/state/stateenvirons" jujuversion "github.com/juju/juju/version" @@ -427,10 +422,6 @@ func (c *BootstrapCommand) Run(ctx *cmd.Context) error { } if !isCAAS { - // Populate the tools catalogue. - if err := c.populateTools(ctx, st, args.ControllerConfig); err != nil { - return errors.Trace(err) - } // Add custom image metadata to environment storage. if len(args.CustomImageMetadata) > 0 { if err := c.saveCustomImageMetadata(st, env, args.CustomImageMetadata); err != nil { @@ -574,58 +565,6 @@ func (c *BootstrapCommand) startMongo(ctx stdcontext.Context, isCAAS bool, addrs return nil } -// populateTools stores uploaded tools in provider storage -// and updates the tools metadata. -func (c *BootstrapCommand) populateTools(ctx context.Context, st *state.State, controllerConfig controller.Config) error { - agentConfig := c.CurrentConfig() - dataDir := agentConfig.DataDir() - - current := version.Binary{ - Number: jujuversion.Current, - Arch: arch.HostArch(), - Release: coreos.HostOSTypeName(), - } - agentTools, err := agenttools.ReadTools(dataDir, current) - if err != nil { - return errors.Trace(err) - } - - data, err := os.ReadFile(filepath.Join( - agenttools.SharedToolsDir(dataDir, current), - "tools.tar.gz", - )) - if err != nil { - return errors.Trace(err) - } - - objectStore, err := objectstore.ObjectStoreFactory(ctx, - objectstore.BackendTypeOrDefault(controllerConfig.ObjectStoreType()), - st.ControllerModelUUID(), - objectstore.WithMongoSession(st), - objectstore.WithLogger(logger), - ) - if err != nil { - return errors.Trace(err) - } - - toolStorage, err := st.ToolsStorage(objectStore) - if err != nil { - return errors.Trace(err) - } - defer func() { _ = toolStorage.Close() }() - - metadata := binarystorage.Metadata{ - Version: agentTools.Version.String(), - Size: agentTools.Size, - SHA256: agentTools.SHA256, - } - logger.Debugf("Adding agent binary: %v", agentTools.Version) - if err := toolStorage.Add(ctx, bytes.NewReader(data), metadata); err != nil { - return errors.Trace(err) - } - return nil -} - // saveCustomImageMetadata stores the custom image metadata to the database, func (c *BootstrapCommand) saveCustomImageMetadata(st *state.State, env environs.BootstrapEnviron, imageMetadata []*imagemetadata.ImageMetadata) error { logger.Debugf("saving custom image metadata") diff --git a/cmd/jujud/agent/bootstrap_test.go b/cmd/jujud/agent/bootstrap_test.go index 601db91d22b..72fe750a300 100644 --- a/cmd/jujud/agent/bootstrap_test.go +++ b/cmd/jujud/agent/bootstrap_test.go @@ -25,7 +25,6 @@ import ( jtesting "github.com/juju/testing" jc "github.com/juju/testing/checkers" "github.com/juju/utils/v3" - "github.com/juju/version/v2" "go.uber.org/mock/gomock" gc "gopkg.in/check.v1" @@ -638,50 +637,6 @@ func (s *BootstrapSuite) TestSystemIdentityWritten(c *gc.C) { c.Assert(string(data), gc.Equals, "private-key") } -func (s *BootstrapSuite) TestDownloadedToolsMetadata(c *gc.C) { - // Tools downloaded by cloud-init script. - s.testToolsMetadata(c) -} - -func (s *BootstrapSuite) TestUploadedToolsMetadata(c *gc.C) { - // Tools uploaded over ssh. - s.writeDownloadedTools(c, &tools.Tools{ - Version: testing.CurrentVersion(), - URL: "file:///does/not/matter", - }) - s.testToolsMetadata(c) -} - -func (s *BootstrapSuite) testToolsMetadata(c *gc.C) { - envtesting.RemoveFakeToolsMetadata(c, s.toolsStorage) - - _, cmd, err := s.initBootstrapCommand(c, nil) - - c.Assert(err, jc.ErrorIsNil) - err = cmd.Run(cmdtesting.Context(c)) - c.Assert(err, jc.ErrorIsNil) - - // We don't write metadata at bootstrap anymore. - ss := simplestreams.NewSimpleStreams(sstesting.TestDataSourceFactory()) - simplestreamsMetadata, err := envtools.ReadMetadata(ss, s.toolsStorage, "released") - c.Assert(err, jc.ErrorIsNil) - c.Assert(simplestreamsMetadata, gc.HasLen, 0) - - // The tools should have been added to tools storage. - st, closer := s.getSystemState(c) - defer closer() - - storage, err := st.ToolsStorage(jujutesting.NewObjectStore(c, st.ControllerModelUUID(), st)) - c.Assert(err, jc.ErrorIsNil) - defer storage.Close() - metadata, err := storage.AllMetadata() - c.Assert(err, jc.ErrorIsNil) - c.Assert(metadata, gc.HasLen, 1) - m := metadata[0] - v := version.MustParseBinary(m.Version) - c.Assert(v.Release, gc.Equals, coreos.HostOSTypeName()) -} - func createImageMetadata() []*imagemetadata.ImageMetadata { return []*imagemetadata.ImageMetadata{{ Id: "imageId", diff --git a/cmd/jujud/agent/machine/manifolds.go b/cmd/jujud/agent/machine/manifolds.go index 26c7831cd71..fe59ee72184 100644 --- a/cmd/jujud/agent/machine/manifolds.go +++ b/cmd/jujud/agent/machine/manifolds.go @@ -835,13 +835,15 @@ func IAASManifolds(config ManifoldsConfig) dependency.Manifolds { manifolds := dependency.Manifolds{ // Bootstrap worker is responsible for setting up the initial machine. bootstrapName: ifDatabaseUpgradeComplete(bootstrap.Manifold(bootstrap.ManifoldConfig{ - AgentName: agentName, - StateName: stateName, - ObjectStoreName: objectStoreName, - BootstrapGateName: isBootstrapGateName, - AgentBinarySeeder: bootstrap.IAASAgentBinarySeeder, - RequiresBootstrap: bootstrap.RequiresBootstrap, - Logger: loggo.GetLogger("juju.worker.bootstrap"), + AgentName: agentName, + StateName: stateName, + ObjectStoreName: objectStoreName, + ServiceFactoryName: serviceFactoryName, + BootstrapGateName: isBootstrapGateName, + AgentBinaryUploader: bootstrap.IAASAgentBinaryUploader, + RequiresBootstrap: bootstrap.RequiresBootstrap, + CompletesBootstrap: bootstrap.CompletesBootstrap, + Logger: loggo.GetLogger("juju.worker.bootstrap"), })), toolsVersionCheckerName: ifNotMigrating(toolsversionchecker.Manifold(toolsversionchecker.ManifoldConfig{ @@ -1051,13 +1053,14 @@ func CAASManifolds(config ManifoldsConfig) dependency.Manifolds { return mergeManifolds(config, dependency.Manifolds{ // Bootstrap worker is responsible for setting up the initial machine. bootstrapName: ifDatabaseUpgradeComplete(bootstrap.Manifold(bootstrap.ManifoldConfig{ - AgentName: agentName, - StateName: stateName, - ObjectStoreName: objectStoreName, - BootstrapGateName: isBootstrapGateName, - AgentBinarySeeder: bootstrap.CAASAgentBinarySeeder, - RequiresBootstrap: bootstrap.RequiresBootstrap, - Logger: loggo.GetLogger("juju.worker.bootstrap"), + AgentName: agentName, + StateName: stateName, + ObjectStoreName: objectStoreName, + ServiceFactoryName: serviceFactoryName, + BootstrapGateName: isBootstrapGateName, + AgentBinaryUploader: bootstrap.CAASAgentBinaryUploader, + RequiresBootstrap: bootstrap.RequiresBootstrap, + Logger: loggo.GetLogger("juju.worker.bootstrap"), })), // TODO(caas) - when we support HA, only want this on primary diff --git a/internal/bootstrap/agentbinary.go b/internal/bootstrap/agentbinary.go index 1bc44d8e70b..19b0a584f87 100644 --- a/internal/bootstrap/agentbinary.go +++ b/internal/bootstrap/agentbinary.go @@ -15,7 +15,6 @@ import ( "github.com/juju/version/v2" agenttools "github.com/juju/juju/agent/tools" - "github.com/juju/juju/controller" "github.com/juju/juju/core/arch" coreos "github.com/juju/juju/core/os" "github.com/juju/juju/state/binarystorage" @@ -43,7 +42,7 @@ type AgentBinaryStorage interface { // PopulateAgentBinary is the function that is used to populate the agent // binary at bootstrap. -func PopulateAgentBinary(ctx context.Context, dataDir string, storage AgentBinaryStorage, cfg controller.Config, logger Logger) error { +func PopulateAgentBinary(ctx context.Context, dataDir string, storage AgentBinaryStorage, logger Logger) error { current := version.Binary{ Number: jujuversion.Current, Arch: arch.HostArch(), @@ -52,13 +51,13 @@ func PopulateAgentBinary(ctx context.Context, dataDir string, storage AgentBinar agentTools, err := agenttools.ReadTools(dataDir, current) if err != nil { - return fmt.Errorf("cannot read tools: %w", err) + return fmt.Errorf("cannot read agent binary: %w", err) } - data, err := os.ReadFile(filepath.Join( - agenttools.SharedToolsDir(dataDir, current), - AgentCompressedBinaryName, - )) + rootPath := agenttools.SharedToolsDir(dataDir, current) + binaryPath := filepath.Join(rootPath, AgentCompressedBinaryName) + + data, err := os.ReadFile(binaryPath) if err != nil { return errors.Trace(err) } @@ -74,5 +73,15 @@ func PopulateAgentBinary(ctx context.Context, dataDir string, storage AgentBinar return errors.Trace(err) } + // Ensure that we remove the agent binary from disk. + if err := os.Remove(binaryPath); err != nil { + logger.Warningf("failed to remove agent binary: %v", err) + } + // Remove the sha which validates the agent binary file. + shaFilePath := filepath.Join(rootPath, fmt.Sprintf("juju%s.sha256", current.String())) + if err := os.Remove(shaFilePath); err != nil { + logger.Warningf("failed to remove agent binary sha: %v", err) + } + return nil } diff --git a/internal/bootstrap/agentbinary_test.go b/internal/bootstrap/agentbinary_test.go new file mode 100644 index 00000000000..d012e7d731e --- /dev/null +++ b/internal/bootstrap/agentbinary_test.go @@ -0,0 +1,166 @@ +// Copyright 2023 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package bootstrap + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + + jc "github.com/juju/testing/checkers" + "github.com/juju/version/v2" + gomock "go.uber.org/mock/gomock" + gc "gopkg.in/check.v1" + + "github.com/juju/juju/core/arch" + coreos "github.com/juju/juju/core/os" + "github.com/juju/juju/state/binarystorage" + jujuversion "github.com/juju/juju/version" +) + +type agentBinarySuite struct { + baseSuite +} + +var _ = gc.Suite(&agentBinarySuite{}) + +func (s *agentBinarySuite) TestPopulateAgentBinary(c *gc.C) { + defer s.setupMocks(c).Finish() + + current := version.Binary{ + Number: jujuversion.Current, + Arch: arch.HostArch(), + Release: coreos.HostOSTypeName(), + } + + dir, toolsPath := s.ensureDirs(c, current) + size := int64(4) + + s.writeDownloadTools(c, toolsPath, downloadTools{ + Version: current.String(), + URL: filepath.Join(dir, "tools", fmt.Sprintf("%s.tgz", current.String())), + SHA256: "sha256", + Size: size, + }) + + s.writeAgentBinary(c, toolsPath, current) + + s.storage.EXPECT().Add(gomock.Any(), gomock.Any(), binarystorage.Metadata{ + Version: current.String(), + Size: size, + SHA256: "sha256", + }).Return(nil) + + err := PopulateAgentBinary(context.Background(), dir, s.storage, s.logger) + c.Assert(err, jc.ErrorIsNil) +} + +func (s *agentBinarySuite) TestPopulateAgentBinaryAddError(c *gc.C) { + defer s.setupMocks(c).Finish() + + current := version.Binary{ + Number: jujuversion.Current, + Arch: arch.HostArch(), + Release: coreos.HostOSTypeName(), + } + + dir, toolsPath := s.ensureDirs(c, current) + size := int64(4) + + s.writeDownloadTools(c, toolsPath, downloadTools{ + Version: current.String(), + URL: filepath.Join(dir, "tools", fmt.Sprintf("%s.tgz", current.String())), + SHA256: "sha256", + Size: size, + }) + + s.writeAgentBinary(c, toolsPath, current) + + s.storage.EXPECT().Add(gomock.Any(), gomock.Any(), binarystorage.Metadata{ + Version: current.String(), + Size: size, + SHA256: "sha256", + }).Return(errors.New("boom")) + + err := PopulateAgentBinary(context.Background(), dir, s.storage, s.logger) + c.Assert(err, gc.ErrorMatches, "boom") +} + +func (s *agentBinarySuite) TestPopulateAgentBinaryNoDownloadedToolsFile(c *gc.C) { + defer s.setupMocks(c).Finish() + + current := version.Binary{ + Number: jujuversion.Current, + Arch: arch.HostArch(), + Release: coreos.HostOSTypeName(), + } + + dir, _ := s.ensureDirs(c, current) + + err := PopulateAgentBinary(context.Background(), dir, s.storage, s.logger) + c.Assert(err, jc.ErrorIs, os.ErrNotExist) +} + +func (s *agentBinarySuite) TestPopulateAgentBinaryNoBinaryFile(c *gc.C) { + defer s.setupMocks(c).Finish() + + current := version.Binary{ + Number: jujuversion.Current, + Arch: arch.HostArch(), + Release: coreos.HostOSTypeName(), + } + + dir, toolsPath := s.ensureDirs(c, current) + size := int64(4) + + s.writeDownloadTools(c, toolsPath, downloadTools{ + Version: current.String(), + URL: filepath.Join(dir, "tools", fmt.Sprintf("%s.tgz", current.String())), + SHA256: "sha256", + Size: size, + }) + + err := PopulateAgentBinary(context.Background(), dir, s.storage, s.logger) + c.Assert(err, jc.ErrorIs, os.ErrNotExist) +} + +func (s *agentBinarySuite) ensureDirs(c *gc.C, current version.Binary) (string, string) { + dir := c.MkDir() + + path := filepath.Join(dir, "tools", current.String()) + + err := os.MkdirAll(path, 0755) + c.Assert(err, jc.ErrorIsNil) + + _, err = os.Stat(path) + c.Assert(err, jc.ErrorIsNil) + + return dir, path +} + +func (s *agentBinarySuite) writeDownloadTools(c *gc.C, dir string, tools downloadTools) { + b, err := json.Marshal(tools) + c.Assert(err, jc.ErrorIsNil) + + err = os.WriteFile(filepath.Join(dir, "downloaded-tools.txt"), b, 0644) + c.Assert(err, jc.ErrorIsNil) +} + +func (S *agentBinarySuite) writeAgentBinary(c *gc.C, dir string, current version.Binary) { + err := os.WriteFile(filepath.Join(dir, "tools.tar.gz"), []byte("data"), 0644) + c.Assert(err, jc.ErrorIsNil) + + err = os.WriteFile(filepath.Join(dir, fmt.Sprintf("%s.sha256", current.String())), []byte("sha256"), 0644) + c.Assert(err, jc.ErrorIsNil) +} + +type downloadTools struct { + Version string `json:"version"` + URL string `json:"url"` + SHA256 string `json:"sha256"` + Size int64 `json:"size"` +} diff --git a/internal/bootstrap/bootstrap_mock_test.go b/internal/bootstrap/bootstrap_mock_test.go new file mode 100644 index 00000000000..8100293b580 --- /dev/null +++ b/internal/bootstrap/bootstrap_mock_test.go @@ -0,0 +1,51 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/juju/juju/internal/bootstrap (interfaces: AgentBinaryStorage) + +// Package bootstrap is a generated GoMock package. +package bootstrap + +import ( + context "context" + io "io" + reflect "reflect" + + binarystorage "github.com/juju/juju/state/binarystorage" + gomock "go.uber.org/mock/gomock" +) + +// MockAgentBinaryStorage is a mock of AgentBinaryStorage interface. +type MockAgentBinaryStorage struct { + ctrl *gomock.Controller + recorder *MockAgentBinaryStorageMockRecorder +} + +// MockAgentBinaryStorageMockRecorder is the mock recorder for MockAgentBinaryStorage. +type MockAgentBinaryStorageMockRecorder struct { + mock *MockAgentBinaryStorage +} + +// NewMockAgentBinaryStorage creates a new mock instance. +func NewMockAgentBinaryStorage(ctrl *gomock.Controller) *MockAgentBinaryStorage { + mock := &MockAgentBinaryStorage{ctrl: ctrl} + mock.recorder = &MockAgentBinaryStorageMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockAgentBinaryStorage) EXPECT() *MockAgentBinaryStorageMockRecorder { + return m.recorder +} + +// Add mocks base method. +func (m *MockAgentBinaryStorage) Add(arg0 context.Context, arg1 io.Reader, arg2 binarystorage.Metadata) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Add", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// Add indicates an expected call of Add. +func (mr *MockAgentBinaryStorageMockRecorder) Add(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Add", reflect.TypeOf((*MockAgentBinaryStorage)(nil).Add), arg0, arg1, arg2) +} diff --git a/internal/bootstrap/package_test.go b/internal/bootstrap/package_test.go new file mode 100644 index 00000000000..b664fede3d2 --- /dev/null +++ b/internal/bootstrap/package_test.go @@ -0,0 +1,38 @@ +// Copyright 2023 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package bootstrap + +import ( + "testing" + + jujutesting "github.com/juju/testing" + "go.uber.org/mock/gomock" + gc "gopkg.in/check.v1" + + jujujujutesting "github.com/juju/juju/testing" +) + +//go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination bootstrap_mock_test.go github.com/juju/juju/internal/bootstrap AgentBinaryStorage + +func Test(t *testing.T) { + gc.TestingT(t) +} + +type baseSuite struct { + jujutesting.IsolationSuite + + storage *MockAgentBinaryStorage + + logger Logger +} + +func (s *baseSuite) setupMocks(c *gc.C) *gomock.Controller { + ctrl := gomock.NewController(c) + + s.storage = NewMockAgentBinaryStorage(ctrl) + + s.logger = jujujujutesting.NewCheckLogger(c) + + return ctrl +} diff --git a/internal/cloudconfig/userdatacfg_test.go b/internal/cloudconfig/userdatacfg_test.go index f848372707d..9d6ead9ef6d 100644 --- a/internal/cloudconfig/userdatacfg_test.go +++ b/internal/cloudconfig/userdatacfg_test.go @@ -349,7 +349,6 @@ echo '.*' > '/var/lib/juju/bootstrap-params' echo 'Installing Juju machine agent'.* /var/lib/juju/tools/1\.2\.3-ubuntu-amd64/jujud bootstrap-state --timeout 10m0s --data-dir '/var/lib/juju' --debug /sbin/remove-juju-services -rm \$bin/tools\.tar\.gz && rm \$bin/juju1\.2\.3-ubuntu-amd64\.sha256 `, }, @@ -384,7 +383,6 @@ install -D -m 600 /dev/null '/var/lib/juju/bootstrap-params' echo '.*' > '/var/lib/juju/bootstrap-params' echo 'Installing Juju machine agent'.* /var/lib/juju/tools/1\.2\.3\.123-ubuntu-amd64/jujud bootstrap-state --timeout 10m0s --data-dir '/var/lib/juju' --debug -rm \$bin/tools\.tar\.gz && rm \$bin/juju1\.2\.3\.123-ubuntu-amd64\.sha256 `, }, @@ -414,7 +412,6 @@ echo -n '{"version":"1\.2\.3-ubuntu-amd64","url":"https://state-addr\.testing\.i mkdir -p '/var/lib/juju/agents/machine-99' cat > '/var/lib/juju/agents/machine-99/agent\.conf' << 'EOF'\\n.*\\nEOF chmod 0600 '/var/lib/juju/agents/machine-99/agent\.conf' -rm \$bin/tools\.tar\.gz && rm \$bin/juju1\.2\.3-ubuntu-amd64\.sha256 `, }, @@ -735,13 +732,12 @@ func (s *cloudinitSuite) TestCloudInitConfigCloudInitUserData(c *gc.C) { `set -xe`, // first line of juju specified cmds } ending := []string{ - `rm $bin/tools.tar.gz && rm $bin/juju2.3.4-ubuntu-amd64.sha256`, // last line of juju specified cmds `mkdir /tmp/postruncmd`, `mkdir "/tmp/postruncmd 2"`, } c.Assert(len(cmds), jc.GreaterThan, 6) c.Assert(cmds[:3], gc.DeepEquals, beginning) - c.Assert(cmds[len(cmds)-3:], gc.DeepEquals, ending) + c.Assert(cmds[len(cmds)-2:], gc.DeepEquals, ending) c.Assert(cloudcfg.SystemUpgrade(), gc.Equals, false) diff --git a/internal/cloudconfig/userdatacfg_unix.go b/internal/cloudconfig/userdatacfg_unix.go index a74a0d210d9..bb195c67dd4 100644 --- a/internal/cloudconfig/userdatacfg_unix.go +++ b/internal/cloudconfig/userdatacfg_unix.go @@ -340,12 +340,6 @@ func (w *unixConfigure) ConfigureJuju() error { return errors.Trace(err) } - // Don't remove tools tarball until after bootstrap agent - // runs, so it has a chance to add it to its catalogue. - defer w.conf.AddRunCmd( - fmt.Sprintf("rm $bin/tools.tar.gz && rm $bin/juju%s.sha256", w.icfg.AgentVersion()), - ) - // We add the machine agent's configuration info // before running bootstrap-state so that bootstrap-state // has a chance to rewrite it to change the password. diff --git a/worker/bootstrap/bootstrap_mock_test.go b/worker/bootstrap/bootstrap_mock_test.go new file mode 100644 index 00000000000..ccae305c6ad --- /dev/null +++ b/worker/bootstrap/bootstrap_mock_test.go @@ -0,0 +1,90 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/juju/juju/worker/bootstrap (interfaces: ControllerConfigService,ObjectStoreGetter) + +// Package bootstrap is a generated GoMock package. +package bootstrap + +import ( + context "context" + reflect "reflect" + + controller "github.com/juju/juju/controller" + objectstore "github.com/juju/juju/core/objectstore" + gomock "go.uber.org/mock/gomock" +) + +// MockControllerConfigService is a mock of ControllerConfigService interface. +type MockControllerConfigService struct { + ctrl *gomock.Controller + recorder *MockControllerConfigServiceMockRecorder +} + +// MockControllerConfigServiceMockRecorder is the mock recorder for MockControllerConfigService. +type MockControllerConfigServiceMockRecorder struct { + mock *MockControllerConfigService +} + +// NewMockControllerConfigService creates a new mock instance. +func NewMockControllerConfigService(ctrl *gomock.Controller) *MockControllerConfigService { + mock := &MockControllerConfigService{ctrl: ctrl} + mock.recorder = &MockControllerConfigServiceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockControllerConfigService) EXPECT() *MockControllerConfigServiceMockRecorder { + return m.recorder +} + +// ControllerConfig mocks base method. +func (m *MockControllerConfigService) ControllerConfig(arg0 context.Context) (controller.Config, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ControllerConfig", arg0) + ret0, _ := ret[0].(controller.Config) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ControllerConfig indicates an expected call of ControllerConfig. +func (mr *MockControllerConfigServiceMockRecorder) ControllerConfig(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControllerConfig", reflect.TypeOf((*MockControllerConfigService)(nil).ControllerConfig), arg0) +} + +// MockObjectStoreGetter is a mock of ObjectStoreGetter interface. +type MockObjectStoreGetter struct { + ctrl *gomock.Controller + recorder *MockObjectStoreGetterMockRecorder +} + +// MockObjectStoreGetterMockRecorder is the mock recorder for MockObjectStoreGetter. +type MockObjectStoreGetterMockRecorder struct { + mock *MockObjectStoreGetter +} + +// NewMockObjectStoreGetter creates a new mock instance. +func NewMockObjectStoreGetter(ctrl *gomock.Controller) *MockObjectStoreGetter { + mock := &MockObjectStoreGetter{ctrl: ctrl} + mock.recorder = &MockObjectStoreGetterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockObjectStoreGetter) EXPECT() *MockObjectStoreGetterMockRecorder { + return m.recorder +} + +// GetObjectStore mocks base method. +func (m *MockObjectStoreGetter) GetObjectStore(arg0 context.Context, arg1 string) (objectstore.ObjectStore, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetObjectStore", arg0, arg1) + ret0, _ := ret[0].(objectstore.ObjectStore) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetObjectStore indicates an expected call of GetObjectStore. +func (mr *MockObjectStoreGetterMockRecorder) GetObjectStore(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetObjectStore", reflect.TypeOf((*MockObjectStoreGetter)(nil).GetObjectStore), arg0, arg1) +} diff --git a/worker/bootstrap/manifold.go b/worker/bootstrap/manifold.go index 6911c87fd7c..93d9baf0233 100644 --- a/worker/bootstrap/manifold.go +++ b/worker/bootstrap/manifold.go @@ -6,21 +6,22 @@ package bootstrap import ( "context" "io" + "os" + "path/filepath" "github.com/juju/errors" "github.com/juju/worker/v3" "github.com/juju/worker/v3/dependency" "github.com/juju/juju/agent" - "github.com/juju/juju/controller" - "github.com/juju/juju/core/application" "github.com/juju/juju/core/objectstore" "github.com/juju/juju/internal/bootstrap" + "github.com/juju/juju/internal/cloudconfig" "github.com/juju/juju/internal/servicefactory" - st "github.com/juju/juju/state" "github.com/juju/juju/state/binarystorage" "github.com/juju/juju/worker/common" "github.com/juju/juju/worker/gate" + workerobjectstore "github.com/juju/juju/worker/objectstore" "github.com/juju/juju/worker/state" ) @@ -47,11 +48,15 @@ type BinaryAgentStorage interface { } // AgentBinaryBootstrapFunc is the function that is used to populate the tools. -type AgentBinaryBootstrapFunc func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, controller.Config, Logger) error +type AgentBinaryBootstrapFunc func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, Logger) error -// RequiresBootstrapFunc is the function that is used to check if the controller -// application exists. -type RequiresBootstrapFunc func(appService ApplicationService) (bool, error) +// RequiresBootstrapFunc is the function that is used to check if the bootstrap +// process is required. +type RequiresBootstrapFunc func(agent.Config) (bool, error) + +// CompletesBootstrapFunc is the function that is used to complete the bootstrap +// process. +type CompletesBootstrapFunc func(agent.Config) error // ManifoldConfig defines the configuration for the trace manifold. type ManifoldConfig struct { @@ -61,9 +66,10 @@ type ManifoldConfig struct { BootstrapGateName string ServiceFactoryName string - Logger Logger - AgentBinarySeeder AgentBinaryBootstrapFunc - RequiresBootstrap RequiresBootstrapFunc + Logger Logger + AgentBinaryUploader AgentBinaryBootstrapFunc + RequiresBootstrap RequiresBootstrapFunc + CompletesBootstrap CompletesBootstrapFunc } // Validate validates the manifold configuration. @@ -86,8 +92,14 @@ func (cfg ManifoldConfig) Validate() error { if cfg.Logger == nil { return errors.NotValidf("nil Logger") } - if cfg.AgentBinarySeeder == nil { - return errors.NotValidf("nil AgentBinarySeeder") + if cfg.AgentBinaryUploader == nil { + return errors.NotValidf("nil AgentBinaryUploader") + } + if cfg.RequiresBootstrap == nil { + return errors.NotValidf("nil RequiresBootstrap") + } + if cfg.CompletesBootstrap == nil { + return errors.NotValidf("nil CompletesBootstrap") } return nil } @@ -107,58 +119,62 @@ func Manifold(config ManifoldConfig) dependency.Manifold { return nil, errors.Trace(err) } - var a agent.Agent - if err := context.Get(config.AgentName, &a); err != nil { - return nil, err - } - var bootstrapUnlocker gate.Unlocker if err := context.Get(config.BootstrapGateName, &bootstrapUnlocker); err != nil { return nil, errors.Trace(err) } - var stTracker state.StateTracker - if err := context.Get(config.StateName, &stTracker); err != nil { - return nil, errors.Trace(err) - } - - // Get the state pool after grabbing dependencies so we don't need - // to remember to call Done on it if they're not running yet. - _, st, err := stTracker.Use() - if err != nil { - return nil, errors.Trace(err) + var a agent.Agent + if err := context.Get(config.AgentName, &a); err != nil { + return nil, err } // If the controller application exists, then we don't need to // bootstrap. Uninstall the worker, as we don't need it running // anymore. - if ok, err := config.RequiresBootstrap(&applicationStateService{st: st}); err != nil { + if ok, err := config.RequiresBootstrap(a.CurrentConfig()); err != nil { return nil, errors.Trace(err) } else if !ok { - _ = stTracker.Done() bootstrapUnlocker.Unlock() return nil, dependency.ErrUninstall } - var objectStore objectstore.ObjectStore - if err := context.Get(config.ObjectStoreName, &objectStore); err != nil { - _ = stTracker.Done() + var objectStoreGetter workerobjectstore.ObjectStoreGetter + if err := context.Get(config.ObjectStoreName, &objectStoreGetter); err != nil { return nil, errors.Trace(err) } var controllerServiceFactory servicefactory.ControllerServiceFactory if err := context.Get(config.ServiceFactoryName, &controllerServiceFactory); err != nil { + return nil, errors.Trace(err) + } + + var stTracker state.StateTracker + if err := context.Get(config.StateName, &stTracker); err != nil { + return nil, errors.Trace(err) + } + + // Get the state pool after grabbing dependencies so we don't need + // to remember to call Done on it if they're not running yet. + statePool, _, err := stTracker.Use() + if err != nil { + return nil, errors.Trace(err) + } + + systemState, err := statePool.SystemState() + if err != nil { _ = stTracker.Done() return nil, errors.Trace(err) } w, err := NewWorker(WorkerConfig{ Agent: a, - ObjectStore: objectStore, + ObjectStoreGetter: objectStoreGetter, ControllerConfigService: controllerServiceFactory.ControllerConfig(), - State: st, + State: systemState, BootstrapUnlocker: bootstrapUnlocker, - AgentBinarySeeder: config.AgentBinarySeeder, + AgentBinaryUploader: config.AgentBinaryUploader, + CompletesBootstrap: config.CompletesBootstrap, Logger: config.Logger, }) if err != nil { @@ -173,51 +189,38 @@ func Manifold(config ManifoldConfig) dependency.Manifold { } } -// CAASAgentBinarySeeder is the function that is used to populate the tools +// CAASAgentBinaryUploader is the function that is used to populate the tools // for CAAS. -func CAASAgentBinarySeeder(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, controller.Config, Logger) error { +func CAASAgentBinaryUploader(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, Logger) error { // CAAS doesn't need to populate the tools. return nil } -// IAASAgentBinarySeeder is the function that is used to populate the tools +// IAASAgentBinaryUploader is the function that is used to populate the tools // for IAAS. -func IAASAgentBinarySeeder(ctx context.Context, dataDir string, storageService BinaryAgentStorageService, objectStore objectstore.ObjectStore, controllerConfig controller.Config, logger Logger) error { +func IAASAgentBinaryUploader(ctx context.Context, dataDir string, storageService BinaryAgentStorageService, objectStore objectstore.ObjectStore, logger Logger) error { storage, err := storageService.AgentBinaryStorage(objectStore) if err != nil { return errors.Trace(err) } defer storage.Close() - return bootstrap.PopulateAgentBinary(ctx, dataDir, storage, controllerConfig, logger) -} - -// ApplicationService is the interface that is used to check if an application -// exists. -type ApplicationService interface { - ApplicationExists(name string) (bool, error) + return bootstrap.PopulateAgentBinary(ctx, dataDir, storage, logger) } -// RequiresBootstrap returns true if the controller application does not exist. -func RequiresBootstrap(appService ApplicationService) (bool, error) { - ok, err := appService.ApplicationExists(application.ControllerApplicationName) - if err != nil { +// RequiresBootstrap returns true if the bootstrap params file exists. +// It is expected at the end of bootstrap that the file is removed. +func RequiresBootstrap(config agent.Config) (bool, error) { + _, err := os.Stat(filepath.Join(config.DataDir(), cloudconfig.FileNameBootstrapParams)) + if err != nil && !os.IsNotExist(err) { return false, errors.Trace(err) } - return !ok, nil -} - -type applicationStateService struct { - st *st.State + return !os.IsNotExist(err), nil } -func (s *applicationStateService) ApplicationExists(name string) (bool, error) { - _, err := s.st.Application(name) - if err == nil { - return true, nil - } - if errors.Is(err, errors.NotFound) { - return false, nil - } - return false, errors.Annotatef(err, "application exists") +// CompletesBootstrap removes the bootstrap params file, completing the +// bootstrap process. +func CompletesBootstrap(config agent.Config) error { + // Remove the bootstrap params file. + return os.Remove(filepath.Join(config.DataDir(), cloudconfig.FileNameBootstrapParams)) } diff --git a/worker/bootstrap/manifold_test.go b/worker/bootstrap/manifold_test.go index 0da4895c0e2..5b6fff6af2d 100644 --- a/worker/bootstrap/manifold_test.go +++ b/worker/bootstrap/manifold_test.go @@ -12,9 +12,9 @@ import ( dependencytesting "github.com/juju/worker/v3/dependency/testing" gc "gopkg.in/check.v1" - controller "github.com/juju/juju/controller" + "github.com/juju/juju/agent" "github.com/juju/juju/core/objectstore" - state "github.com/juju/juju/state" + "github.com/juju/juju/domain/servicefactory/testing" ) type manifoldSuite struct { @@ -35,36 +35,56 @@ func (s *manifoldSuite) TestValidateConfig(c *gc.C) { cfg.StateName = "" c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) + cfg.ObjectStoreName = "" + c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) + + cfg.ServiceFactoryName = "" + c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) + + cfg.BootstrapGateName = "" + c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) + cfg = s.getConfig() cfg.Logger = nil c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) + + cfg = s.getConfig() + cfg.RequiresBootstrap = nil + c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) + + cfg = s.getConfig() + cfg.CompletesBootstrap = nil + c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) } func (s *manifoldSuite) getConfig() ManifoldConfig { return ManifoldConfig{ - AgentName: "agent", - ObjectStoreName: "object-store", - StateName: "state", - BootstrapGateName: "bootstrap-gate", - Logger: s.logger, - AgentBinarySeeder: func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, controller.Config, Logger) error { + AgentName: "agent", + ObjectStoreName: "object-store", + StateName: "state", + BootstrapGateName: "bootstrap-gate", + ServiceFactoryName: "service-factory", + Logger: s.logger, + AgentBinaryUploader: func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, Logger) error { return nil }, - RequiresBootstrap: func(appService ApplicationService) (bool, error) { return false, nil }, + RequiresBootstrap: func(agent.Config) (bool, error) { return false, nil }, + CompletesBootstrap: func(agent.Config) error { return nil }, } } func (s *manifoldSuite) getContext() dependency.Context { resources := map[string]any{ - "agent": s.agent, - "state": s.stateTracker, - "object-store": s.objectStore, - "bootstrap-gate": s.bootstrapUnlocker, + "agent": s.agent, + "state": s.stateTracker, + "object-store": s.objectStore, + "bootstrap-gate": s.bootstrapUnlocker, + "service-factory": testing.NewTestingServiceFactory(), } return dependencytesting.StubContext(nil, resources) } -var expectedInputs = []string{"agent", "state", "object-store", "bootstrap-gate"} +var expectedInputs = []string{"agent", "state", "object-store", "bootstrap-gate", "service-factory"} func (s *manifoldSuite) TestInputs(c *gc.C) { c.Assert(Manifold(s.getConfig()).Inputs, jc.SameContents, expectedInputs) @@ -73,14 +93,9 @@ func (s *manifoldSuite) TestInputs(c *gc.C) { func (s *manifoldSuite) TestStartAlreadyBootstrapped(c *gc.C) { defer s.setupMocks(c).Finish() - s.expectStateTracker() s.expectGateUnlock() + s.expectAgentConfig(c) _, err := Manifold(s.getConfig()).Start(s.getContext()) c.Assert(err, jc.ErrorIs, dependency.ErrUninstall) } - -func (s *manifoldSuite) expectStateTracker() { - s.stateTracker.EXPECT().Use().Return(&state.StatePool{}, &state.State{}, nil) - s.stateTracker.EXPECT().Done() -} diff --git a/worker/bootstrap/package_test.go b/worker/bootstrap/package_test.go index 2c677827c2b..385720e0b85 100644 --- a/worker/bootstrap/package_test.go +++ b/worker/bootstrap/package_test.go @@ -6,7 +6,9 @@ package bootstrap import ( "testing" + "github.com/juju/names/v4" jujutesting "github.com/juju/testing" + "github.com/juju/utils/v3" "go.uber.org/mock/gomock" gc "gopkg.in/check.v1" @@ -17,6 +19,7 @@ import ( //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination state_mock_test.go github.com/juju/juju/worker/state StateTracker //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination objectstore_mock_test.go github.com/juju/juju/core/objectstore ObjectStore //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination lock_mock_test.go github.com/juju/juju/worker/gate Unlocker +//go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination bootstrap_mock_test.go github.com/juju/juju/worker/bootstrap ControllerConfigService,ObjectStoreGetter func TestPackage(t *testing.T) { gc.TestingT(t) @@ -25,10 +28,13 @@ func TestPackage(t *testing.T) { type baseSuite struct { jujutesting.IsolationSuite - agent *MockAgent - stateTracker *MockStateTracker - objectStore *MockObjectStore - bootstrapUnlocker *MockUnlocker + agent *MockAgent + agentConfig *MockConfig + stateTracker *MockStateTracker + objectStore *MockObjectStore + objectStoreGetter *MockObjectStoreGetter + bootstrapUnlocker *MockUnlocker + controllerConfigService *MockControllerConfigService logger Logger } @@ -37,9 +43,12 @@ func (s *baseSuite) setupMocks(c *gc.C) *gomock.Controller { ctrl := gomock.NewController(c) s.agent = NewMockAgent(ctrl) + s.agentConfig = NewMockConfig(ctrl) s.stateTracker = NewMockStateTracker(ctrl) s.objectStore = NewMockObjectStore(ctrl) + s.objectStoreGetter = NewMockObjectStoreGetter(ctrl) s.bootstrapUnlocker = NewMockUnlocker(ctrl) + s.controllerConfigService = NewMockControllerConfigService(ctrl) s.logger = jujujujutesting.NewCheckLogger(c) @@ -49,3 +58,9 @@ func (s *baseSuite) setupMocks(c *gc.C) *gomock.Controller { func (s *baseSuite) expectGateUnlock() { s.bootstrapUnlocker.EXPECT().Unlock() } + +func (s *baseSuite) expectAgentConfig(c *gc.C) { + s.agentConfig.EXPECT().DataDir().Return(c.MkDir()).AnyTimes() + s.agentConfig.EXPECT().Controller().Return(names.NewControllerTag(utils.MustNewUUID().String())).AnyTimes() + s.agent.EXPECT().CurrentConfig().Return(s.agentConfig).AnyTimes() +} diff --git a/worker/bootstrap/worker.go b/worker/bootstrap/worker.go index c8f6ca0020c..b3b1303796d 100644 --- a/worker/bootstrap/worker.go +++ b/worker/bootstrap/worker.go @@ -19,7 +19,8 @@ import ( const ( // States which report the state of the worker. - stateStarted = "started" + stateStarted = "started" + stateCompleted = "completed" ) // ControllerConfigService is the interface that is used to get the @@ -28,14 +29,21 @@ type ControllerConfigService interface { ControllerConfig(context.Context) (controller.Config, error) } +// ObjectStoreGetter is the interface that is used to get a object store. +type ObjectStoreGetter interface { + // GetObjectStore returns a object store for the given namespace. + GetObjectStore(context.Context, string) (objectstore.ObjectStore, error) +} + // WorkerConfig encapsulates the configuration options for the // bootstrap worker. type WorkerConfig struct { Agent agent.Agent - ObjectStore objectstore.ObjectStore + ObjectStoreGetter ObjectStoreGetter ControllerConfigService ControllerConfigService BootstrapUnlocker gate.Unlocker - AgentBinarySeeder AgentBinaryBootstrapFunc + AgentBinaryUploader AgentBinaryBootstrapFunc + CompletesBootstrap CompletesBootstrapFunc // Deprecated: This is only here, until we can remove the state layer. State *state.State @@ -48,8 +56,8 @@ func (c *WorkerConfig) Validate() error { if c.Agent == nil { return errors.NotValidf("nil Agent") } - if c.ObjectStore == nil { - return errors.NotValidf("nil ObjectStore") + if c.ObjectStoreGetter == nil { + return errors.NotValidf("nil ObjectStoreGetter") } if c.ControllerConfigService == nil { return errors.NotValidf("nil ControllerConfigService") @@ -57,8 +65,11 @@ func (c *WorkerConfig) Validate() error { if c.BootstrapUnlocker == nil { return errors.NotValidf("nil BootstrapUnlocker") } - if c.AgentBinarySeeder == nil { - return errors.NotValidf("nil AgentBinarySeeder") + if c.AgentBinaryUploader == nil { + return errors.NotValidf("nil AgentBinaryUploader") + } + if c.CompletesBootstrap == nil { + return errors.NotValidf("nil CompletesBootstrap") } if c.Logger == nil { return errors.NotValidf("nil Logger") @@ -115,17 +126,25 @@ func (w *bootstrapWorker) loop() error { agentConfig := w.cfg.Agent.CurrentConfig() dataDir := agentConfig.DataDir() - controllerConfig, err := w.cfg.ControllerConfigService.ControllerConfig(ctx) + objectStore, err := w.cfg.ObjectStoreGetter.GetObjectStore(ctx, agentConfig.Controller().Id()) if err != nil { - return fmt.Errorf("failed to get controller config: %v", err) + return fmt.Errorf("failed to get object store: %v", err) } // Agent binary seeder will populate the tools for the agent. agentStorage := agentStorageShim{State: w.cfg.State} - if err := w.cfg.AgentBinarySeeder(ctx, dataDir, agentStorage, w.cfg.ObjectStore, controllerConfig, w.cfg.Logger); err != nil { + if err := w.cfg.AgentBinaryUploader(ctx, dataDir, agentStorage, objectStore, w.cfg.Logger); err != nil { return errors.Trace(err) } + // Complete the bootstrap, only after this is complete do we unlock the + // bootstrap gate. + if err := w.cfg.CompletesBootstrap(agentConfig); err != nil { + return errors.Trace(err) + } + + w.reportInternalState(stateCompleted) + w.cfg.BootstrapUnlocker.Unlock() return nil } diff --git a/worker/bootstrap/worker_test.go b/worker/bootstrap/worker_test.go index 9d5d66e71a8..0e441a06862 100644 --- a/worker/bootstrap/worker_test.go +++ b/worker/bootstrap/worker_test.go @@ -13,6 +13,7 @@ import ( gomock "go.uber.org/mock/gomock" gc "gopkg.in/check.v1" + agent "github.com/juju/juju/agent" controller "github.com/juju/juju/controller" "github.com/juju/juju/core/objectstore" "github.com/juju/juju/state" @@ -30,11 +31,16 @@ var _ = gc.Suite(&workerSuite{}) func (s *workerSuite) TestKilled(c *gc.C) { defer s.setupMocks(c).Finish() + s.expectGateUnlock() + s.expectControllerConfig() + s.expectAgentConfig(c) + s.expectObjectStoreGetter() + w := s.newWorker(c) defer workertest.DirtyKill(c, w) - s.expectGateUnlock() s.ensureStartup(c) + s.ensureFinished(c) workertest.CleanKill(c, w) } @@ -43,12 +49,14 @@ func (s *workerSuite) newWorker(c *gc.C) worker.Worker { w, err := newWorker(WorkerConfig{ Logger: s.logger, Agent: s.agent, - ObjectStore: s.objectStore, + ObjectStoreGetter: s.objectStoreGetter, BootstrapUnlocker: s.bootstrapUnlocker, - AgentBinarySeeder: func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, controller.Config, Logger) error { + AgentBinaryUploader: func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, Logger) error { return nil }, - State: &state.State{}, + CompletesBootstrap: func(agent.Config) error { return nil }, + State: &state.State{}, + ControllerConfigService: s.controllerConfigService, }, s.states) c.Assert(err, jc.ErrorIsNil) return w @@ -65,10 +73,26 @@ func (s *workerSuite) setupMocks(c *gc.C) *gomock.Controller { } func (s *workerSuite) ensureStartup(c *gc.C) { + s.ensureState(c, stateStarted) +} + +func (s *workerSuite) ensureFinished(c *gc.C) { + s.ensureState(c, stateCompleted) +} + +func (s *workerSuite) ensureState(c *gc.C, st string) { select { case state := <-s.states: - c.Assert(state, gc.Equals, stateStarted) + c.Assert(state, gc.Equals, st) case <-time.After(testing.ShortWait * 10): - c.Fatalf("timed out waiting for startup") + c.Fatalf("timed out waiting for %s", st) } } + +func (s *workerSuite) expectControllerConfig() { + s.controllerConfigService.EXPECT().ControllerConfig(gomock.Any()).Return(controller.Config{}, nil).AnyTimes() +} + +func (s *workerSuite) expectObjectStoreGetter() { + s.objectStoreGetter.EXPECT().GetObjectStore(gomock.Any(), gomock.Any()).Return(s.objectStore, nil) +} diff --git a/worker/objectstore/manifold.go b/worker/objectstore/manifold.go index c5db9e7c268..815c93b8294 100644 --- a/worker/objectstore/manifold.go +++ b/worker/objectstore/manifold.go @@ -221,7 +221,7 @@ func output(in worker.Worker, out any) error { var target ObjectStoreGetter = w *out = target default: - return errors.Errorf("expected output of Tracer, got %T", out) + return errors.Errorf("expected output of ObjectStore, got %T", out) } return nil } From f70c420a77e16249f9fecbd8503972956be2b7fe Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Fri, 1 Dec 2023 17:18:24 +0000 Subject: [PATCH 3/7] Use the right controller UUID --- worker/bootstrap/worker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/worker/bootstrap/worker.go b/worker/bootstrap/worker.go index b3b1303796d..6f0df5e5522 100644 --- a/worker/bootstrap/worker.go +++ b/worker/bootstrap/worker.go @@ -126,7 +126,7 @@ func (w *bootstrapWorker) loop() error { agentConfig := w.cfg.Agent.CurrentConfig() dataDir := agentConfig.DataDir() - objectStore, err := w.cfg.ObjectStoreGetter.GetObjectStore(ctx, agentConfig.Controller().Id()) + objectStore, err := w.cfg.ObjectStoreGetter.GetObjectStore(ctx, w.cfg.State.ControllerModelUUID()) if err != nil { return fmt.Errorf("failed to get object store: %v", err) } From 335e7c09caa325ec7c37fba15b90c2cbafa29271 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Mon, 4 Dec 2023 13:23:12 +0000 Subject: [PATCH 4/7] Add tests to ensure controller model UUID is called --- worker/bootstrap/bootstrap_mock_test.go | 55 ++++++++++++++++++++++++- worker/bootstrap/package_test.go | 4 +- worker/bootstrap/worker.go | 45 +++++++++++++++----- worker/bootstrap/worker_test.go | 31 ++++++++++++++ 4 files changed, 122 insertions(+), 13 deletions(-) diff --git a/worker/bootstrap/bootstrap_mock_test.go b/worker/bootstrap/bootstrap_mock_test.go index ccae305c6ad..66d1b20e345 100644 --- a/worker/bootstrap/bootstrap_mock_test.go +++ b/worker/bootstrap/bootstrap_mock_test.go @@ -1,5 +1,5 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: github.com/juju/juju/worker/bootstrap (interfaces: ControllerConfigService,ObjectStoreGetter) +// Source: github.com/juju/juju/worker/bootstrap (interfaces: ControllerConfigService,ObjectStoreGetter,LegacyState) // Package bootstrap is a generated GoMock package. package bootstrap @@ -10,6 +10,7 @@ import ( controller "github.com/juju/juju/controller" objectstore "github.com/juju/juju/core/objectstore" + binarystorage "github.com/juju/juju/state/binarystorage" gomock "go.uber.org/mock/gomock" ) @@ -88,3 +89,55 @@ func (mr *MockObjectStoreGetterMockRecorder) GetObjectStore(arg0, arg1 interface mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetObjectStore", reflect.TypeOf((*MockObjectStoreGetter)(nil).GetObjectStore), arg0, arg1) } + +// MockLegacyState is a mock of LegacyState interface. +type MockLegacyState struct { + ctrl *gomock.Controller + recorder *MockLegacyStateMockRecorder +} + +// MockLegacyStateMockRecorder is the mock recorder for MockLegacyState. +type MockLegacyStateMockRecorder struct { + mock *MockLegacyState +} + +// NewMockLegacyState creates a new mock instance. +func NewMockLegacyState(ctrl *gomock.Controller) *MockLegacyState { + mock := &MockLegacyState{ctrl: ctrl} + mock.recorder = &MockLegacyStateMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockLegacyState) EXPECT() *MockLegacyStateMockRecorder { + return m.recorder +} + +// ControllerModelUUID mocks base method. +func (m *MockLegacyState) ControllerModelUUID() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ControllerModelUUID") + ret0, _ := ret[0].(string) + return ret0 +} + +// ControllerModelUUID indicates an expected call of ControllerModelUUID. +func (mr *MockLegacyStateMockRecorder) ControllerModelUUID() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControllerModelUUID", reflect.TypeOf((*MockLegacyState)(nil).ControllerModelUUID)) +} + +// ToolsStorage mocks base method. +func (m *MockLegacyState) ToolsStorage(arg0 objectstore.ObjectStore) (binarystorage.StorageCloser, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ToolsStorage", arg0) + ret0, _ := ret[0].(binarystorage.StorageCloser) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ToolsStorage indicates an expected call of ToolsStorage. +func (mr *MockLegacyStateMockRecorder) ToolsStorage(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ToolsStorage", reflect.TypeOf((*MockLegacyState)(nil).ToolsStorage), arg0) +} diff --git a/worker/bootstrap/package_test.go b/worker/bootstrap/package_test.go index 385720e0b85..c49d1f8417a 100644 --- a/worker/bootstrap/package_test.go +++ b/worker/bootstrap/package_test.go @@ -19,7 +19,7 @@ import ( //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination state_mock_test.go github.com/juju/juju/worker/state StateTracker //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination objectstore_mock_test.go github.com/juju/juju/core/objectstore ObjectStore //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination lock_mock_test.go github.com/juju/juju/worker/gate Unlocker -//go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination bootstrap_mock_test.go github.com/juju/juju/worker/bootstrap ControllerConfigService,ObjectStoreGetter +//go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination bootstrap_mock_test.go github.com/juju/juju/worker/bootstrap ControllerConfigService,ObjectStoreGetter,LegacyState func TestPackage(t *testing.T) { gc.TestingT(t) @@ -30,6 +30,7 @@ type baseSuite struct { agent *MockAgent agentConfig *MockConfig + state *MockLegacyState stateTracker *MockStateTracker objectStore *MockObjectStore objectStoreGetter *MockObjectStoreGetter @@ -44,6 +45,7 @@ func (s *baseSuite) setupMocks(c *gc.C) *gomock.Controller { s.agent = NewMockAgent(ctrl) s.agentConfig = NewMockConfig(ctrl) + s.state = NewMockLegacyState(ctrl) s.stateTracker = NewMockStateTracker(ctrl) s.objectStore = NewMockObjectStore(ctrl) s.objectStoreGetter = NewMockObjectStoreGetter(ctrl) diff --git a/worker/bootstrap/worker.go b/worker/bootstrap/worker.go index 6f0df5e5522..6755fd86b7e 100644 --- a/worker/bootstrap/worker.go +++ b/worker/bootstrap/worker.go @@ -13,7 +13,7 @@ import ( "github.com/juju/juju/agent" "github.com/juju/juju/controller" "github.com/juju/juju/core/objectstore" - "github.com/juju/juju/state" + "github.com/juju/juju/state/binarystorage" "github.com/juju/juju/worker/gate" ) @@ -35,6 +35,20 @@ type ObjectStoreGetter interface { GetObjectStore(context.Context, string) (objectstore.ObjectStore, error) } +// LegacyState is the interface that is used to get the legacy state (mongo). +type LegacyState interface { + // ControllerModelUUID returns the UUID of the model that was + // bootstrapped. This is the only model that can have controller + // machines. The owner of this model is also considered "special", in + // that they are the only user that is able to create other users + // (until we have more fine grained permissions), and they cannot be + // disabled. + ControllerModelUUID() string + // ToolsStorage returns a new binarystorage.StorageCloser that stores tools + // metadata in the "juju" database "toolsmetadata" collection. + ToolsStorage(store objectstore.ObjectStore) (binarystorage.StorageCloser, error) +} + // WorkerConfig encapsulates the configuration options for the // bootstrap worker. type WorkerConfig struct { @@ -46,7 +60,7 @@ type WorkerConfig struct { CompletesBootstrap CompletesBootstrapFunc // Deprecated: This is only here, until we can remove the state layer. - State *state.State + State LegacyState Logger Logger } @@ -126,14 +140,8 @@ func (w *bootstrapWorker) loop() error { agentConfig := w.cfg.Agent.CurrentConfig() dataDir := agentConfig.DataDir() - objectStore, err := w.cfg.ObjectStoreGetter.GetObjectStore(ctx, w.cfg.State.ControllerModelUUID()) - if err != nil { - return fmt.Errorf("failed to get object store: %v", err) - } - - // Agent binary seeder will populate the tools for the agent. - agentStorage := agentStorageShim{State: w.cfg.State} - if err := w.cfg.AgentBinaryUploader(ctx, dataDir, agentStorage, objectStore, w.cfg.Logger); err != nil { + // Seed the agent binary to the object store. + if err := w.seedAgentBinary(ctx, dataDir); err != nil { return errors.Trace(err) } @@ -165,8 +173,23 @@ func (w *bootstrapWorker) scopedContext() (context.Context, context.CancelFunc) return w.tomb.Context(ctx), cancel } +func (w *bootstrapWorker) seedAgentBinary(ctx context.Context, dataDir string) error { + objectStore, err := w.cfg.ObjectStoreGetter.GetObjectStore(ctx, w.cfg.State.ControllerModelUUID()) + if err != nil { + return fmt.Errorf("failed to get object store: %v", err) + } + + // Agent binary seeder will populate the tools for the agent. + agentStorage := agentStorageShim{State: w.cfg.State} + if err := w.cfg.AgentBinaryUploader(ctx, dataDir, agentStorage, objectStore, w.cfg.Logger); err != nil { + return errors.Trace(err) + } + + return nil +} + type agentStorageShim struct { - State *state.State + State LegacyState } // AgentBinaryStorage returns the interface for the BinaryAgentStorage. diff --git a/worker/bootstrap/worker_test.go b/worker/bootstrap/worker_test.go index 0e441a06862..da197a004a3 100644 --- a/worker/bootstrap/worker_test.go +++ b/worker/bootstrap/worker_test.go @@ -8,6 +8,7 @@ import ( time "time" jc "github.com/juju/testing/checkers" + "github.com/juju/utils/v3" "github.com/juju/worker/v3" "github.com/juju/worker/v3/workertest" gomock "go.uber.org/mock/gomock" @@ -45,6 +46,36 @@ func (s *workerSuite) TestKilled(c *gc.C) { workertest.CleanKill(c, w) } +func (s *workerSuite) TestSeedAgentBinary(c *gc.C) { + defer s.setupMocks(c).Finish() + + // Ensure that the ControllerModelUUID is used for the namespace for the + // object store. If it's not the controller model uuid, then the agent + // binary will not be found. + + uuid := utils.MustNewUUID().String() + + s.state.EXPECT().ControllerModelUUID().Return(uuid) + s.objectStoreGetter.EXPECT().GetObjectStore(gomock.Any(), uuid).Return(s.objectStore, nil) + + var called bool + w := &bootstrapWorker{ + internalStates: s.states, + cfg: WorkerConfig{ + ObjectStoreGetter: s.objectStoreGetter, + AgentBinaryUploader: func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, Logger) error { + called = true + return nil + }, + State: s.state, + Logger: s.logger, + }, + } + err := w.seedAgentBinary(context.Background(), c.MkDir()) + c.Assert(err, jc.ErrorIsNil) + c.Assert(called, jc.IsTrue) +} + func (s *workerSuite) newWorker(c *gc.C) worker.Worker { w, err := newWorker(WorkerConfig{ Logger: s.logger, From 91d1f092ab43b72e3dab2a022746bbdbb1362afe Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Mon, 4 Dec 2023 16:57:03 +0000 Subject: [PATCH 5/7] Rename functions to be more specific The following renames the RequiresBootstrap to BootstrapParamsFileExists, so it becomes explicit about what it does. Prior to the changes in this PR, the concept was rather general, as it wasn't sure about exactly was the cause of a node to bootstrap. This pin points it exactly. --- cmd/jujud/agent/machine/manifolds.go | 34 ++++++++-------- internal/bootstrap/agentbinary.go | 2 +- worker/bootstrap/manifold.go | 60 +++++++++++++++------------- worker/bootstrap/manifold_test.go | 54 +++++++++++++++++++++++-- worker/bootstrap/worker.go | 18 ++++----- worker/bootstrap/worker_test.go | 6 +-- 6 files changed, 112 insertions(+), 62 deletions(-) diff --git a/cmd/jujud/agent/machine/manifolds.go b/cmd/jujud/agent/machine/manifolds.go index fe59ee72184..2d53867d0cb 100644 --- a/cmd/jujud/agent/machine/manifolds.go +++ b/cmd/jujud/agent/machine/manifolds.go @@ -835,15 +835,15 @@ func IAASManifolds(config ManifoldsConfig) dependency.Manifolds { manifolds := dependency.Manifolds{ // Bootstrap worker is responsible for setting up the initial machine. bootstrapName: ifDatabaseUpgradeComplete(bootstrap.Manifold(bootstrap.ManifoldConfig{ - AgentName: agentName, - StateName: stateName, - ObjectStoreName: objectStoreName, - ServiceFactoryName: serviceFactoryName, - BootstrapGateName: isBootstrapGateName, - AgentBinaryUploader: bootstrap.IAASAgentBinaryUploader, - RequiresBootstrap: bootstrap.RequiresBootstrap, - CompletesBootstrap: bootstrap.CompletesBootstrap, - Logger: loggo.GetLogger("juju.worker.bootstrap"), + AgentName: agentName, + StateName: stateName, + ObjectStoreName: objectStoreName, + ServiceFactoryName: serviceFactoryName, + BootstrapGateName: isBootstrapGateName, + AgentBinaryUploader: bootstrap.IAASAgentBinaryUploader, + BootstrapParamsFileExists: bootstrap.BootstrapParamsFileExists, + RemoveBootstrapParamsFile: bootstrap.RemoveBootstrapParamsFile, + Logger: loggo.GetLogger("juju.worker.bootstrap"), })), toolsVersionCheckerName: ifNotMigrating(toolsversionchecker.Manifold(toolsversionchecker.ManifoldConfig{ @@ -1053,14 +1053,14 @@ func CAASManifolds(config ManifoldsConfig) dependency.Manifolds { return mergeManifolds(config, dependency.Manifolds{ // Bootstrap worker is responsible for setting up the initial machine. bootstrapName: ifDatabaseUpgradeComplete(bootstrap.Manifold(bootstrap.ManifoldConfig{ - AgentName: agentName, - StateName: stateName, - ObjectStoreName: objectStoreName, - ServiceFactoryName: serviceFactoryName, - BootstrapGateName: isBootstrapGateName, - AgentBinaryUploader: bootstrap.CAASAgentBinaryUploader, - RequiresBootstrap: bootstrap.RequiresBootstrap, - Logger: loggo.GetLogger("juju.worker.bootstrap"), + AgentName: agentName, + StateName: stateName, + ObjectStoreName: objectStoreName, + ServiceFactoryName: serviceFactoryName, + BootstrapGateName: isBootstrapGateName, + AgentBinaryUploader: bootstrap.CAASAgentBinaryUploader, + BootstrapParamsFileExists: bootstrap.BootstrapParamsFileExists, + Logger: loggo.GetLogger("juju.worker.bootstrap"), })), // TODO(caas) - when we support HA, only want this on primary diff --git a/internal/bootstrap/agentbinary.go b/internal/bootstrap/agentbinary.go index 19b0a584f87..9fcb0b00a0d 100644 --- a/internal/bootstrap/agentbinary.go +++ b/internal/bootstrap/agentbinary.go @@ -77,7 +77,7 @@ func PopulateAgentBinary(ctx context.Context, dataDir string, storage AgentBinar if err := os.Remove(binaryPath); err != nil { logger.Warningf("failed to remove agent binary: %v", err) } - // Remove the sha which validates the agent binary file. + // Remove the sha that validates the agent binary file. shaFilePath := filepath.Join(rootPath, fmt.Sprintf("juju%s.sha256", current.String())) if err := os.Remove(shaFilePath); err != nil { logger.Warningf("failed to remove agent binary sha: %v", err) diff --git a/worker/bootstrap/manifold.go b/worker/bootstrap/manifold.go index 93d9baf0233..2f14590fef7 100644 --- a/worker/bootstrap/manifold.go +++ b/worker/bootstrap/manifold.go @@ -50,13 +50,14 @@ type BinaryAgentStorage interface { // AgentBinaryBootstrapFunc is the function that is used to populate the tools. type AgentBinaryBootstrapFunc func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, Logger) error -// RequiresBootstrapFunc is the function that is used to check if the bootstrap -// process is required. -type RequiresBootstrapFunc func(agent.Config) (bool, error) +// BootstrapParamsFileExistsFunc is the function that is used to check if the +// bootstrap param file exists. We expect that the file exists for only the +// bootstrap controller node. +type BootstrapParamsFileExistsFunc func(agent.Config) (bool, error) -// CompletesBootstrapFunc is the function that is used to complete the bootstrap +// RemoveBootstrapParamsFileFunc is the function that is used to complete the bootstrap // process. -type CompletesBootstrapFunc func(agent.Config) error +type RemoveBootstrapParamsFileFunc func(agent.Config) error // ManifoldConfig defines the configuration for the trace manifold. type ManifoldConfig struct { @@ -66,10 +67,10 @@ type ManifoldConfig struct { BootstrapGateName string ServiceFactoryName string - Logger Logger - AgentBinaryUploader AgentBinaryBootstrapFunc - RequiresBootstrap RequiresBootstrapFunc - CompletesBootstrap CompletesBootstrapFunc + Logger Logger + AgentBinaryUploader AgentBinaryBootstrapFunc + BootstrapParamsFileExists BootstrapParamsFileExistsFunc + RemoveBootstrapParamsFile RemoveBootstrapParamsFileFunc } // Validate validates the manifold configuration. @@ -95,11 +96,11 @@ func (cfg ManifoldConfig) Validate() error { if cfg.AgentBinaryUploader == nil { return errors.NotValidf("nil AgentBinaryUploader") } - if cfg.RequiresBootstrap == nil { - return errors.NotValidf("nil RequiresBootstrap") + if cfg.BootstrapParamsFileExists == nil { + return errors.NotValidf("nil BootstrapParamsFileExists") } - if cfg.CompletesBootstrap == nil { - return errors.NotValidf("nil CompletesBootstrap") + if cfg.RemoveBootstrapParamsFile == nil { + return errors.NotValidf("nil RemoveBootstrapParamsFile") } return nil } @@ -132,7 +133,7 @@ func Manifold(config ManifoldConfig) dependency.Manifold { // If the controller application exists, then we don't need to // bootstrap. Uninstall the worker, as we don't need it running // anymore. - if ok, err := config.RequiresBootstrap(a.CurrentConfig()); err != nil { + if ok, err := config.BootstrapParamsFileExists(a.CurrentConfig()); err != nil { return nil, errors.Trace(err) } else if !ok { bootstrapUnlocker.Unlock() @@ -168,14 +169,14 @@ func Manifold(config ManifoldConfig) dependency.Manifold { } w, err := NewWorker(WorkerConfig{ - Agent: a, - ObjectStoreGetter: objectStoreGetter, - ControllerConfigService: controllerServiceFactory.ControllerConfig(), - State: systemState, - BootstrapUnlocker: bootstrapUnlocker, - AgentBinaryUploader: config.AgentBinaryUploader, - CompletesBootstrap: config.CompletesBootstrap, - Logger: config.Logger, + Agent: a, + ObjectStoreGetter: objectStoreGetter, + ControllerConfigService: controllerServiceFactory.ControllerConfig(), + State: systemState, + BootstrapUnlocker: bootstrapUnlocker, + AgentBinaryUploader: config.AgentBinaryUploader, + RemoveBootstrapParamsFile: config.RemoveBootstrapParamsFile, + Logger: config.Logger, }) if err != nil { _ = stTracker.Done() @@ -208,19 +209,22 @@ func IAASAgentBinaryUploader(ctx context.Context, dataDir string, storageService return bootstrap.PopulateAgentBinary(ctx, dataDir, storage, logger) } -// RequiresBootstrap returns true if the bootstrap params file exists. +// BootstrapParamsFileExists returns true if the bootstrap params file exists. // It is expected at the end of bootstrap that the file is removed. -func RequiresBootstrap(config agent.Config) (bool, error) { +func BootstrapParamsFileExists(config agent.Config) (bool, error) { _, err := os.Stat(filepath.Join(config.DataDir(), cloudconfig.FileNameBootstrapParams)) - if err != nil && !os.IsNotExist(err) { + if err != nil { + if os.IsNotExist(err) { + return false, nil + } return false, errors.Trace(err) } - return !os.IsNotExist(err), nil + return true, nil } -// CompletesBootstrap removes the bootstrap params file, completing the +// RemoveBootstrapParamsFile removes the bootstrap params file, completing the // bootstrap process. -func CompletesBootstrap(config agent.Config) error { +func RemoveBootstrapParamsFile(config agent.Config) error { // Remove the bootstrap params file. return os.Remove(filepath.Join(config.DataDir(), cloudconfig.FileNameBootstrapParams)) } diff --git a/worker/bootstrap/manifold_test.go b/worker/bootstrap/manifold_test.go index 5b6fff6af2d..013732f9d4d 100644 --- a/worker/bootstrap/manifold_test.go +++ b/worker/bootstrap/manifold_test.go @@ -5,6 +5,8 @@ package bootstrap import ( "context" + "os" + "path/filepath" "github.com/juju/errors" jc "github.com/juju/testing/checkers" @@ -15,6 +17,7 @@ import ( "github.com/juju/juju/agent" "github.com/juju/juju/core/objectstore" "github.com/juju/juju/domain/servicefactory/testing" + "github.com/juju/juju/internal/cloudconfig" ) type manifoldSuite struct { @@ -49,11 +52,11 @@ func (s *manifoldSuite) TestValidateConfig(c *gc.C) { c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) cfg = s.getConfig() - cfg.RequiresBootstrap = nil + cfg.BootstrapParamsFileExists = nil c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) cfg = s.getConfig() - cfg.CompletesBootstrap = nil + cfg.RemoveBootstrapParamsFile = nil c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) } @@ -68,8 +71,8 @@ func (s *manifoldSuite) getConfig() ManifoldConfig { AgentBinaryUploader: func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, Logger) error { return nil }, - RequiresBootstrap: func(agent.Config) (bool, error) { return false, nil }, - CompletesBootstrap: func(agent.Config) error { return nil }, + BootstrapParamsFileExists: func(agent.Config) (bool, error) { return false, nil }, + RemoveBootstrapParamsFile: func(agent.Config) error { return nil }, } } @@ -99,3 +102,46 @@ func (s *manifoldSuite) TestStartAlreadyBootstrapped(c *gc.C) { _, err := Manifold(s.getConfig()).Start(s.getContext()) c.Assert(err, jc.ErrorIs, dependency.ErrUninstall) } + +func (s *manifoldSuite) TestBootstrapParamsFileExists(c *gc.C) { + defer s.setupMocks(c).Finish() + + tests := []struct { + name string + ok bool + err error + setup func(*gc.C, string) + }{{ + name: "file exists", + ok: true, + err: nil, + setup: func(c *gc.C, dir string) { + path := filepath.Join(dir, cloudconfig.FileNameBootstrapParams) + err := os.WriteFile(path, []byte("test"), 0644) + c.Assert(err, jc.ErrorIsNil) + }, + }, { + name: "file does not exist", + ok: false, + err: nil, + setup: func(c *gc.C, dir string) {}, + }} + + for _, test := range tests { + c.Logf("test %q", test.name) + + dir := c.MkDir() + s.agentConfig.EXPECT().DataDir().Return(dir) + + test.setup(c, dir) + + ok, err := BootstrapParamsFileExists(s.agentConfig) + if test.err != nil { + c.Assert(err, gc.ErrorMatches, test.err.Error()) + continue + } + + c.Assert(err, jc.ErrorIsNil) + c.Check(ok, gc.Equals, test.ok) + } +} diff --git a/worker/bootstrap/worker.go b/worker/bootstrap/worker.go index 6755fd86b7e..89051fbe051 100644 --- a/worker/bootstrap/worker.go +++ b/worker/bootstrap/worker.go @@ -52,12 +52,12 @@ type LegacyState interface { // WorkerConfig encapsulates the configuration options for the // bootstrap worker. type WorkerConfig struct { - Agent agent.Agent - ObjectStoreGetter ObjectStoreGetter - ControllerConfigService ControllerConfigService - BootstrapUnlocker gate.Unlocker - AgentBinaryUploader AgentBinaryBootstrapFunc - CompletesBootstrap CompletesBootstrapFunc + Agent agent.Agent + ObjectStoreGetter ObjectStoreGetter + ControllerConfigService ControllerConfigService + BootstrapUnlocker gate.Unlocker + AgentBinaryUploader AgentBinaryBootstrapFunc + RemoveBootstrapParamsFile RemoveBootstrapParamsFileFunc // Deprecated: This is only here, until we can remove the state layer. State LegacyState @@ -82,8 +82,8 @@ func (c *WorkerConfig) Validate() error { if c.AgentBinaryUploader == nil { return errors.NotValidf("nil AgentBinaryUploader") } - if c.CompletesBootstrap == nil { - return errors.NotValidf("nil CompletesBootstrap") + if c.RemoveBootstrapParamsFile == nil { + return errors.NotValidf("nil RemoveBootstrapParamsFile") } if c.Logger == nil { return errors.NotValidf("nil Logger") @@ -147,7 +147,7 @@ func (w *bootstrapWorker) loop() error { // Complete the bootstrap, only after this is complete do we unlock the // bootstrap gate. - if err := w.cfg.CompletesBootstrap(agentConfig); err != nil { + if err := w.cfg.RemoveBootstrapParamsFile(agentConfig); err != nil { return errors.Trace(err) } diff --git a/worker/bootstrap/worker_test.go b/worker/bootstrap/worker_test.go index da197a004a3..a959adf68cb 100644 --- a/worker/bootstrap/worker_test.go +++ b/worker/bootstrap/worker_test.go @@ -85,9 +85,9 @@ func (s *workerSuite) newWorker(c *gc.C) worker.Worker { AgentBinaryUploader: func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, Logger) error { return nil }, - CompletesBootstrap: func(agent.Config) error { return nil }, - State: &state.State{}, - ControllerConfigService: s.controllerConfigService, + RemoveBootstrapParamsFile: func(agent.Config) error { return nil }, + State: &state.State{}, + ControllerConfigService: s.controllerConfigService, }, s.states) c.Assert(err, jc.ErrorIsNil) return w From 1a9d5c21673fd4a9aeaf68b93e7d07c58da67f65 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Tue, 5 Dec 2023 08:46:00 +0000 Subject: [PATCH 6/7] Ensure that we clean up the params file is CAAS Luckily this was picked up in the github PR tests, but shows directly that manifold config just is useless when it comes to strict typing. The solution is to use field less structs, so any change to the underlying struct is an error. Except that the readability dramatically reduces. Of all the things in go, this is the worst part of it. I'd love to have a vet or keyword that ensure all fields are added for certain types. --- cmd/jujud/agent/machine/manifolds.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/jujud/agent/machine/manifolds.go b/cmd/jujud/agent/machine/manifolds.go index 2d53867d0cb..04691776fb3 100644 --- a/cmd/jujud/agent/machine/manifolds.go +++ b/cmd/jujud/agent/machine/manifolds.go @@ -1060,6 +1060,7 @@ func CAASManifolds(config ManifoldsConfig) dependency.Manifolds { BootstrapGateName: isBootstrapGateName, AgentBinaryUploader: bootstrap.CAASAgentBinaryUploader, BootstrapParamsFileExists: bootstrap.BootstrapParamsFileExists, + RemoveBootstrapParamsFile: bootstrap.RemoveBootstrapParamsFile, Logger: loggo.GetLogger("juju.worker.bootstrap"), })), From 97996d7fb4adc731a5cf5699b7b8b4f6471ddec1 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Tue, 5 Dec 2023 15:29:23 +0000 Subject: [PATCH 7/7] Use new flag service The flag service is passed through the bootstrap worker so that we can find out if the bootstrap is required or not. This should fix the issue we have with bootstrapping on k8s. --- cmd/jujud/agent/machine/manifolds.go | 34 +++---- domain/autocert/state/state.go | 2 +- domain/flag/service/service.go | 8 +- domain/flag/service/service_test.go | 12 +++ domain/flag/state/state.go | 3 + domain/flag/state/state_test.go | 4 +- internal/servicefactory/interface.go | 3 + worker/bootstrap/bootstrap_mock_test.go | 54 ++++++++++- worker/bootstrap/manifold.go | 91 +++++++---------- worker/bootstrap/manifold_test.go | 58 +---------- worker/bootstrap/package_test.go | 4 +- worker/bootstrap/worker.go | 28 +++--- worker/bootstrap/worker_test.go | 12 ++- .../servicefactory_mock_test.go | 97 ++++++++++++------- .../servicefactory_mock_test.go | 45 ++++++--- 15 files changed, 258 insertions(+), 197 deletions(-) diff --git a/cmd/jujud/agent/machine/manifolds.go b/cmd/jujud/agent/machine/manifolds.go index 04691776fb3..707ff0b58cd 100644 --- a/cmd/jujud/agent/machine/manifolds.go +++ b/cmd/jujud/agent/machine/manifolds.go @@ -835,15 +835,14 @@ func IAASManifolds(config ManifoldsConfig) dependency.Manifolds { manifolds := dependency.Manifolds{ // Bootstrap worker is responsible for setting up the initial machine. bootstrapName: ifDatabaseUpgradeComplete(bootstrap.Manifold(bootstrap.ManifoldConfig{ - AgentName: agentName, - StateName: stateName, - ObjectStoreName: objectStoreName, - ServiceFactoryName: serviceFactoryName, - BootstrapGateName: isBootstrapGateName, - AgentBinaryUploader: bootstrap.IAASAgentBinaryUploader, - BootstrapParamsFileExists: bootstrap.BootstrapParamsFileExists, - RemoveBootstrapParamsFile: bootstrap.RemoveBootstrapParamsFile, - Logger: loggo.GetLogger("juju.worker.bootstrap"), + AgentName: agentName, + StateName: stateName, + ObjectStoreName: objectStoreName, + ServiceFactoryName: serviceFactoryName, + BootstrapGateName: isBootstrapGateName, + AgentBinaryUploader: bootstrap.IAASAgentBinaryUploader, + RequiresBootstrap: bootstrap.RequiresBootstrap, + Logger: loggo.GetLogger("juju.worker.bootstrap"), })), toolsVersionCheckerName: ifNotMigrating(toolsversionchecker.Manifold(toolsversionchecker.ManifoldConfig{ @@ -1053,15 +1052,14 @@ func CAASManifolds(config ManifoldsConfig) dependency.Manifolds { return mergeManifolds(config, dependency.Manifolds{ // Bootstrap worker is responsible for setting up the initial machine. bootstrapName: ifDatabaseUpgradeComplete(bootstrap.Manifold(bootstrap.ManifoldConfig{ - AgentName: agentName, - StateName: stateName, - ObjectStoreName: objectStoreName, - ServiceFactoryName: serviceFactoryName, - BootstrapGateName: isBootstrapGateName, - AgentBinaryUploader: bootstrap.CAASAgentBinaryUploader, - BootstrapParamsFileExists: bootstrap.BootstrapParamsFileExists, - RemoveBootstrapParamsFile: bootstrap.RemoveBootstrapParamsFile, - Logger: loggo.GetLogger("juju.worker.bootstrap"), + AgentName: agentName, + StateName: stateName, + ObjectStoreName: objectStoreName, + ServiceFactoryName: serviceFactoryName, + BootstrapGateName: isBootstrapGateName, + AgentBinaryUploader: bootstrap.CAASAgentBinaryUploader, + RequiresBootstrap: bootstrap.RequiresBootstrap, + Logger: loggo.GetLogger("juju.worker.bootstrap"), })), // TODO(caas) - when we support HA, only want this on primary diff --git a/domain/autocert/state/state.go b/domain/autocert/state/state.go index 0c7a0c192a9..ea84841b25b 100644 --- a/domain/autocert/state/state.go +++ b/domain/autocert/state/state.go @@ -84,7 +84,7 @@ WHERE name = $M.name` if err := db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { return errors.Trace(tx.Query(ctx, s, sqlair.M{"name": name}).Get(&row)) }); err != nil { - if err.Error() == sql.ErrNoRows.Error() { + if errors.Is(err, sql.ErrNoRows) { return nil, errors.Annotatef(errors.NotFound, "autocert %s", name) } return nil, errors.Annotate(err, "querying autocert cache") diff --git a/domain/flag/service/service.go b/domain/flag/service/service.go index 0140da571de..74dbae4876f 100644 --- a/domain/flag/service/service.go +++ b/domain/flag/service/service.go @@ -5,6 +5,8 @@ package service import ( "context" + + "github.com/juju/errors" ) // State describes retrieval and persistence methods for storage. @@ -32,5 +34,9 @@ func (s *Service) SetFlag(ctx context.Context, flag string, value bool) error { // GetFlag returns the value of a flag. func (s *Service) GetFlag(ctx context.Context, flag string) (bool, error) { - return s.st.GetFlag(ctx, flag) + value, err := s.st.GetFlag(ctx, flag) + if err != nil && !errors.Is(err, errors.NotFound) { + return false, err + } + return value, nil } diff --git a/domain/flag/service/service_test.go b/domain/flag/service/service_test.go index 5eb4e94ae5d..3542eb46bc4 100644 --- a/domain/flag/service/service_test.go +++ b/domain/flag/service/service_test.go @@ -6,6 +6,7 @@ package service import ( "context" + "github.com/juju/errors" "github.com/juju/testing" jc "github.com/juju/testing/checkers" gomock "go.uber.org/mock/gomock" @@ -41,6 +42,17 @@ func (s *serviceSuite) TestGetFlag(c *gc.C) { c.Check(value, jc.IsTrue) } +func (s *serviceSuite) TestGetFlagNotFound(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetFlag(gomock.Any(), "foo").Return(false, errors.NotFoundf("flag")) + + service := NewService(s.state) + value, err := service.GetFlag(context.Background(), "foo") + c.Assert(err, jc.ErrorIsNil) + c.Check(value, jc.IsFalse) +} + func (s *serviceSuite) setupMocks(c *gc.C) *gomock.Controller { ctrl := gomock.NewController(c) diff --git a/domain/flag/state/state.go b/domain/flag/state/state.go index 718063c9d22..530cbd318ec 100644 --- a/domain/flag/state/state.go +++ b/domain/flag/state/state.go @@ -83,6 +83,9 @@ WHERE name = ?; err = db.StdTxn(ctx, func(ctx context.Context, tx *sql.Tx) error { row := tx.QueryRowContext(ctx, query, flag) if err := row.Scan(&value); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return errors.NotFoundf("flag %q", flag) + } return errors.Trace(err) } return nil diff --git a/domain/flag/state/state_test.go b/domain/flag/state/state_test.go index 51860eaa0d3..db4614fe5ab 100644 --- a/domain/flag/state/state_test.go +++ b/domain/flag/state/state_test.go @@ -5,8 +5,8 @@ package state import ( "context" - "database/sql" + "github.com/juju/errors" jc "github.com/juju/testing/checkers" gc "gopkg.in/check.v1" @@ -30,7 +30,7 @@ func (s *stateSuite) SetUpTest(c *gc.C) { func (s *stateSuite) TestGetFlagNotFound(c *gc.C) { value, err := s.state.GetFlag(context.Background(), "foo") - c.Assert(err, jc.ErrorIs, sql.ErrNoRows) + c.Assert(err, jc.ErrorIs, errors.NotFound) c.Assert(value, jc.IsFalse) } diff --git a/internal/servicefactory/interface.go b/internal/servicefactory/interface.go index 00f831618ad..2212a61e7b6 100644 --- a/internal/servicefactory/interface.go +++ b/internal/servicefactory/interface.go @@ -10,6 +10,7 @@ import ( controllernodeservice "github.com/juju/juju/domain/controllernode/service" credentialservice "github.com/juju/juju/domain/credential/service" externalcontrollerservice "github.com/juju/juju/domain/externalcontroller/service" + flagservice "github.com/juju/juju/domain/flag/service" modelservice "github.com/juju/juju/domain/model/service" modelconfigservice "github.com/juju/juju/domain/modelconfig/service" modeldefaultsservice "github.com/juju/juju/domain/modeldefaults/service" @@ -45,6 +46,8 @@ type ControllerServiceFactory interface { // Primarily used for agent blob store. Although can be used for other // blob related operations. AgentObjectStore() *objectstoreservice.Service + // Flag returns the flag service. + Flag() *flagservice.Service } // ModelServiceFactory provides access to the services required by the diff --git a/worker/bootstrap/bootstrap_mock_test.go b/worker/bootstrap/bootstrap_mock_test.go index 66d1b20e345..babf0562532 100644 --- a/worker/bootstrap/bootstrap_mock_test.go +++ b/worker/bootstrap/bootstrap_mock_test.go @@ -1,5 +1,5 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: github.com/juju/juju/worker/bootstrap (interfaces: ControllerConfigService,ObjectStoreGetter,LegacyState) +// Source: github.com/juju/juju/worker/bootstrap (interfaces: ControllerConfigService,FlagService,ObjectStoreGetter,LegacyState) // Package bootstrap is a generated GoMock package. package bootstrap @@ -52,6 +52,58 @@ func (mr *MockControllerConfigServiceMockRecorder) ControllerConfig(arg0 interfa return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControllerConfig", reflect.TypeOf((*MockControllerConfigService)(nil).ControllerConfig), arg0) } +// MockFlagService is a mock of FlagService interface. +type MockFlagService struct { + ctrl *gomock.Controller + recorder *MockFlagServiceMockRecorder +} + +// MockFlagServiceMockRecorder is the mock recorder for MockFlagService. +type MockFlagServiceMockRecorder struct { + mock *MockFlagService +} + +// NewMockFlagService creates a new mock instance. +func NewMockFlagService(ctrl *gomock.Controller) *MockFlagService { + mock := &MockFlagService{ctrl: ctrl} + mock.recorder = &MockFlagServiceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockFlagService) EXPECT() *MockFlagServiceMockRecorder { + return m.recorder +} + +// GetFlag mocks base method. +func (m *MockFlagService) GetFlag(arg0 context.Context, arg1 string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetFlag", arg0, arg1) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetFlag indicates an expected call of GetFlag. +func (mr *MockFlagServiceMockRecorder) GetFlag(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetFlag", reflect.TypeOf((*MockFlagService)(nil).GetFlag), arg0, arg1) +} + +// SetFlag mocks base method. +func (m *MockFlagService) SetFlag(arg0 context.Context, arg1 string, arg2 bool) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SetFlag", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// SetFlag indicates an expected call of SetFlag. +func (mr *MockFlagServiceMockRecorder) SetFlag(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetFlag", reflect.TypeOf((*MockFlagService)(nil).SetFlag), arg0, arg1, arg2) +} + // MockObjectStoreGetter is a mock of ObjectStoreGetter interface. type MockObjectStoreGetter struct { ctrl *gomock.Controller diff --git a/worker/bootstrap/manifold.go b/worker/bootstrap/manifold.go index 2f14590fef7..8d930b97443 100644 --- a/worker/bootstrap/manifold.go +++ b/worker/bootstrap/manifold.go @@ -6,8 +6,6 @@ package bootstrap import ( "context" "io" - "os" - "path/filepath" "github.com/juju/errors" "github.com/juju/worker/v3" @@ -16,7 +14,6 @@ import ( "github.com/juju/juju/agent" "github.com/juju/juju/core/objectstore" "github.com/juju/juju/internal/bootstrap" - "github.com/juju/juju/internal/cloudconfig" "github.com/juju/juju/internal/servicefactory" "github.com/juju/juju/state/binarystorage" "github.com/juju/juju/worker/common" @@ -25,6 +22,12 @@ import ( "github.com/juju/juju/worker/state" ) +const ( + // BootstrapFlag is the name of the flag that is used to check if the + // bootstrap process has completed. + BootstrapFlag = "bootstrapped" +) + // Logger represents the logging methods called. type Logger interface { Errorf(message string, args ...any) @@ -50,15 +53,6 @@ type BinaryAgentStorage interface { // AgentBinaryBootstrapFunc is the function that is used to populate the tools. type AgentBinaryBootstrapFunc func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, Logger) error -// BootstrapParamsFileExistsFunc is the function that is used to check if the -// bootstrap param file exists. We expect that the file exists for only the -// bootstrap controller node. -type BootstrapParamsFileExistsFunc func(agent.Config) (bool, error) - -// RemoveBootstrapParamsFileFunc is the function that is used to complete the bootstrap -// process. -type RemoveBootstrapParamsFileFunc func(agent.Config) error - // ManifoldConfig defines the configuration for the trace manifold. type ManifoldConfig struct { AgentName string @@ -67,10 +61,9 @@ type ManifoldConfig struct { BootstrapGateName string ServiceFactoryName string - Logger Logger - AgentBinaryUploader AgentBinaryBootstrapFunc - BootstrapParamsFileExists BootstrapParamsFileExistsFunc - RemoveBootstrapParamsFile RemoveBootstrapParamsFileFunc + Logger Logger + AgentBinaryUploader AgentBinaryBootstrapFunc + RequiresBootstrap func(context.Context, FlagService) (bool, error) } // Validate validates the manifold configuration. @@ -96,11 +89,8 @@ func (cfg ManifoldConfig) Validate() error { if cfg.AgentBinaryUploader == nil { return errors.NotValidf("nil AgentBinaryUploader") } - if cfg.BootstrapParamsFileExists == nil { - return errors.NotValidf("nil BootstrapParamsFileExists") - } - if cfg.RemoveBootstrapParamsFile == nil { - return errors.NotValidf("nil RemoveBootstrapParamsFile") + if cfg.RequiresBootstrap == nil { + return errors.NotValidf("nil RequiresBootstrap") } return nil } @@ -115,25 +105,31 @@ func Manifold(config ManifoldConfig) dependency.Manifold { config.BootstrapGateName, config.ServiceFactoryName, }, - Start: func(context dependency.Context) (worker.Worker, error) { + Start: func(ctx dependency.Context) (worker.Worker, error) { if err := config.Validate(); err != nil { return nil, errors.Trace(err) } var bootstrapUnlocker gate.Unlocker - if err := context.Get(config.BootstrapGateName, &bootstrapUnlocker); err != nil { + if err := ctx.Get(config.BootstrapGateName, &bootstrapUnlocker); err != nil { return nil, errors.Trace(err) } var a agent.Agent - if err := context.Get(config.AgentName, &a); err != nil { + if err := ctx.Get(config.AgentName, &a); err != nil { return nil, err } + var controllerServiceFactory servicefactory.ControllerServiceFactory + if err := ctx.Get(config.ServiceFactoryName, &controllerServiceFactory); err != nil { + return nil, errors.Trace(err) + } + // If the controller application exists, then we don't need to // bootstrap. Uninstall the worker, as we don't need it running // anymore. - if ok, err := config.BootstrapParamsFileExists(a.CurrentConfig()); err != nil { + flagService := controllerServiceFactory.Flag() + if ok, err := config.RequiresBootstrap(context.TODO(), flagService); err != nil { return nil, errors.Trace(err) } else if !ok { bootstrapUnlocker.Unlock() @@ -141,17 +137,12 @@ func Manifold(config ManifoldConfig) dependency.Manifold { } var objectStoreGetter workerobjectstore.ObjectStoreGetter - if err := context.Get(config.ObjectStoreName, &objectStoreGetter); err != nil { - return nil, errors.Trace(err) - } - - var controllerServiceFactory servicefactory.ControllerServiceFactory - if err := context.Get(config.ServiceFactoryName, &controllerServiceFactory); err != nil { + if err := ctx.Get(config.ObjectStoreName, &objectStoreGetter); err != nil { return nil, errors.Trace(err) } var stTracker state.StateTracker - if err := context.Get(config.StateName, &stTracker); err != nil { + if err := ctx.Get(config.StateName, &stTracker); err != nil { return nil, errors.Trace(err) } @@ -169,14 +160,14 @@ func Manifold(config ManifoldConfig) dependency.Manifold { } w, err := NewWorker(WorkerConfig{ - Agent: a, - ObjectStoreGetter: objectStoreGetter, - ControllerConfigService: controllerServiceFactory.ControllerConfig(), - State: systemState, - BootstrapUnlocker: bootstrapUnlocker, - AgentBinaryUploader: config.AgentBinaryUploader, - RemoveBootstrapParamsFile: config.RemoveBootstrapParamsFile, - Logger: config.Logger, + Agent: a, + ObjectStoreGetter: objectStoreGetter, + ControllerConfigService: controllerServiceFactory.ControllerConfig(), + FlagService: flagService, + State: systemState, + BootstrapUnlocker: bootstrapUnlocker, + AgentBinaryUploader: config.AgentBinaryUploader, + Logger: config.Logger, }) if err != nil { _ = stTracker.Done() @@ -209,22 +200,12 @@ func IAASAgentBinaryUploader(ctx context.Context, dataDir string, storageService return bootstrap.PopulateAgentBinary(ctx, dataDir, storage, logger) } -// BootstrapParamsFileExists returns true if the bootstrap params file exists. -// It is expected at the end of bootstrap that the file is removed. -func BootstrapParamsFileExists(config agent.Config) (bool, error) { - _, err := os.Stat(filepath.Join(config.DataDir(), cloudconfig.FileNameBootstrapParams)) +// RequiresBootstrap is the function that is used to check if the bootstrap +// process has completed. +func RequiresBootstrap(ctx context.Context, flagService FlagService) (bool, error) { + bootstrapped, err := flagService.GetFlag(ctx, BootstrapFlag) if err != nil { - if os.IsNotExist(err) { - return false, nil - } return false, errors.Trace(err) } - return true, nil -} - -// RemoveBootstrapParamsFile removes the bootstrap params file, completing the -// bootstrap process. -func RemoveBootstrapParamsFile(config agent.Config) error { - // Remove the bootstrap params file. - return os.Remove(filepath.Join(config.DataDir(), cloudconfig.FileNameBootstrapParams)) + return !bootstrapped, nil } diff --git a/worker/bootstrap/manifold_test.go b/worker/bootstrap/manifold_test.go index 013732f9d4d..e7d1e96d5c0 100644 --- a/worker/bootstrap/manifold_test.go +++ b/worker/bootstrap/manifold_test.go @@ -5,8 +5,6 @@ package bootstrap import ( "context" - "os" - "path/filepath" "github.com/juju/errors" jc "github.com/juju/testing/checkers" @@ -14,10 +12,8 @@ import ( dependencytesting "github.com/juju/worker/v3/dependency/testing" gc "gopkg.in/check.v1" - "github.com/juju/juju/agent" "github.com/juju/juju/core/objectstore" "github.com/juju/juju/domain/servicefactory/testing" - "github.com/juju/juju/internal/cloudconfig" ) type manifoldSuite struct { @@ -52,11 +48,7 @@ func (s *manifoldSuite) TestValidateConfig(c *gc.C) { c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) cfg = s.getConfig() - cfg.BootstrapParamsFileExists = nil - c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) - - cfg = s.getConfig() - cfg.RemoveBootstrapParamsFile = nil + cfg.RequiresBootstrap = nil c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) } @@ -71,8 +63,9 @@ func (s *manifoldSuite) getConfig() ManifoldConfig { AgentBinaryUploader: func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, Logger) error { return nil }, - BootstrapParamsFileExists: func(agent.Config) (bool, error) { return false, nil }, - RemoveBootstrapParamsFile: func(agent.Config) error { return nil }, + RequiresBootstrap: func(context.Context, FlagService) (bool, error) { + return false, nil + }, } } @@ -102,46 +95,3 @@ func (s *manifoldSuite) TestStartAlreadyBootstrapped(c *gc.C) { _, err := Manifold(s.getConfig()).Start(s.getContext()) c.Assert(err, jc.ErrorIs, dependency.ErrUninstall) } - -func (s *manifoldSuite) TestBootstrapParamsFileExists(c *gc.C) { - defer s.setupMocks(c).Finish() - - tests := []struct { - name string - ok bool - err error - setup func(*gc.C, string) - }{{ - name: "file exists", - ok: true, - err: nil, - setup: func(c *gc.C, dir string) { - path := filepath.Join(dir, cloudconfig.FileNameBootstrapParams) - err := os.WriteFile(path, []byte("test"), 0644) - c.Assert(err, jc.ErrorIsNil) - }, - }, { - name: "file does not exist", - ok: false, - err: nil, - setup: func(c *gc.C, dir string) {}, - }} - - for _, test := range tests { - c.Logf("test %q", test.name) - - dir := c.MkDir() - s.agentConfig.EXPECT().DataDir().Return(dir) - - test.setup(c, dir) - - ok, err := BootstrapParamsFileExists(s.agentConfig) - if test.err != nil { - c.Assert(err, gc.ErrorMatches, test.err.Error()) - continue - } - - c.Assert(err, jc.ErrorIsNil) - c.Check(ok, gc.Equals, test.ok) - } -} diff --git a/worker/bootstrap/package_test.go b/worker/bootstrap/package_test.go index c49d1f8417a..f01cdeec0ed 100644 --- a/worker/bootstrap/package_test.go +++ b/worker/bootstrap/package_test.go @@ -19,7 +19,7 @@ import ( //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination state_mock_test.go github.com/juju/juju/worker/state StateTracker //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination objectstore_mock_test.go github.com/juju/juju/core/objectstore ObjectStore //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination lock_mock_test.go github.com/juju/juju/worker/gate Unlocker -//go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination bootstrap_mock_test.go github.com/juju/juju/worker/bootstrap ControllerConfigService,ObjectStoreGetter,LegacyState +//go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination bootstrap_mock_test.go github.com/juju/juju/worker/bootstrap ControllerConfigService,FlagService,ObjectStoreGetter,LegacyState func TestPackage(t *testing.T) { gc.TestingT(t) @@ -36,6 +36,7 @@ type baseSuite struct { objectStoreGetter *MockObjectStoreGetter bootstrapUnlocker *MockUnlocker controllerConfigService *MockControllerConfigService + flagService *MockFlagService logger Logger } @@ -51,6 +52,7 @@ func (s *baseSuite) setupMocks(c *gc.C) *gomock.Controller { s.objectStoreGetter = NewMockObjectStoreGetter(ctrl) s.bootstrapUnlocker = NewMockUnlocker(ctrl) s.controllerConfigService = NewMockControllerConfigService(ctrl) + s.flagService = NewMockFlagService(ctrl) s.logger = jujujujutesting.NewCheckLogger(c) diff --git a/worker/bootstrap/worker.go b/worker/bootstrap/worker.go index 89051fbe051..342898a074b 100644 --- a/worker/bootstrap/worker.go +++ b/worker/bootstrap/worker.go @@ -29,6 +29,13 @@ type ControllerConfigService interface { ControllerConfig(context.Context) (controller.Config, error) } +// FlagService is the interface that is used to set the value of a +// flag. +type FlagService interface { + GetFlag(context.Context, string) (bool, error) + SetFlag(context.Context, string, bool) error +} + // ObjectStoreGetter is the interface that is used to get a object store. type ObjectStoreGetter interface { // GetObjectStore returns a object store for the given namespace. @@ -52,12 +59,12 @@ type LegacyState interface { // WorkerConfig encapsulates the configuration options for the // bootstrap worker. type WorkerConfig struct { - Agent agent.Agent - ObjectStoreGetter ObjectStoreGetter - ControllerConfigService ControllerConfigService - BootstrapUnlocker gate.Unlocker - AgentBinaryUploader AgentBinaryBootstrapFunc - RemoveBootstrapParamsFile RemoveBootstrapParamsFileFunc + Agent agent.Agent + ObjectStoreGetter ObjectStoreGetter + ControllerConfigService ControllerConfigService + FlagService FlagService + BootstrapUnlocker gate.Unlocker + AgentBinaryUploader AgentBinaryBootstrapFunc // Deprecated: This is only here, until we can remove the state layer. State LegacyState @@ -82,8 +89,8 @@ func (c *WorkerConfig) Validate() error { if c.AgentBinaryUploader == nil { return errors.NotValidf("nil AgentBinaryUploader") } - if c.RemoveBootstrapParamsFile == nil { - return errors.NotValidf("nil RemoveBootstrapParamsFile") + if c.FlagService == nil { + return errors.NotValidf("nil FlagService") } if c.Logger == nil { return errors.NotValidf("nil Logger") @@ -145,9 +152,8 @@ func (w *bootstrapWorker) loop() error { return errors.Trace(err) } - // Complete the bootstrap, only after this is complete do we unlock the - // bootstrap gate. - if err := w.cfg.RemoveBootstrapParamsFile(agentConfig); err != nil { + // Set the bootstrap flag, to indicate that the bootstrap has completed. + if err := w.cfg.FlagService.SetFlag(ctx, BootstrapFlag, true); err != nil { return errors.Trace(err) } diff --git a/worker/bootstrap/worker_test.go b/worker/bootstrap/worker_test.go index a959adf68cb..714a6fd7e32 100644 --- a/worker/bootstrap/worker_test.go +++ b/worker/bootstrap/worker_test.go @@ -14,7 +14,6 @@ import ( gomock "go.uber.org/mock/gomock" gc "gopkg.in/check.v1" - agent "github.com/juju/juju/agent" controller "github.com/juju/juju/controller" "github.com/juju/juju/core/objectstore" "github.com/juju/juju/state" @@ -36,6 +35,7 @@ func (s *workerSuite) TestKilled(c *gc.C) { s.expectControllerConfig() s.expectAgentConfig(c) s.expectObjectStoreGetter() + s.expectBootstrapFlagSet() w := s.newWorker(c) defer workertest.DirtyKill(c, w) @@ -85,9 +85,9 @@ func (s *workerSuite) newWorker(c *gc.C) worker.Worker { AgentBinaryUploader: func(context.Context, string, BinaryAgentStorageService, objectstore.ObjectStore, Logger) error { return nil }, - RemoveBootstrapParamsFile: func(agent.Config) error { return nil }, - State: &state.State{}, - ControllerConfigService: s.controllerConfigService, + State: &state.State{}, + ControllerConfigService: s.controllerConfigService, + FlagService: s.flagService, }, s.states) c.Assert(err, jc.ErrorIsNil) return w @@ -127,3 +127,7 @@ func (s *workerSuite) expectControllerConfig() { func (s *workerSuite) expectObjectStoreGetter() { s.objectStoreGetter.EXPECT().GetObjectStore(gomock.Any(), gomock.Any()).Return(s.objectStore, nil) } + +func (s *workerSuite) expectBootstrapFlagSet() { + s.flagService.EXPECT().SetFlag(gomock.Any(), BootstrapFlag, true).Return(nil) +} diff --git a/worker/servicefactory/servicefactory_mock_test.go b/worker/servicefactory/servicefactory_mock_test.go index 279cfa70503..61b0c7e013f 100644 --- a/worker/servicefactory/servicefactory_mock_test.go +++ b/worker/servicefactory/servicefactory_mock_test.go @@ -13,12 +13,13 @@ import ( service2 "github.com/juju/juju/domain/controllernode/service" service3 "github.com/juju/juju/domain/credential/service" service4 "github.com/juju/juju/domain/externalcontroller/service" - service5 "github.com/juju/juju/domain/model/service" - service6 "github.com/juju/juju/domain/modelconfig/service" - service7 "github.com/juju/juju/domain/modeldefaults/service" - service8 "github.com/juju/juju/domain/modelmanager/service" - service9 "github.com/juju/juju/domain/objectstore/service" - service10 "github.com/juju/juju/domain/upgrade/service" + service5 "github.com/juju/juju/domain/flag/service" + service6 "github.com/juju/juju/domain/model/service" + service7 "github.com/juju/juju/domain/modelconfig/service" + service8 "github.com/juju/juju/domain/modeldefaults/service" + service9 "github.com/juju/juju/domain/modelmanager/service" + service10 "github.com/juju/juju/domain/objectstore/service" + service11 "github.com/juju/juju/domain/upgrade/service" servicefactory "github.com/juju/juju/internal/servicefactory" gomock "go.uber.org/mock/gomock" ) @@ -47,10 +48,10 @@ func (m *MockControllerServiceFactory) EXPECT() *MockControllerServiceFactoryMoc } // AgentObjectStore mocks base method. -func (m *MockControllerServiceFactory) AgentObjectStore() *service9.Service { +func (m *MockControllerServiceFactory) AgentObjectStore() *service10.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "AgentObjectStore") - ret0, _ := ret[0].(*service9.Service) + ret0, _ := ret[0].(*service10.Service) return ret0 } @@ -144,11 +145,25 @@ func (mr *MockControllerServiceFactoryMockRecorder) ExternalController() *gomock return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExternalController", reflect.TypeOf((*MockControllerServiceFactory)(nil).ExternalController)) } +// Flag mocks base method. +func (m *MockControllerServiceFactory) Flag() *service5.Service { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Flag") + ret0, _ := ret[0].(*service5.Service) + return ret0 +} + +// Flag indicates an expected call of Flag. +func (mr *MockControllerServiceFactoryMockRecorder) Flag() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Flag", reflect.TypeOf((*MockControllerServiceFactory)(nil).Flag)) +} + // Model mocks base method. -func (m *MockControllerServiceFactory) Model() *service5.Service { +func (m *MockControllerServiceFactory) Model() *service6.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Model") - ret0, _ := ret[0].(*service5.Service) + ret0, _ := ret[0].(*service6.Service) return ret0 } @@ -159,10 +174,10 @@ func (mr *MockControllerServiceFactoryMockRecorder) Model() *gomock.Call { } // ModelDefaults mocks base method. -func (m *MockControllerServiceFactory) ModelDefaults() *service7.Service { +func (m *MockControllerServiceFactory) ModelDefaults() *service8.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ModelDefaults") - ret0, _ := ret[0].(*service7.Service) + ret0, _ := ret[0].(*service8.Service) return ret0 } @@ -173,10 +188,10 @@ func (mr *MockControllerServiceFactoryMockRecorder) ModelDefaults() *gomock.Call } // ModelManager mocks base method. -func (m *MockControllerServiceFactory) ModelManager() *service8.Service { +func (m *MockControllerServiceFactory) ModelManager() *service9.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ModelManager") - ret0, _ := ret[0].(*service8.Service) + ret0, _ := ret[0].(*service9.Service) return ret0 } @@ -187,10 +202,10 @@ func (mr *MockControllerServiceFactoryMockRecorder) ModelManager() *gomock.Call } // Upgrade mocks base method. -func (m *MockControllerServiceFactory) Upgrade() *service10.Service { +func (m *MockControllerServiceFactory) Upgrade() *service11.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Upgrade") - ret0, _ := ret[0].(*service10.Service) + ret0, _ := ret[0].(*service11.Service) return ret0 } @@ -224,10 +239,10 @@ func (m *MockModelServiceFactory) EXPECT() *MockModelServiceFactoryMockRecorder } // Config mocks base method. -func (m *MockModelServiceFactory) Config(arg0 service6.ModelDefaultsProvider) *service6.Service { +func (m *MockModelServiceFactory) Config(arg0 service7.ModelDefaultsProvider) *service7.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Config", arg0) - ret0, _ := ret[0].(*service6.Service) + ret0, _ := ret[0].(*service7.Service) return ret0 } @@ -238,10 +253,10 @@ func (mr *MockModelServiceFactoryMockRecorder) Config(arg0 interface{}) *gomock. } // ObjectStore mocks base method. -func (m *MockModelServiceFactory) ObjectStore() *service9.Service { +func (m *MockModelServiceFactory) ObjectStore() *service10.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ObjectStore") - ret0, _ := ret[0].(*service9.Service) + ret0, _ := ret[0].(*service10.Service) return ret0 } @@ -275,10 +290,10 @@ func (m *MockServiceFactory) EXPECT() *MockServiceFactoryMockRecorder { } // AgentObjectStore mocks base method. -func (m *MockServiceFactory) AgentObjectStore() *service9.Service { +func (m *MockServiceFactory) AgentObjectStore() *service10.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "AgentObjectStore") - ret0, _ := ret[0].(*service9.Service) + ret0, _ := ret[0].(*service10.Service) return ret0 } @@ -317,10 +332,10 @@ func (mr *MockServiceFactoryMockRecorder) Cloud() *gomock.Call { } // Config mocks base method. -func (m *MockServiceFactory) Config(arg0 service6.ModelDefaultsProvider) *service6.Service { +func (m *MockServiceFactory) Config(arg0 service7.ModelDefaultsProvider) *service7.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Config", arg0) - ret0, _ := ret[0].(*service6.Service) + ret0, _ := ret[0].(*service7.Service) return ret0 } @@ -386,11 +401,25 @@ func (mr *MockServiceFactoryMockRecorder) ExternalController() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExternalController", reflect.TypeOf((*MockServiceFactory)(nil).ExternalController)) } +// Flag mocks base method. +func (m *MockServiceFactory) Flag() *service5.Service { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Flag") + ret0, _ := ret[0].(*service5.Service) + return ret0 +} + +// Flag indicates an expected call of Flag. +func (mr *MockServiceFactoryMockRecorder) Flag() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Flag", reflect.TypeOf((*MockServiceFactory)(nil).Flag)) +} + // Model mocks base method. -func (m *MockServiceFactory) Model() *service5.Service { +func (m *MockServiceFactory) Model() *service6.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Model") - ret0, _ := ret[0].(*service5.Service) + ret0, _ := ret[0].(*service6.Service) return ret0 } @@ -401,10 +430,10 @@ func (mr *MockServiceFactoryMockRecorder) Model() *gomock.Call { } // ModelDefaults mocks base method. -func (m *MockServiceFactory) ModelDefaults() *service7.Service { +func (m *MockServiceFactory) ModelDefaults() *service8.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ModelDefaults") - ret0, _ := ret[0].(*service7.Service) + ret0, _ := ret[0].(*service8.Service) return ret0 } @@ -415,10 +444,10 @@ func (mr *MockServiceFactoryMockRecorder) ModelDefaults() *gomock.Call { } // ModelManager mocks base method. -func (m *MockServiceFactory) ModelManager() *service8.Service { +func (m *MockServiceFactory) ModelManager() *service9.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ModelManager") - ret0, _ := ret[0].(*service8.Service) + ret0, _ := ret[0].(*service9.Service) return ret0 } @@ -429,10 +458,10 @@ func (mr *MockServiceFactoryMockRecorder) ModelManager() *gomock.Call { } // ObjectStore mocks base method. -func (m *MockServiceFactory) ObjectStore() *service9.Service { +func (m *MockServiceFactory) ObjectStore() *service10.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ObjectStore") - ret0, _ := ret[0].(*service9.Service) + ret0, _ := ret[0].(*service10.Service) return ret0 } @@ -443,10 +472,10 @@ func (mr *MockServiceFactoryMockRecorder) ObjectStore() *gomock.Call { } // Upgrade mocks base method. -func (m *MockServiceFactory) Upgrade() *service10.Service { +func (m *MockServiceFactory) Upgrade() *service11.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Upgrade") - ret0, _ := ret[0].(*service10.Service) + ret0, _ := ret[0].(*service11.Service) return ret0 } diff --git a/worker/upgradedatabase/servicefactory_mock_test.go b/worker/upgradedatabase/servicefactory_mock_test.go index 6574239c001..d8e3390106a 100644 --- a/worker/upgradedatabase/servicefactory_mock_test.go +++ b/worker/upgradedatabase/servicefactory_mock_test.go @@ -13,11 +13,12 @@ import ( service2 "github.com/juju/juju/domain/controllernode/service" service3 "github.com/juju/juju/domain/credential/service" service4 "github.com/juju/juju/domain/externalcontroller/service" - service5 "github.com/juju/juju/domain/model/service" - service6 "github.com/juju/juju/domain/modeldefaults/service" - service7 "github.com/juju/juju/domain/modelmanager/service" - service8 "github.com/juju/juju/domain/objectstore/service" - service9 "github.com/juju/juju/domain/upgrade/service" + service5 "github.com/juju/juju/domain/flag/service" + service6 "github.com/juju/juju/domain/model/service" + service7 "github.com/juju/juju/domain/modeldefaults/service" + service8 "github.com/juju/juju/domain/modelmanager/service" + service9 "github.com/juju/juju/domain/objectstore/service" + service10 "github.com/juju/juju/domain/upgrade/service" gomock "go.uber.org/mock/gomock" ) @@ -45,10 +46,10 @@ func (m *MockControllerServiceFactory) EXPECT() *MockControllerServiceFactoryMoc } // AgentObjectStore mocks base method. -func (m *MockControllerServiceFactory) AgentObjectStore() *service8.Service { +func (m *MockControllerServiceFactory) AgentObjectStore() *service9.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "AgentObjectStore") - ret0, _ := ret[0].(*service8.Service) + ret0, _ := ret[0].(*service9.Service) return ret0 } @@ -142,11 +143,25 @@ func (mr *MockControllerServiceFactoryMockRecorder) ExternalController() *gomock return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExternalController", reflect.TypeOf((*MockControllerServiceFactory)(nil).ExternalController)) } +// Flag mocks base method. +func (m *MockControllerServiceFactory) Flag() *service5.Service { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Flag") + ret0, _ := ret[0].(*service5.Service) + return ret0 +} + +// Flag indicates an expected call of Flag. +func (mr *MockControllerServiceFactoryMockRecorder) Flag() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Flag", reflect.TypeOf((*MockControllerServiceFactory)(nil).Flag)) +} + // Model mocks base method. -func (m *MockControllerServiceFactory) Model() *service5.Service { +func (m *MockControllerServiceFactory) Model() *service6.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Model") - ret0, _ := ret[0].(*service5.Service) + ret0, _ := ret[0].(*service6.Service) return ret0 } @@ -157,10 +172,10 @@ func (mr *MockControllerServiceFactoryMockRecorder) Model() *gomock.Call { } // ModelDefaults mocks base method. -func (m *MockControllerServiceFactory) ModelDefaults() *service6.Service { +func (m *MockControllerServiceFactory) ModelDefaults() *service7.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ModelDefaults") - ret0, _ := ret[0].(*service6.Service) + ret0, _ := ret[0].(*service7.Service) return ret0 } @@ -171,10 +186,10 @@ func (mr *MockControllerServiceFactoryMockRecorder) ModelDefaults() *gomock.Call } // ModelManager mocks base method. -func (m *MockControllerServiceFactory) ModelManager() *service7.Service { +func (m *MockControllerServiceFactory) ModelManager() *service8.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ModelManager") - ret0, _ := ret[0].(*service7.Service) + ret0, _ := ret[0].(*service8.Service) return ret0 } @@ -185,10 +200,10 @@ func (mr *MockControllerServiceFactoryMockRecorder) ModelManager() *gomock.Call } // Upgrade mocks base method. -func (m *MockControllerServiceFactory) Upgrade() *service9.Service { +func (m *MockControllerServiceFactory) Upgrade() *service10.Service { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Upgrade") - ret0, _ := ret[0].(*service9.Service) + ret0, _ := ret[0].(*service10.Service) return ret0 }