Skip to content

Commit

Permalink
Merge pull request juju#17560 from barrettj12/agenttools-modelsvc
Browse files Browse the repository at this point in the history
juju#17560

The agenttools facade now uses a model config service to access model config, and a model agent service to access the current agent version (as introduced in juju#17598). Unfortunately, we can't entirely remove state from this facade yet, as we still need it to update the model.

We introduce a new interfaces `ModelConfigService` and `ModelAgentService`, stored in `service.go`, and replaced with gomock-generated mocks in testing.

BREAKING CHANGE: agenttools now checks all preferred streams in preference order, starting with the user-specified stream in `agent-stream`. Previously, it always checked the "released" stream first, even if a custom `agent-stream` was set, and it would only check the user-defined stream if nothing was found in "released".

Additional changes:
- Add a new env variable to the Makefile (`JUJU_PUBLISH_STREAM`) that allows `make simplestreams` to publish to a specified stream.
- General modernising and code improvement on the agenttools facade, such as moving internal functions to methods on the API struct.

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing
- ~[ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~
- ~[ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~

## QA steps

<!-- Describe steps to verify that the change works. -->

This facade reads the following keys from model config: `agent-version`, `agent-stream`, `development`. `agent-version` is immutable, but the other two can be changed and observed to have an effect.

By default, the toolsversionchecker worker checks for updated tools every six hours. To QA this, we need to set this value to something much smaller, e.g. 5 seconds:
https://github.com/juju/juju/blob/65266db76c2f84e4a545a574d4db82d3388d89eb/internal/worker/toolsversionchecker/manifold.go#L47-L49

Publish simplestreams data into "proposed" and "devel" streams:
```bash
# set version to 4.0-beta5
$ JUJU_PUBLISH_STREAM=proposed make simplestreams
# set version to 4.0-beta5
$ JUJU_PUBLISH_STREAM=devel make simplestreams
# set version back to 4.0-beta4
$ make go-build
```

Start the simplestreams server:
```
$ cd _build/simplestreams/tools
$ python3 -m http.server
Serving HTTP on 0.0.0.0 port 8000 (http://0.0.0.0:8000/) ...
```

Use `hostname -I | cut -d' ' -f1` to get your computer's IP address, we will feed this to Juju config.

Bootstrap Juju with the given simplestreams server, and `agent-stream` initially set to "proposed":
```bash
juju bootstrap lxd c \n --config agent-metadata-url=http://192.168.1.25:8000/ \n --config agent-stream=proposed
```

Switch to the `controller` model and run `juju status`. We should see that an upgrade to 4.0-beta5 is available.
```
$ juju status
Model Controller Cloud/Region Version Timestamp Notes
controller c localhost/localhost 4.0-beta4.1 17:02:06+12:00 upgrade available: 4.0-beta5
```

Now change the agent stream to `devel`, wait for the toolsversionchecker worker to update, and check that an upgrade to 4.0-beta6 is available.
```
$ juju model-config agent-stream=devel
$ juju status
Model Controller Cloud/Region Version Timestamp Notes
controller c localhost/localhost 4.0-beta4.1 17:02:06+12:00 upgrade available: 4.0-beta6
```

You can do the same with the `development` model config key and the `devel` stream, however observe that this key only has an effect for non-development versions of Juju, so you would have to try it with versions e.g. 4.0.0 and 4.0.1.


## Links

<!-- Link to all relevant specification, documentation, bug, issue or JIRA card. -->

**Jira card:** JUJU-6104
  • Loading branch information
jujubot authored Jul 4, 2024
2 parents 5e3648a + fe93736 commit ef73674
Show file tree
Hide file tree
Showing 7 changed files with 283 additions and 93 deletions.
12 changes: 8 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ BIN_DIR ?= ${BUILD_DIR}/${GOOS}_${GOARCH}/bin
# for built juju binaries.
JUJU_METADATA_SOURCE ?= ${BUILD_DIR}/simplestreams

# JUJU_PUBLISH_STREAM defines the simplestreams stream that we will publish
# agents to (default "released").
JUJU_PUBLISH_STREAM ?= "released"

# TEST_PACKAGE_LIST is the path to a file that is a newline delimited list of
# packages to test. This file must be sorted.
TEST_PACKAGE_LIST ?=
Expand All @@ -54,7 +58,7 @@ tool_platform_paths = $(addsuffix /${1},$(call bin_platform_paths,${2}))

# simplestream_paths takes a list of Go style platforms to calculate the
# paths to their respective simplestreams agent binary archives.
simplestream_paths = $(addprefix ${JUJU_METADATA_SOURCE}/, $(addprefix tools/released/juju-${JUJU_VERSION}-, $(addsuffix .tgz,$(subst /,-,${1}))))
simplestream_paths = $(addprefix ${JUJU_METADATA_SOURCE}/, $(addprefix tools/${JUJU_PUBLISH_STREAM}/juju-${JUJU_VERSION}-, $(addsuffix .tgz,$(subst /,-,${1}))))

# CLIENT_PACKAGE_PLATFORMS defines a white space seperated list of platforms
# to build the Juju client binaries for. Platforms are defined as GO style
Expand Down Expand Up @@ -406,14 +410,14 @@ ${BUILD_DIR}/%/bin/pebble: phony_explicit
# build for pebble
$(run_go_build)

${JUJU_METADATA_SOURCE}/tools/released/juju-${JUJU_VERSION}-%.tgz: phony_explicit juju $(BUILD_AGENT_TARGETS) $(BUILD_CGO_AGENT_TARGETS)
${JUJU_METADATA_SOURCE}/tools/${JUJU_PUBLISH_STREAM}/juju-${JUJU_VERSION}-%.tgz: phony_explicit juju $(BUILD_AGENT_TARGETS) $(BUILD_CGO_AGENT_TARGETS)
@echo "Packaging simplestream tools for juju ${JUJU_VERSION} on $*"
@mkdir -p ${JUJU_METADATA_SOURCE}/tools/released
@mkdir -p ${JUJU_METADATA_SOURCE}/tools/${JUJU_PUBLISH_STREAM}
@tar czf "$@" -C $(call bin_platform_paths,$(subst -,/,$*)) .

.PHONY: simplestreams
simplestreams: juju juju-metadata ${SIMPLESTREAMS_TARGETS}
@juju metadata generate-agent-binaries -d ${JUJU_METADATA_SOURCE} --clean --prevent-fallback ;
@juju metadata generate-agent-binaries -d ${JUJU_METADATA_SOURCE} --clean --prevent-fallback --stream ${JUJU_PUBLISH_STREAM} ;
@echo "\nRun export JUJU_METADATA_SOURCE=\"${JUJU_METADATA_SOURCE}\" if not defined in your env"

.PHONY: build
Expand Down
78 changes: 35 additions & 43 deletions apiserver/facades/controller/agenttools/agenttools.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,12 @@ import (
"github.com/juju/juju/apiserver/facade"
corelogger "github.com/juju/juju/core/logger"
"github.com/juju/juju/environs"
"github.com/juju/juju/environs/config"
"github.com/juju/juju/environs/simplestreams"
"github.com/juju/juju/environs/tools"
coretools "github.com/juju/juju/internal/tools"
"github.com/juju/juju/state"
)

var (
findTools = tools.FindTools
)

// AgentToolsAPI implements the API used by the machine model worker.
type AgentToolsAPI struct {
modelGetter ModelGetter
Expand All @@ -33,6 +28,9 @@ type AgentToolsAPI struct {
findTools toolsFinder
envVersionUpdate envVersionUpdater
logger corelogger.Logger

modelConfigService ModelConfigService
modelAgentService ModelAgentService
}

// NewAgentToolsAPI creates a new instance of the Model API.
Expand All @@ -43,14 +41,18 @@ func NewAgentToolsAPI(
envVersionUpdate func(*state.Model, version.Number) error,
authorizer facade.Authorizer,
logger corelogger.Logger,
modelConfigService ModelConfigService,
modelAgentService ModelAgentService,
) (*AgentToolsAPI, error) {
return &AgentToolsAPI{
modelGetter: modelGetter,
newEnviron: newEnviron,
authorizer: authorizer,
findTools: findTools,
envVersionUpdate: envVersionUpdate,
logger: logger,
modelGetter: modelGetter,
newEnviron: newEnviron,
authorizer: authorizer,
findTools: findTools,
envVersionUpdate: envVersionUpdate,
logger: logger,
modelConfigService: modelConfigService,
modelAgentService: modelAgentService,
}, nil
}

Expand All @@ -63,28 +65,25 @@ type newEnvironFunc func() (environs.Environ, error)
type toolsFinder func(context.Context, tools.SimplestreamsFetcher, environs.BootstrapEnviron, int, int, []string, coretools.Filter) (coretools.List, error)
type envVersionUpdater func(*state.Model, version.Number) error

func checkToolsAvailability(ctx context.Context, newEnviron newEnvironFunc, modelCfg *config.Config, finder toolsFinder) (version.Number, error) {
currentVersion, ok := modelCfg.AgentVersion()
if !ok || currentVersion == version.Zero {
return version.Zero, nil
func (api *AgentToolsAPI) checkToolsAvailability(ctx context.Context) (version.Number, error) {
currentVersion, err := api.modelAgentService.GetModelAgentVersion(ctx)
if err != nil {
return version.Zero, errors.Annotate(err, "getting agent version from service")
}

env, err := newEnviron()
env, err := api.newEnviron()
if err != nil {
return version.Zero, errors.Annotatef(err, "cannot make model")
return version.Zero, errors.Annotatef(err, "cannot make cloud provider")
}

ss := simplestreams.NewSimpleStreams(simplestreams.DefaultDataSourceFactory())

// finder receives major and minor as parameters as it uses them to filter versions and
// only return patches for the passed major.minor (from major.minor.patch).
// We'll try the released stream first, then fall back to the current configured stream
// if no released tools are found.
vers, err := finder(ctx, ss, env, currentVersion.Major, currentVersion.Minor, []string{tools.ReleasedStream}, coretools.Filter{})
preferredStream := tools.PreferredStreams(&currentVersion, modelCfg.Development(), modelCfg.AgentStream())[0]
if preferredStream != tools.ReleasedStream && errors.Cause(err) == coretools.ErrNoMatches {
vers, err = finder(ctx, ss, env, currentVersion.Major, currentVersion.Minor, []string{preferredStream}, coretools.Filter{})
modelCfg, err := api.modelConfigService.ModelConfig(ctx)
if err != nil {
return version.Zero, errors.Annotate(err, "cannot get model config")
}

preferredStreams := tools.PreferredStreams(&currentVersion, modelCfg.Development(), modelCfg.AgentStream())
vers, err := api.findTools(ctx, ss, env, currentVersion.Major, currentVersion.Minor, preferredStreams, coretools.Filter{})
if err != nil {
return version.Zero, errors.Annotatef(err, "cannot find available agent binaries")
}
Expand All @@ -94,25 +93,13 @@ func checkToolsAvailability(ctx context.Context, newEnviron newEnvironFunc, mode
return newest, nil
}

var modelConfig = func(e *state.Model) (*config.Config, error) {
return e.Config()
}

// Base implementation of envVersionUpdater
func envVersionUpdate(env *state.Model, ver version.Number) error {
return env.UpdateLatestToolsVersion(ver)
}

func updateToolsAvailability(ctx context.Context, modelGetter ModelGetter, newEnviron newEnvironFunc, finder toolsFinder, update envVersionUpdater, logger corelogger.Logger) error {
model, err := modelGetter.Model()
if err != nil {
return errors.Annotate(err, "cannot get model")
}
cfg, err := modelConfig(model)
if err != nil {
return errors.Annotate(err, "cannot get config")
}
ver, err := checkToolsAvailability(ctx, newEnviron, cfg, finder)
func (api *AgentToolsAPI) updateToolsAvailability(ctx context.Context) error {
ver, err := api.checkToolsAvailability(ctx)
if err != nil {
if errors.Is(err, errors.NotFound) {
// No newer tools, so exit silently.
Expand All @@ -121,10 +108,15 @@ func updateToolsAvailability(ctx context.Context, modelGetter ModelGetter, newEn
return errors.Annotate(err, "cannot get latest version")
}
if ver == version.Zero {
logger.Debugf("The lookup of agent binaries returned version Zero. This should only happen during bootstrap.")
api.logger.Debugf("The lookup of agent binaries returned version Zero. This should only happen during bootstrap.")
return nil
}
return update(model, ver)

model, err := api.modelGetter.Model()
if err != nil {
return errors.Annotate(err, "cannot get model")
}
return api.envVersionUpdate(model, ver)
}

// UpdateToolsAvailable invokes a lookup and further update in environ
Expand All @@ -133,5 +125,5 @@ func (api *AgentToolsAPI) UpdateToolsAvailable(ctx context.Context) error {
if !api.authorizer.AuthController() {
return apiservererrors.ErrPerm
}
return updateToolsAvailability(ctx, api.modelGetter, api.newEnviron, api.findTools, api.envVersionUpdate, api.logger)
return api.updateToolsAvailability(ctx)
}
105 changes: 60 additions & 45 deletions apiserver/facades/controller/agenttools/agenttools_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/juju/errors"
jc "github.com/juju/testing/checkers"
"github.com/juju/version/v2"
"go.uber.org/mock/gomock"
gc "gopkg.in/check.v1"

"github.com/juju/juju/environs"
Expand All @@ -24,27 +25,31 @@ var _ = gc.Suite(&AgentToolsSuite{})

type AgentToolsSuite struct {
coretesting.BaseSuite
modelConfigService *MockModelConfigService
modelAgentService *MockModelAgentService
}

type dummyEnviron struct {
environs.Environ
}

type configGetter struct {
cfg *config.Config
}

func (s *configGetter) ModelConfig() (*config.Config, error) {
return s.cfg, nil
func (s *AgentToolsSuite) setupMocks(c *gc.C) *gomock.Controller {
ctrl := gomock.NewController(c)
s.modelConfigService = NewMockModelConfigService(ctrl)
s.modelAgentService = NewMockModelAgentService(ctrl)
return ctrl
}

func (s *AgentToolsSuite) TestCheckTools(c *gc.C) {
sConfig := coretesting.FakeConfig()
sConfig = sConfig.Merge(coretesting.Attrs{
"agent-version": "2.5.0",
})
cfg, err := config.New(config.NoDefaults, sConfig)
defer s.setupMocks(c).Finish()

expVer, err := version.Parse("2.5.0")
c.Assert(err, jc.ErrorIsNil)
s.modelAgentService.EXPECT().GetModelAgentVersion(gomock.Any()).Return(expVer, nil)
modelConfig, err := config.New(config.NoDefaults, coretesting.FakeConfig())
c.Assert(err, jc.ErrorIsNil)
s.modelConfigService.EXPECT().ModelConfig(gomock.Any()).Return(modelConfig, nil)

var (
calledWithMajor, calledWithMinor int
)
Expand All @@ -59,20 +64,29 @@ func (s *AgentToolsSuite) TestCheckTools(c *gc.C) {
return coretools.List{&t}, nil
}

ver, err := checkToolsAvailability(context.Background(), getDummyEnviron, cfg, fakeToolFinder)
api, err := NewAgentToolsAPI(nil, getDummyEnviron, fakeToolFinder, nil, nil, loggertesting.WrapCheckLog(c), s.modelConfigService, s.modelAgentService)
c.Assert(err, jc.ErrorIsNil)

obtainedVer, err := api.checkToolsAvailability(context.Background())
c.Assert(err, jc.ErrorIsNil)
c.Assert(ver, gc.Not(gc.Equals), version.Zero)
c.Assert(ver, gc.Equals, version.Number{Major: 2, Minor: 5, Patch: 0})
c.Assert(obtainedVer, gc.Equals, expVer)
}

func (s *AgentToolsSuite) TestCheckToolsNonReleasedStream(c *gc.C) {
defer s.setupMocks(c).Finish()

expVer, err := version.Parse("2.5-alpha1")
c.Assert(err, jc.ErrorIsNil)
s.modelAgentService.EXPECT().GetModelAgentVersion(gomock.Any()).Return(expVer, nil)

sConfig := coretesting.FakeConfig()
sConfig = sConfig.Merge(coretesting.Attrs{
"agent-version": "2.5-alpha1",
"agent-stream": "proposed",
"agent-stream": "proposed",
})
cfg, err := config.New(config.NoDefaults, sConfig)
c.Assert(err, jc.ErrorIsNil)
s.modelConfigService.EXPECT().ModelConfig(gomock.Any()).Return(cfg, nil)

var (
calledWithMajor, calledWithMinor int
calledWithStreams [][]string
Expand All @@ -90,30 +104,31 @@ func (s *AgentToolsSuite) TestCheckToolsNonReleasedStream(c *gc.C) {
c.Assert(calledWithMinor, gc.Equals, 5)
return coretools.List{&t}, nil
}
ver, err := checkToolsAvailability(context.Background(), getDummyEnviron, cfg, fakeToolFinder)

api, err := NewAgentToolsAPI(nil, getDummyEnviron, fakeToolFinder, nil, nil, loggertesting.WrapCheckLog(c), s.modelConfigService, s.modelAgentService)
c.Assert(err, jc.ErrorIsNil)
c.Assert(calledWithStreams, gc.DeepEquals, [][]string{{"released"}, {"proposed"}})
c.Assert(ver, gc.Not(gc.Equals), version.Zero)
c.Assert(ver, gc.Equals, version.Number{Major: 2, Minor: 5, Patch: 0})
}

type mockState struct {
configGetter
obtainedVer, err := api.checkToolsAvailability(context.Background())
c.Assert(err, jc.ErrorIsNil)
c.Assert(calledWithStreams, gc.DeepEquals, [][]string{{"proposed", "released"}})
c.Assert(obtainedVer, gc.Equals, version.Number{Major: 2, Minor: 5, Patch: 0})
}

type mockState struct{}

func (e *mockState) Model() (*state.Model, error) {
return &state.Model{}, nil
}

func (s *AgentToolsSuite) TestUpdateToolsAvailability(c *gc.C) {
fakeModelConfig := func(_ *state.Model) (*config.Config, error) {
sConfig := coretesting.FakeConfig()
sConfig = sConfig.Merge(coretesting.Attrs{
"agent-version": "2.5.0",
})
return config.New(config.NoDefaults, sConfig)
}
s.PatchValue(&modelConfig, fakeModelConfig)
defer s.setupMocks(c).Finish()

expVer, err := version.Parse("2.5.0")
c.Assert(err, jc.ErrorIsNil)
s.modelAgentService.EXPECT().GetModelAgentVersion(gomock.Any()).Return(expVer, nil)
modelConfig, err := config.New(config.NoDefaults, coretesting.FakeConfig())
c.Assert(err, jc.ErrorIsNil)
s.modelConfigService.EXPECT().ModelConfig(gomock.Any()).Return(modelConfig, nil)

fakeToolFinder := func(_ context.Context, _ tools.SimplestreamsFetcher, _ environs.BootstrapEnviron, _ int, _ int, _ []string, _ coretools.Filter) (coretools.List, error) {
ver := version.Binary{Number: version.Number{Major: 2, Minor: 5, Patch: 2}}
Expand All @@ -129,24 +144,23 @@ func (s *AgentToolsSuite) TestUpdateToolsAvailability(c *gc.C) {
return nil
}

cfg, err := config.New(config.NoDefaults, coretesting.FakeConfig())
c.Assert(err, jc.ErrorIsNil)
err = updateToolsAvailability(context.Background(), &mockState{configGetter{cfg}}, getDummyEnviron, fakeToolFinder, fakeUpdate, loggertesting.WrapCheckLog(c))
api, err := NewAgentToolsAPI(&mockState{}, getDummyEnviron, fakeToolFinder, fakeUpdate, nil, loggertesting.WrapCheckLog(c), s.modelConfigService, s.modelAgentService)
c.Assert(err, jc.ErrorIsNil)

c.Assert(ver, gc.Not(gc.Equals), version.Zero)
err = api.updateToolsAvailability(context.Background())
c.Assert(err, jc.ErrorIsNil)
c.Assert(ver, gc.Equals, version.Number{Major: 2, Minor: 5, Patch: 2})
}

func (s *AgentToolsSuite) TestUpdateToolsAvailabilityNoMatches(c *gc.C) {
fakeModelConfig := func(_ *state.Model) (*config.Config, error) {
sConfig := coretesting.FakeConfig()
sConfig = sConfig.Merge(coretesting.Attrs{
"agent-version": "2.5.0",
})
return config.New(config.NoDefaults, sConfig)
}
s.PatchValue(&modelConfig, fakeModelConfig)
defer s.setupMocks(c).Finish()

expVer, err := version.Parse("2.5.0")
c.Assert(err, jc.ErrorIsNil)
s.modelAgentService.EXPECT().GetModelAgentVersion(gomock.Any()).Return(expVer, nil)
modelConfig, err := config.New(config.NoDefaults, coretesting.FakeConfig())
c.Assert(err, jc.ErrorIsNil)
s.modelConfigService.EXPECT().ModelConfig(gomock.Any()).Return(modelConfig, nil)

// No new tools available.
fakeToolFinder := func(_ context.Context, _ tools.SimplestreamsFetcher, _ environs.BootstrapEnviron, _ int, _ int, _ []string, _ coretools.Filter) (coretools.List, error) {
Expand All @@ -159,9 +173,10 @@ func (s *AgentToolsSuite) TestUpdateToolsAvailabilityNoMatches(c *gc.C) {
return nil
}

cfg, err := config.New(config.NoDefaults, coretesting.FakeConfig())
api, err := NewAgentToolsAPI(&mockState{}, getDummyEnviron, fakeToolFinder, fakeUpdate, nil, loggertesting.WrapCheckLog(c), s.modelConfigService, s.modelAgentService)
c.Assert(err, jc.ErrorIsNil)
err = updateToolsAvailability(context.Background(), &mockState{configGetter{cfg}}, getDummyEnviron, fakeToolFinder, fakeUpdate, loggertesting.WrapCheckLog(c))

err = api.updateToolsAvailability(context.Background())
c.Assert(err, jc.ErrorIsNil)
}

Expand Down
2 changes: 2 additions & 0 deletions apiserver/facades/controller/agenttools/package_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"github.com/juju/juju/testing"
)

//go:generate go run go.uber.org/mock/mockgen -typed -package agenttools -destination service_mock_test.go github.com/juju/juju/apiserver/facades/controller/agenttools ModelConfigService,ModelAgentService

func TestAll(t *stdtesting.T) {
testing.MgoTestPackage(t)
}
12 changes: 11 additions & 1 deletion apiserver/facades/controller/agenttools/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/juju/juju/apiserver/facade"
"github.com/juju/juju/environs"
"github.com/juju/juju/environs/tools"
"github.com/juju/juju/state/stateenvirons"
)

Expand All @@ -32,5 +33,14 @@ func newFacade(ctx facade.ModelContext) (*AgentToolsAPI, error) {
newEnviron := stateenvirons.GetNewEnvironFunc(environs.New)
return newEnviron(model, ctx.ServiceFactory().Cloud(), ctx.ServiceFactory().Credential())
}
return NewAgentToolsAPI(st, newEnviron, findTools, envVersionUpdate, ctx.Auth(), ctx.Logger().Child("model"))
return NewAgentToolsAPI(
st,
newEnviron,
tools.FindTools,
envVersionUpdate,
ctx.Auth(),
ctx.Logger().Child("model"),
ctx.ServiceFactory().Config(),
ctx.ServiceFactory().Agent(),
)
}
Loading

0 comments on commit ef73674

Please sign in to comment.