-
Notifications
You must be signed in to change notification settings - Fork 503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: wire up EnvironConfigGetter to use model config service #18028
Conversation
979d7cc
to
22aa7e0
Compare
/build |
60c5f8c
to
f856fc1
Compare
fa89117
to
0ab72ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a first pass. I agree that removing the precheck instance elevates a lot of issues, but we must be sure to make the epics with links back to the PR to reinstate them when we implement them.
@@ -368,18 +360,14 @@ func (s *bootstrapSuite) TestInitializeStateFailsSecondTime(c *gc.C) { | |||
AdminUser: adminUser, | |||
StateInitializationParams: args, | |||
MongoDialOpts: mongotest.DialOpts(), | |||
StateNewPolicy: state.NewPolicyFunc(nil), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprised this can go as well.
StateNewPolicy: stateenvirons.GetNewPolicyFunc( | ||
cloudGetter{cloud: &args.ControllerCloud}, | ||
credentialGetter{cred: args.ControllerCloudCredential}, | ||
// We don't need the storage service at bootstrap. | ||
func(modelUUID model.UUID, registry storage.ProviderRegistry) state.StoragePoolGetter { | ||
return noopStoragePoolGetter{} | ||
}, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to work out if we're at liberty to do this. The policy func is also required for config and constraints validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it due to a comment in agent/agentbootstrap/bootstrap.go:
// Deprecated: use InstancePrechecker
StateNewPolicy state.NewPolicyFunc
InstancePrecheckerGetter func(*state.State) (environs.InstancePrechecker, error)
9fe3ea8
to
86b486a
Compare
6877789
to
f5422c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, just a few things. Have not QAed.
@@ -200,7 +198,7 @@ type Model interface { | |||
OpenedPortRangesForMachine(string) (state.MachinePortRanges, error) | |||
// The following methods are required for querying the featureset | |||
// supported by the model. | |||
Config() (*config.Config, error) | |||
//Config() (*config.Config, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM bar a few remaining comments.
I have not QA'ed this (yet).
This allows for facade tests which double write data to share a model UUID between state and the service factory setup. Needed to start with getting ModelConfig, if the requested model state does not exist in a service factory, the environ config getters will fail during test.
4bca216
to
5b84ab0
Compare
The last force push was for fixing merge conflicts, which produced errors. |
This is a stop gap on the way to reducing dependence on Environs and replacing with appropriate calls into the different domain services where possible. That work was started in PR 18013. GetNewPolicyFunc needs a model config service getter. Callers do not have a model at the setup time of GetNewPolicyFunc. Use a model config service getter instead. Special case the model config service getter with the environStatePolicy structure to limit use and not require everywhere, using a model config service is preferable. A short term solution one the way to removing the Policy's all together. Delete stateenvirons NewInstancePrechecker and associated code. The functionality has been replace and no longer required here. Remove configvalidator_test.go. These are model config tests which are failing in state after changes to the EnvironConfigGetter. We're near the end of the work to move model config from state to a domain. Thus they can be removed now rather than fixing.
In the process of removing model config from state, there is functionality which needs to be skipped for now until it can be re-implemented in a domain. In this case skip validation model constraints for now. The model domain will be completed after model config. Skip 2 tests which fail due to above change. Remove once they have been handled in the model epic.
Over zealous with instance prechecker removal from stateenviron. These tests will work and the instance prechecker will be reimplmented in the machine domain.
Some tests are double writting a model to mongo and the domain services to allow tests to pass as some data is still in mongo and some now in a domain. We want to get model config from a domain, not mongo.
modelServiceGetter was a misleading name, use modelConfigServiceGetter instead. Comment why both modelConfigServiceGetter and ModelConfigService are available to the environStatePolicy.
5b84ab0
to
1aab7b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Send it, once all the tests go green.
This is unused and unnecessary, we only need the modelConfigServiceGetter here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to QA this, and I still believe we can remove the modelConfigService
from stateEnvironPolicy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA'd now and all looks good. Some issues with destroying the model, however I think those are unrelated and already existed in main. No need to block this PR IMO.
P.S. you might want to squash the commits on this PR before merging. |
/merge |
For Environs, ensure that Config is provided by the ModelConfigService rather than state. This is a stop gap on the way to reducing dependence on Environs and replacing with appropriate calls into the different domain services where possible.
The above changed required that NewCAASBrokerFunc and NewEnvironFunc were also updated to allow in a ModelConfigService.
In the process of this work, it became clear it should be done in conjunction with removing the NewInstancePrechecker to avoid unnecessary work when it was removed next. The InstancePrechecker will be replaced in the machine service.
TestAddApplicationUnits was removed as it was testing the InstancePrechecker which has been removed. The InstancePrechecker will be reimplemented in JUJU-6797
Checklist
[ ] 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 packagesQA steps
This change touches many pieces of juju around deploy and providers. Test by using bootstrap, deploy, model config, refresh, status and destroy in different ways. Unfortunately the bash tests do not run easily for unrelated reasons.
Model migration is tested via the GitHub Actions.
Test updating model config
Links
Jira card: JUJU-6674
Jira card: JUJU-6687