refactor: drives model config into mongo from dqlite. #18042

tlm · 2024-09-06T04:56:00Z

This PR introduces provider based model config defaults into the model default service. Previously when calculating a model's default values for config we only considered config attributes owned by the provider and not defaults the provider wished to have opinions on for keys that are owned by the entire controller.

This has now been migrated into the ModelDefaults service through a new environs interface. The new interface was introduced so we did not have to keep using prepare config that would also require a cloud spec for validation.

Because of the above changes it is now possible to remove the Mongo code that has opinions about setting up model config on a newly created model and allow the domain services to drive the process end to end.

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

Bootstrap a controller to any provider. In this case I used LXD but the same principal will work for any provider.
Add a new model to the controller
Ask the controller for the model-config of the new model.
Confirm in the model config that the storage defaults for the provider have been set and are the correct value as per the code in the provider. In the case of LXD we are confirming that storage-default-filesystem-source is set to lxd.

Documentation changes

N/A

Links

Jira card: JUJU-6422

wallyworld

A couple of questions to start with.

environs/interface.go

wallyworld · 2024-09-25T04:56:46Z

environs/interface.go

@@ -60,6 +61,24 @@ type EnvironProvider interface {
 	PrepareConfig(context.Context, PrepareConfigParams) (*config.Config, error)
 }

