refactor: wire model config service through storage backend #18136

barrettj12 · 2024-09-24T16:45:18Z

The storageBackend in state is still using Mongo model config. As part of our work on the model config epic, this needs to be removed.

To reduce churn, I have refactored the original storageBackend into two separate structs:

storageBackend, which includes all the storage backend functionality that doesn't require model config
storageConfigBackend, which augments storageBackend with a model config service

This allows us to clearly see which methods and applications actually require model config, and which don't. As it turns out, there are only 10 or so usages of the storage backend that require model config, the other ~40 don't need it at all.

We've also had to wire a model config service through a significant number of state methods, including the add machine and add application code. This is similar to what was done with the prechecker in #16987.

The testing factory now requires a model config service to make applications, units or machines. I've added a .WithModelConfigService method that returns a copy of the factory with a model config service.

Checklist

Code style: imports ordered, good names, simple structure, etc
Comments saying why design decisions were made
Go unit tests, with comments saying what you're testing
~~[ ] Integration tests, with comments saying what you're testing~~
~~[ ] doc.go added or updated in changed packages~~

QA steps

This change affects many different parts of Juju. Ensure that basic Juju functionality continues to work (bootstrap, add model, deploy, destroy, migration).

In theory, we should test that the storage-default-block-source and storage-default-filesystem-source model config keys work as expected. However, these keys do not seem to work on the tip of main, and they do not work here either.

Links

Jira card: JUJU-6679

hmlanigan

Still need to finish qa

apiserver/facades/agent/caasapplication/mock_test.go

hmlanigan · 2024-09-30T21:28:07Z

apiserver/facades/client/client/api_test.go

+// modelConfigService is a convenience function to get the controller model's
+// model config service inside a test.


question: why are we using the controller's model config in a non controller test?

The controller model is already being used literally everywhere in this test suite:

juju/apiserver/facades/client/client/status_test.go

Line 487 in 3253078

f, release := s.NewFactory(c, s.ControllerModelUUID())

I was just following the pattern.

Remember we are only reading two model config keys, and in this test suite, they shouldn't have an effect, so I'm not sure it's worth worrying about too much.

apiserver/facades/controller/crossmodelrelations/mock_test.go

hmlanigan · 2024-09-30T21:42:15Z

apiserver/facades/controller/firewaller/firewaller_base_test.go

