Skip to content

Commit

Permalink
fix: model machine info checks controller model machines
Browse files Browse the repository at this point in the history
Before this patch, any calls to MachineModelInfo() (located in
apiserver/common) would have actually be returning info from the
controller model machines.
This was clearly reproducible if you had a model with 2 machines (and a
no-ha controller), then a call to `juju show-model` would return with an
error stating that machine "1" wasn't found. Another way of reproducing
was to migrate that model.

The fix is to inject the correct MachineService on the modelmanager
facade.
  • Loading branch information
nvinuesa committed Dec 3, 2024
1 parent 5175f08 commit eed0b5c
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 1 deletion.
38 changes: 38 additions & 0 deletions apiserver/facades/client/modelmanager/mocks/service_mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 20 additions & 0 deletions apiserver/facades/client/modelmanager/modelinfo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,8 @@ func (s *modelInfoSuite) getAPI(c *gc.C) (*modelmanager.ModelManagerAPI, *gomock
modelInfoService := mocks.NewMockModelInfoService(ctrl)
mockModelDomainServices.EXPECT().ModelInfo().Return(modelInfoService)

mockModelDomainServices.EXPECT().Machine().Return(s.mockMachineService).AnyTimes()

modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(jujuversion.Current, nil)
modelInfoService.EXPECT().GetModelInfo(gomock.Any()).Return(coremodel.ReadOnlyModel{
AgentVersion: version.MustParse("1.99.9"),
Expand Down Expand Up @@ -435,6 +437,10 @@ func (s *modelInfoSuite) TestModelInfoWriteAccess(c *gc.C) {
api, ctrl := s.getAPIWithUser(c, mary)
defer ctrl.Finish()
maryName := coreusertesting.GenNewName(c, "mary")

s.mockMachineService = mocks.NewMockMachineService(ctrl)
s.mockModelDomainServices.EXPECT().Machine().Return(s.mockMachineService)

s.mockAccessService.EXPECT().ReadUserAccessLevelForTarget(gomock.Any(), user.NameFromTag(maryName), permission.ID{
ObjectType: permission.Model,
Key: s.st.model.cfg.UUID(),
Expand Down Expand Up @@ -569,6 +575,7 @@ func (s *modelInfoSuite) TestModelInfoErrorNoAccess(c *gc.C) {
func (s *modelInfoSuite) TestRunningMigration(c *gc.C) {
api, ctrl := s.getAPI(c)
defer ctrl.Finish()

s.mockSecretBackendService.EXPECT().BackendSummaryInfoForModel(gomock.Any(), coremodel.UUID(s.st.model.cfg.UUID())).Return(nil, nil)
s.mockModelService.EXPECT().GetModelUsers(gomock.Any(), coremodel.UUID(coretesting.ModelTag.Id())).Return(s.modelUserInfo, nil)
start := time.Now().Add(-20 * time.Minute)
Expand Down Expand Up @@ -625,6 +632,7 @@ func (s *modelInfoSuite) TestFailedMigration(c *gc.C) {
func (s *modelInfoSuite) TestNoMigration(c *gc.C) {
api, ctrl := s.getAPI(c)
defer ctrl.Finish()

s.mockSecretBackendService.EXPECT().BackendSummaryInfoForModel(gomock.Any(), coremodel.UUID(s.st.model.cfg.UUID())).Return(nil, nil)
s.mockModelService.EXPECT().GetModelUsers(gomock.Any(), coremodel.UUID(coretesting.ModelTag.Id())).Return(s.modelUserInfo, nil)
s.mockMachineService.EXPECT().GetMachineUUID(gomock.Any(), machine.Name("1")).Return("deadbeef1", nil)
Expand Down Expand Up @@ -734,6 +742,8 @@ func (s *modelInfoSuite) TestDeadModelWithGetModelInfoFailure(c *gc.C) {
modelDomainServices.EXPECT().Agent().Return(modelAgentService)
modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(jujuversion.Current, nil)

modelDomainServices.EXPECT().Machine().Return(s.mockMachineService)

s.assertSuccess(c, api, s.st.model.cfg.UUID(), state.Dead, life.Dead)
}

Expand All @@ -758,6 +768,8 @@ func (s *modelInfoSuite) TestDeadModelWithGetModelTargetAgentVersionFailure(c *g
modelDomainServices.EXPECT().Agent().Return(modelAgentService)
modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(version.Zero, errors.NotFoundf("model agent version"))

modelDomainServices.EXPECT().Machine().Return(s.mockMachineService)

s.assertSuccess(c, api, s.st.model.cfg.UUID(), state.Dead, life.Dead)
}

Expand Down Expand Up @@ -819,6 +831,8 @@ func (s *modelInfoSuite) TestDyingModelWithGetModelInfoFailure(c *gc.C) {
modelDomainServices.EXPECT().Agent().Return(modelAgentService)
modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(jujuversion.Current, nil)

modelDomainServices.EXPECT().Machine().Return(s.mockMachineService)

s.assertSuccess(c, api, s.st.model.cfg.UUID(), state.Dying, life.Dying)
}

Expand All @@ -843,6 +857,8 @@ func (s *modelInfoSuite) TestDyingModelWithGetModelTargetAgentVersionFailure(c *
modelDomainServices.EXPECT().Agent().Return(modelAgentService)
modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(jujuversion.Current, errors.NotFoundf("model agent version"))

modelDomainServices.EXPECT().Machine().Return(s.mockMachineService)

s.assertSuccess(c, api, s.st.model.cfg.UUID(), state.Dying, life.Dying)
}

Expand Down Expand Up @@ -920,6 +936,8 @@ func (s *modelInfoSuite) TestImportingModelWithGetModelInfoFailure(c *gc.C) {
modelDomainServices.EXPECT().Agent().Return(modelAgentService)
modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(jujuversion.Current, nil)

modelDomainServices.EXPECT().Machine().Return(s.mockMachineService)

s.assertSuccess(c, api, s.st.model.cfg.UUID(), state.Alive, life.Alive)
}

Expand All @@ -945,6 +963,8 @@ func (s *modelInfoSuite) TestImportingModelWithGetModelTargetAgentVersionFailure
modelDomainServices.EXPECT().Agent().Return(modelAgentService)
modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(version.Zero, errors.NotFoundf("model agent version"))

modelDomainServices.EXPECT().Machine().Return(s.mockMachineService)

s.assertSuccess(c, api, s.st.model.cfg.UUID(), state.Alive, life.Alive)
}

Expand Down
2 changes: 1 addition & 1 deletion apiserver/facades/client/modelmanager/modelmanager.go
Original file line number Diff line number Diff line change
Expand Up @@ -1192,7 +1192,7 @@ func (m *ModelManagerAPI) getModelInfo(ctx context.Context, tag names.ModelTag,
}
}
if canSeeMachinesAndSecrets {
if info.Machines, err = common.ModelMachineInfo(ctx, st, m.machineService); shouldErr(err) {
if info.Machines, err = common.ModelMachineInfo(ctx, st, modelDomainServices.Machine()); shouldErr(err) {
return params.ModelInfo{}, err
}
}
Expand Down
5 changes: 5 additions & 0 deletions apiserver/facades/client/modelmanager/modelmanager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -378,12 +378,15 @@ func (s *modelManagerSuite) expectCreateModelOnModelDB(
// Expect calls to get various model services.
modelInfoService := mocks.NewMockModelInfoService(ctrl)
networkService := mocks.NewMockNetworkService(ctrl)
machineService := mocks.NewMockMachineService(ctrl)

s.modelConfigService = mocks.NewMockModelConfigService(ctrl)
modelAgentService := mocks.NewMockModelAgentService(ctrl)
modelDomainServices.EXPECT().ModelInfo().Return(modelInfoService).AnyTimes()
modelDomainServices.EXPECT().Network().Return(networkService)
modelDomainServices.EXPECT().Config().Return(s.modelConfigService).AnyTimes()
modelDomainServices.EXPECT().Agent().Return(modelAgentService).AnyTimes()
modelDomainServices.EXPECT().Machine().Return(machineService)

// Expect calls to functions of the model services.
modelInfoService.EXPECT().CreateModel(gomock.Any(), s.controllerUUID)
Expand Down Expand Up @@ -1130,11 +1133,13 @@ func (s *modelManagerStateSuite) expectCreateModelStateSuite(
modelConfigService := mocks.NewMockModelConfigService(ctrl)
modelInfoService := mocks.NewMockModelInfoService(ctrl)
networkService := mocks.NewMockNetworkService(ctrl)
machineService := mocks.NewMockMachineService(ctrl)

modelDomainServices.EXPECT().Agent().Return(modelAgentService).AnyTimes()
modelDomainServices.EXPECT().Config().Return(modelConfigService).AnyTimes()
modelDomainServices.EXPECT().ModelInfo().Return(modelInfoService).AnyTimes()
modelDomainServices.EXPECT().Network().Return(networkService)
modelDomainServices.EXPECT().Machine().Return(machineService)

blockCommandService := mocks.NewMockBlockCommandService(ctrl)
modelDomainServices.EXPECT().BlockCommand().Return(blockCommandService).AnyTimes()
Expand Down
7 changes: 7 additions & 0 deletions apiserver/facades/client/modelmanager/services.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ type ModelDomainServices interface {
// Network returns the space service.
Network() NetworkService
BlockCommand() BlockCommandService

// Machine returns the machine service.
Machine() MachineService
}

// DomainServicesGetter is a factory for creating model services.
Expand Down Expand Up @@ -320,6 +323,10 @@ func (s domainServices) Network() NetworkService {
return s.domainServices.Network()
}

func (s domainServices) Machine() MachineService {
return s.domainServices.Machine()
}

func (s domainServices) BlockCommand() BlockCommandService {
return s.domainServices.BlockCommand()
}

0 comments on commit eed0b5c

Please sign in to comment.