+// ModelConfigProvider represents an interface that a [EnvironProvider] can
+// implement to provide opinions and defaults into a models config.
+type ModelConfigProvider interface {


We have ConfigSchemaSource which the providers all implement (nothing else implements it, just the providers).
I suspect the intent was to replace this interface? Why keep both; we should look to consolidate.

// ConfigSchemaSource instances provide information on config attributes // and the default attribute values. type ConfigSchemaSource interface { // ConfigSchema returns extra config attributes specific // to this provider only. ConfigSchema() schema.Fields // ConfigDefaults returns the default values for the // provider specific config attributes. ConfigDefaults() schema.Defaults }

It is the intent to replace this long term. It is used in lots of places and I don't think we are ready to pull it. I could try and do it in this PR but it will be a lot of noise.

Can we at least add a TODO or 3 to the affected places and ensure we follow up very soon (not long term) with a PR to do the fixing. This type of tech debt is not something we'd want to carry. The changes should essentially be boiler plate given were just adding a method to a new duplicate interface and all the implementors of the original interface also implement the new method.

wallyworld · 2024-09-25T05:03:17Z

internal/provider/azure/config.go

@@ -82,6 +82,14 @@ func (p azureEnvironProvider) ConfigDefaults() schema.Defaults {
 	return configDefaults
 }

+// ModelConfigDefaults provides a set of default model config attributes that
+// should be set on a models config if they have not been specified by the user.
+func (prov *azureEnvironProvider) ModelConfigDefaults(_ context.Context) (map[string]any, error) {


With this here, is there a reason why the corresponding code in PrepareConfig isn't removed?
There's probably an argument that PrepareConfig could be designed out now. It existed primarily to add in provider specific config to the model, which is essentially what this PR does. It also does validation of the cloud spec but I would argue that's a conflation of responsibilities.

// PrepareConfig prepares the configuration for a new model, based on // the provided arguments. PrepareConfig is expected to produce a // deterministic output. Any unique values should be based on the // "uuid" attribute of the base configuration. This is called for the // controller model during bootstrap, and also for new hosted models. PrepareConfig(context.Context, PrepareConfigParams) (*config.Config, error)

You are correct in every respect above. I want to remove it but I am trying to avoid busy work as this change is a little hard to pull out because of the client.

Can we add a TODO on the method declaration in the interface. At the least, we should follow up immediately with a PR to remove the unnecessary config manipulation, even if the cloud spec validation remains for now.

hmlanigan · 2024-09-25T18:37:06Z

not defaults the provider wished to have opinions on for keys that are owned by the entire controller.

How does this work in multi-cloud controllers? Could the different providers be in conflict?

hmlanigan

Model config has the storage value for lxd, however I expected the model defaults to have it from the start and it did not. What is the expectation here?

storage-default-filesystem-source is not available from model-defaults unless set first by the user.

$ juju model-defaults storage-default-filesystem-source
ERROR there are no default model values for "storage-default-filesystem-source"
$ juju add-model six
Added 'six' model on localhost/localhost with credential 'localhost' for user 'admin'
To use "juju ssh", "juju scp" and "juju debug-hooks" ssh public keys need to be added to the model with "juju add-ssh-key"
$ juju model-config storage-default-filesystem-source
lxd
$ juju model-defaults storage-default-filesystem-source=lxd
$ juju model-defaults storage-default-filesystem-source
Attribute                          Default  Controller
storage-default-filesystem-source  -        lxd

domain/modeldefaults/service/service.go

hmlanigan · 2024-09-25T19:24:24Z

domain/modeldefaults/service/service.go

+// model config provider exists for the supplied cloud type then a
+// [coreerrors.NotFound] error is returned. If the cloud type provider does not
+// support model config then a [coreerrors.NotSupported] error is returned.
+type ModelConfigProviderFunc func(string) (environs.ModelConfigProvider, error)


question: Why use a string here rather than an actual type for a cloud? Same for the ModelCloudType return value.

We don't have a dedicated type to represent this. We use strings everywhere. I agree it would be nice but I am avoiding the side quests at the moment.

hmlanigan · 2024-09-25T19:26:12Z

domain/modeldefaults/service/service.go

+}
+
+// ProviderModelConfigGetter returns a [ModelConfigProviderFunc] for
+// retrieving provider based model config  values.


Is this for ModelConfig or DefaultModelConfig?

hmlanigan · 2024-09-25T19:48:11Z

environs/interface.go

+	// that
+	// should be set on a models config if they have not been specified by the
+	// user.
+	ModelConfigDefaults(context.Context) (map[string]any, error)


question: why provide a context which is never used?

Trying to future proof. Defaults could be calculated in a number of ways include http calls to public api's (possible for ec2) to gather information. Trying to get ahead of the curve now and wire it up.

tlm · 2024-09-25T23:46:05Z

not defaults the provider wished to have opinions on for keys that are owned by the entire controller.

How does this work in multi-cloud controllers? Could the different providers be in conflict?

No we only ever consider the provider that is set for the model. We can only have one cloud per model. Conflicts can't happen.

tlm · 2024-09-25T23:58:56Z

Model config has the storage value for lxd, however I expected the model defaults to have it from the start and it did not. What is the expectation here?

storage-default-filesystem-source is not available from model-defaults unless set first by the user.

$ juju model-defaults storage-default-filesystem-source
ERROR there are no default model values for "storage-default-filesystem-source"
$ juju add-model six
Added 'six' model on localhost/localhost with credential 'localhost' for user 'admin'
To use "juju ssh", "juju scp" and "juju debug-hooks" ssh public keys need to be added to the model with "juju add-ssh-key"
$ juju model-config storage-default-filesystem-source
lxd
$ juju model-defaults storage-default-filesystem-source=lxd
$ juju model-defaults storage-default-filesystem-source
Attribute                          Default  Controller
storage-default-filesystem-source  -        lxd

I would consider this a bug. We have never considered the defaults of the provider in the model defaults command. If you run the same test on 3.6 it has the same result. Possible we could fix this in the model defaults work on the model manager soon.

This commit adds a new interface type to the environ package to describe providers that want to have opinions about a models configuration. I am introducing this change to place together all of the disparate funcs we currently have that mix concerns. This will make it easier to inject into the model defaults domain.

Add missing model config defaults func to the azure provider.

Adds missing model config defaults func to the dummy provider.

This commit adds support get getting provider based model config defaults in the DQlite domain services. Previously this was only happening for config attributes that the provider is extending model config with and not defaults the provider wanted to apply for controller wide attributes. The most common case of a provider wanting to do this is for storage. As well in this PR we are know longer letting the mongo based model config creation process take place. Instead we are using the DQlite domain services to perform the full generation and copying this result down into Mongo while we dual write. Because of this I have been able to remove the controller/modelmanager package that is no longer needed.

Have removed some agent version test by commenting them out. This will need to be address in future updates when moving models fully over to dqlite. Removed validation checks for model config in state layer as they are not needed anymore.

wallyworld · 2024-09-27T01:34:41Z

Model config has the storage value for lxd, however I expected the model defaults to have it from the start and it did not. What is the expectation here?
storage-default-filesystem-source is not available from model-defaults unless set first by the user.
$ juju model-defaults storage-default-filesystem-source
ERROR there are no default model values for "storage-default-filesystem-source"
$ juju add-model six
Added 'six' model on localhost/localhost with credential 'localhost' for user 'admin'
To use "juju ssh", "juju scp" and "juju debug-hooks" ssh public keys need to be added to the model with "juju add-ssh-key"
$ juju model-config storage-default-filesystem-source
lxd
$ juju model-defaults storage-default-filesystem-source=lxd
$ juju model-defaults storage-default-filesystem-source
Attribute                          Default  Controller
storage-default-filesystem-source  -        lxd
I would consider this a bug. We have never considered the defaults of the provider in the model defaults command. If you run the same test on 3.6 it has the same result. Possible we could fix this in the model defaults work on the model manager soon.

Yes, this is current behaviour - model defaults does not have the concept of default values for provider specific config. As seen, when a model is added, and the user doesn't specify anything for a provider config value, the provider has the option to insert a value at that point. We'll need to think carefully about how this could be implemented if we decide to do it, especially with regard to things like how the Source is reported, since now we're introducing potentially what's considered as a new source option.

wallyworld

We can land this since it's great progress, on the proviso that the 2 items of tech debt are addressed short term. Some tech debt we can live with, but here it's a fairly significant dilution of the architecture which we'd not want to carry.

wallyworld · 2024-09-27T01:37:37Z

domain/modeldefaults/service/service.go

+		}
+	}
+
+	return rval, nil
 }

 // ModelDefaults will return the default config values to be used for a model
 // and it's config. If no model for uuid is found then a error satisfying