@@ -52,14 +52,16 @@ func (s *firewallerBaseSuite) SetUpTest(c *gc.C) {
 	// Note that the specific machine ids allocated are assumed
 	// to be numerically consecutive from zero.
 	st := s.ControllerModel(c).State()
+	modelConfigService := s.ControllerDomainServices(c).Config()


This is inconsistent between the tests, some places a helper method is used. Consistently is better for mental load on these tests.

Between which tests?

In this case, the model config service was only needed inside the SetUpTest (here) and inside a single test case. Hence I didn't think it was necessary to add a helper method here.

hmlanigan · 2024-09-30T22:31:26Z

state/storage.go

@@ -109,14 +108,37 @@ func NewStorageBackend(st *State) (*storageBackend, error) {
 	return sb, nil
 }

+// NewStorageConfigBackend creates a backend for managing storage with a model
+// config service.
+func NewStorageConfigBackend(


While I understand why this is being done here, this is bad practice in general. It only works here as the StorageBackend logic will be replaced with the move to domains.

tlm · 2024-10-01T05:33:09Z

PR #18042 fixes the model config keys.

tlm · 2024-10-02T06:02:24Z

agent/agentbootstrap/bootstrap.go

-		DisplayName:             stateParams.BootstrapMachineDisplayName,
-	})
+	m, err := st.AddOneMachine(
+		&modelConfigShim{cfg: stateParams.ControllerModelConfig},


This could be highly problematic as stateParams.ControllerModelConfig is not the final form of model config. It is just a suggestion of something that the client would like to see. I think this could be fine to use but not great.

@SimonRichardson are you able to confirm if there is a nice way for us to reach back into dqlite and pull the modelconfig after the bootstrap ops?

This is only going to be reading two model config keys - storage-default-block-source and storage-default-filesystem-source.

@tlm is indeed correct, we don't have access to dqlite. This is by design. We do not want to expose the database outside of the dependency engine, as it can lead to flakey tests and incorrect modeling. The solution is to move this to either the following:

juju/agent/agentbootstrap/bootstrap.go

Lines 269 to 271 in 06a46d4

databaseBootstrapOptions = append(databaseBootstrapOptions,

machinebootstrap.InsertMachine(agent.BootstrapControllerId),

)

or

juju/internal/worker/bootstrap/worker.go

Lines 179 to 265 in 06a46d4

func (w *bootstrapWorker) loop() error {

// Report the initial started state.

w.reportInternalState(stateStarted)

ctx, cancel := w.scopedContext()

defer cancel()

if err := w.seedMacaroonConfig(ctx); err != nil {

return errors.Annotatef(err, "initialising macaroon bakery config")

}

// Insert all the initial users into the state.

if err := w.seedInitialUsers(ctx); err != nil {

return errors.Annotatef(err, "inserting initial users")

}

agentConfig := w.cfg.Agent.CurrentConfig()

dataDir := agentConfig.DataDir()

// Seed the agent binary to the object store.

cleanup, err := w.seedAgentBinary(ctx, dataDir)

if err != nil {

return errors.Trace(err)

}

// Seed the controller charm to the object store.

bootstrapParams, err := w.bootstrapParams(ctx, dataDir)

if err != nil {

return errors.Annotatef(err, "getting bootstrap params")

}

// Create the user specified storage pools.

if err := w.seedStoragePools(ctx, bootstrapParams.StoragePools); err != nil {

return errors.Annotate(err, "seeding storage pools")

}

controllerConfig, err := w.cfg.ControllerConfigService.ControllerConfig(ctx)

if err != nil {

return errors.Trace(err)

}

if err := w.seedControllerCharm(ctx, dataDir, bootstrapParams); err != nil {

return errors.Trace(err)

}

// Retrieve controller addresses needed to set the API host ports.

bootstrapAddresses, err := w.cfg.BootstrapAddressFinder(ctx, bootstrapParams.BootstrapMachineInstanceId)

if err != nil {

return errors.Trace(err)

}

servingInfo, ok := agentConfig.StateServingInfo()

if !ok {

return errors.Errorf("state serving information not available")

}

// Load spaces from the underlying substrate.

if err := w.cfg.NetworkService.ReloadSpaces(ctx); err != nil {

if !errors.Is(err, errors.NotSupported) {

return errors.Trace(err)

}

w.logger.Debugf("reload spaces not supported due to a non-networking environement")

}

if err := w.seedInitialAuthorizedKeys(ctx, bootstrapParams.ControllerModelAuthorizedKeys); err != nil {

return errors.Trace(err)

}

// Convert the provider addresses that we got from the bootstrap instance

// to space ID decorated addresses.

if err := w.initAPIHostPorts(ctx, controllerConfig, bootstrapAddresses, servingInfo.APIPort); err != nil {

return errors.Trace(err)

}

// Set the bootstrap flag, to indicate that the bootstrap has completed.

if err := w.cfg.FlagService.SetFlag(ctx, flags.BootstrapFlag, true, flags.BootstrapFlagDescription); err != nil {

return errors.Trace(err)

}

// Cleanup only after the bootstrap flag has been set.

cleanup()

w.reportInternalState(stateCompleted)

w.cfg.BootstrapUnlocker.Unlock()

return nil

}

Those are your official two options.

I'll approve this change based on an item in the risk register and a comment stating this AddMachine call should be moved to the bootstrap worker.

SimonRichardson · 2024-10-02T15:10:08Z

internal/testing/factory/factory.go

+func (f *Factory) WithModelConfigService(s state.ModelConfigService) *Factory {
+	f.modelConfigService = s
+	return f
+}


The factory testing pattern should be removed and/or replaced with either mocks or specific integration tests.

Considering we're trying to close out the model config, I'm ok with this, but collective activism should be removing this.

SimonRichardson

Thanks for the grind.

We need to seriously pair down the testing factory, it's growing larger, when we're wanting to reduce it and ultimately remove it.

The original storageBackend has been refactored into two separate structs: - storageBackend, which includes all the storage backend functionality that doesn't require model config - storageConfigBackend, which augments storageBackend with a model config service A model config service has been wired through many state methods.

barrettj12 · 2024-10-02T19:57:40Z

/merge

barrettj12 force-pushed the modelcfg-storagebackend2 branch from a76d4cf to b152140 Compare September 24, 2024 20:06

hpidcock added the 4.0 label Sep 25, 2024

barrettj12 requested review from tlm, hmlanigan and SimonRichardson September 26, 2024 20:59

barrettj12 force-pushed the modelcfg-storagebackend2 branch from 1c77049 to 3253078 Compare September 27, 2024 18:50

hmlanigan reviewed Sep 30, 2024

View reviewed changes

barrettj12 requested a review from hmlanigan October 1, 2024 14:41

hmlanigan approved these changes Oct 1, 2024

View reviewed changes

tlm reviewed Oct 2, 2024

View reviewed changes

SimonRichardson reviewed Oct 2, 2024

View reviewed changes

SimonRichardson approved these changes Oct 2, 2024

View reviewed changes

barrettj12 force-pushed the modelcfg-storagebackend2 branch from cdbf4bd to 12648fd Compare October 2, 2024 15:20

barrettj12 force-pushed the modelcfg-storagebackend2 branch from 12648fd to 676ef45 Compare October 2, 2024 15:23

jujubot merged commit ae6701f into juju:main Oct 2, 2024
19 of 20 checks passed

barrettj12 deleted the modelcfg-storagebackend2 branch October 2, 2024 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: wire model config service through storage backend #18136

refactor: wire model config service through storage backend #18136

barrettj12 commented Sep 24, 2024 •

edited

Loading

hmlanigan left a comment

hmlanigan Sep 30, 2024

barrettj12 Oct 1, 2024

hmlanigan Sep 30, 2024

barrettj12 Oct 1, 2024

hmlanigan Sep 30, 2024

tlm commented Oct 1, 2024

tlm Oct 2, 2024

barrettj12 Oct 2, 2024

SimonRichardson Oct 2, 2024

SimonRichardson Oct 2, 2024

SimonRichardson Oct 2, 2024

SimonRichardson left a comment

barrettj12 commented Oct 2, 2024

		// modelConfigService is a convenience function to get the controller model's
		// model config service inside a test.

	databaseBootstrapOptions = append(databaseBootstrapOptions,
	machinebootstrap.InsertMachine(agent.BootstrapControllerId),
	)

	func (w *bootstrapWorker) loop() error {
	// Report the initial started state.
	w.reportInternalState(stateStarted)

	ctx, cancel := w.scopedContext()
	defer cancel()

	if err := w.seedMacaroonConfig(ctx); err != nil {
	return errors.Annotatef(err, "initialising macaroon bakery config")
	}

	// Insert all the initial users into the state.
	if err := w.seedInitialUsers(ctx); err != nil {
	return errors.Annotatef(err, "inserting initial users")
	}

	agentConfig := w.cfg.Agent.CurrentConfig()
	dataDir := agentConfig.DataDir()

	// Seed the agent binary to the object store.
	cleanup, err := w.seedAgentBinary(ctx, dataDir)
	if err != nil {
	return errors.Trace(err)
	}

	// Seed the controller charm to the object store.
	bootstrapParams, err := w.bootstrapParams(ctx, dataDir)
	if err != nil {
	return errors.Annotatef(err, "getting bootstrap params")
	}

	// Create the user specified storage pools.
	if err := w.seedStoragePools(ctx, bootstrapParams.StoragePools); err != nil {
	return errors.Annotate(err, "seeding storage pools")
	}

	controllerConfig, err := w.cfg.ControllerConfigService.ControllerConfig(ctx)
	if err != nil {
	return errors.Trace(err)
	}

	if err := w.seedControllerCharm(ctx, dataDir, bootstrapParams); err != nil {
	return errors.Trace(err)
	}

	// Retrieve controller addresses needed to set the API host ports.
	bootstrapAddresses, err := w.cfg.BootstrapAddressFinder(ctx, bootstrapParams.BootstrapMachineInstanceId)
	if err != nil {
	return errors.Trace(err)
	}

	servingInfo, ok := agentConfig.StateServingInfo()
	if !ok {
	return errors.Errorf("state serving information not available")
	}

	// Load spaces from the underlying substrate.
	if err := w.cfg.NetworkService.ReloadSpaces(ctx); err != nil {
	if !errors.Is(err, errors.NotSupported) {
	return errors.Trace(err)
	}
	w.logger.Debugf("reload spaces not supported due to a non-networking environement")
	}

	if err := w.seedInitialAuthorizedKeys(ctx, bootstrapParams.ControllerModelAuthorizedKeys); err != nil {
	return errors.Trace(err)
	}

	// Convert the provider addresses that we got from the bootstrap instance
	// to space ID decorated addresses.
	if err := w.initAPIHostPorts(ctx, controllerConfig, bootstrapAddresses, servingInfo.APIPort); err != nil {
	return errors.Trace(err)
	}

	// Set the bootstrap flag, to indicate that the bootstrap has completed.
	if err := w.cfg.FlagService.SetFlag(ctx, flags.BootstrapFlag, true, flags.BootstrapFlagDescription); err != nil {
	return errors.Trace(err)
	}

	// Cleanup only after the bootstrap flag has been set.
	cleanup()

	w.reportInternalState(stateCompleted)

	w.cfg.BootstrapUnlocker.Unlock()
	return nil
	}

refactor: wire model config service through storage backend #18136

refactor: wire model config service through storage backend #18136

Conversation

barrettj12 commented Sep 24, 2024 • edited Loading

Checklist

QA steps

Links

hmlanigan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlm commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonRichardson left a comment

Choose a reason for hiding this comment

barrettj12 commented Oct 2, 2024

barrettj12 commented Sep 24, 2024 •

edited

Loading