Can we removed the unnecessary apostrophe as a drive by

wallyworld · 2024-09-27T01:38:58Z

domain/modeldefaults/service/service.go

+// change are layered in the form of a funnel where we apply the most granular
+// specific last. The current order is:


"the most granular specific last" doesn't really parse to me.
Perhaps tweak the wording a bit.

wallyworld · 2024-09-27T01:48:31Z

domain/modeldefaults/state/state.go

 	"github.com/juju/juju/environs/config"
+	interrors "github.com/juju/juju/internal/errors"


I've been trying to avoid importing both juju errors and the new errors package - my personal preference is to keep things consistent in a given file. Make the changes completely or not at all. Or land new work keeping juju errors and separately in a refactoring PR make the change. As a side effect, would avoid the "interrors" horror.

wallyworld · 2024-09-27T01:52:51Z

domain/modeldefaults/state/state.go

+		err := tx.Query(ctx, stmt, modelUUIDVal).Get(&result)
+		if errors.Is(err, sqlair.ErrNoRows) {
+			return interrors.Errorf(
+				"cannot get cloud type for model %q because model does not exist",


My strong preference is to not double up on the top level "what is this method doing" messaging in every single error. It's sufficient here to just surface the issue with this specific operation, ie "model doesn't exist". And below, the error would be "cannot get cloud type". Then, instead of just "return err" below when calling db.Txn(), you'd include the extra context at that point. The end result is the same but with much less verbosity and message duplication.

wallyworld · 2024-09-27T01:54:05Z

domain/modeldefaults/state/state.go

-		return nil, fmt.Errorf("getting cloud type of model %q cloud: %w", uuid, err)
-	}
-
-	provider, err := environs.Provider(cloudType)


Ugh, how the fark this this ever get landed in state. Glad to see it gone. State should never import environs.

wallyworld · 2024-09-27T01:58:23Z

environs/interface.go

@@ -60,6 +61,24 @@ type EnvironProvider interface {
 	PrepareConfig(context.Context, PrepareConfigParams) (*config.Config, error)
 }

+// ModelConfigProvider represents an interface that a [EnvironProvider] can
+// implement to provide opinions and defaults into a models config.
+type ModelConfigProvider interface {


Can we at least add a TODO or 3 to the affected places and ensure we follow up very soon (not long term) with a PR to do the fixing. This type of tech debt is not something we'd want to carry. The changes should essentially be boiler plate given were just adding a method to a new duplicate interface and all the implementors of the original interface also implement the new method.

wallyworld · 2024-09-27T02:02:09Z

internal/provider/azure/config.go

@@ -82,6 +82,14 @@ func (p azureEnvironProvider) ConfigDefaults() schema.Defaults {
 	return configDefaults
 }

+// ModelConfigDefaults provides a set of default model config attributes that
+// should be set on a models config if they have not been specified by the user.
+func (prov *azureEnvironProvider) ModelConfigDefaults(_ context.Context) (map[string]any, error) {


Can we add a TODO on the method declaration in the interface. At the least, we should follow up immediately with a PR to remove the unnecessary config manipulation, even if the cloud spec validation remains for now.

tlm added do not merge Even if a PR has been approved, do not merge the PR! 4.0 labels Sep 6, 2024

tlm force-pushed the model-config-model-manager branch 2 times, most recently from cd487aa to d5174c7 Compare September 24, 2024 23:33

tlm removed the do not merge Even if a PR has been approved, do not merge the PR! label Sep 24, 2024

tlm requested a review from hmlanigan September 25, 2024 00:27

tlm mentioned this pull request Sep 25, 2024

refactor: delete unused ConfigValidator method from state #18137

Merged

1 task

wallyworld reviewed Sep 25, 2024

View reviewed changes

hmlanigan reviewed Sep 25, 2024

View reviewed changes

tlm added 18 commits September 26, 2024 10:21

refactor: drives model config into mongo from dqlite

cdb70a4

refactor: add missing model config defaults

1c13090

Add missing model config defaults func to the azure provider.

feat: add missing model config defaults to dummy

617cde9

Adds missing model config defaults func to the dummy provider.

feat: add model config defaults to ec2 provider

029107e

feat: add model config defaults to equinx provider

baa7ee9

feat: add model config defaults to gce provider

3021cb0

feat: add model config defaults to lxd provider

db61fa9

feat: add model config defaults to maas provider

f4a848a

feat: adds model config defaults to manual provider

007554e

feat: add model config defaults to oci provider

da909c4

feat: adds openstack model config defaults provider

0f1c51b

feat: add vspher model config defaults provider

7cef763

feat: add kubernetes model config defaults provider

ee8e746

refactor: modelmanager onto domain service changes

420189c

refactor: broken tests for model config defaults

4205836

style: implementing missing type comment

dd4ae92

tlm added 2 commits September 26, 2024 10:23

style: apply pr feedback

f8207fa

test: fixes test in model manager facade

f21f9ce

Have removed some agent version test by commenting them out. This will need to be address in future updates when moving models fully over to dqlite. Removed validation checks for model config in state layer as they are not needed anymore.

tlm force-pushed the model-config-model-manager branch from 06d73c0 to f21f9ce Compare September 26, 2024 23:03

wallyworld approved these changes Sep 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: drives model config into mongo from dqlite. #18042

refactor: drives model config into mongo from dqlite. #18042

tlm commented Sep 6, 2024 •

edited by wallyworld

Loading

wallyworld left a comment

wallyworld Sep 25, 2024

tlm Sep 25, 2024

wallyworld Sep 27, 2024

wallyworld Sep 25, 2024

tlm Sep 25, 2024

wallyworld Sep 27, 2024

hmlanigan commented Sep 25, 2024

hmlanigan left a comment

hmlanigan Sep 25, 2024

tlm Sep 26, 2024

hmlanigan Sep 25, 2024

tlm Sep 26, 2024

hmlanigan Sep 25, 2024

tlm Sep 26, 2024

tlm commented Sep 25, 2024

tlm commented Sep 25, 2024

wallyworld commented Sep 27, 2024

wallyworld left a comment

wallyworld Sep 27, 2024

wallyworld Sep 27, 2024

wallyworld Sep 27, 2024

wallyworld Sep 27, 2024

wallyworld Sep 27, 2024

wallyworld Sep 27, 2024

wallyworld Sep 27, 2024

		// change are layered in the form of a funnel where we apply the most granular
		// specific last. The current order is:

		"github.com/juju/juju/environs/config"
		interrors "github.com/juju/juju/internal/errors"

refactor: drives model config into mongo from dqlite. #18042

Are you sure you want to change the base?

refactor: drives model config into mongo from dqlite. #18042

Conversation

tlm commented Sep 6, 2024 • edited by wallyworld Loading

Checklist

QA steps

Documentation changes

Links

wallyworld 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

Choose a reason for hiding this comment

hmlanigan commented Sep 25, 2024

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

Choose a reason for hiding this comment

tlm commented Sep 25, 2024

tlm commented Sep 25, 2024

wallyworld commented Sep 27, 2024

wallyworld 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlm commented Sep 6, 2024 •

edited by wallyworld

Loading