Skip to content
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

CSS-5866 - Add all Juju providers to JIMM's global provider registry #1058

Merged
merged 7 commits into from
Oct 16, 2023

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Oct 13, 2023

Description

There is currently a bug in JIMM that makes it difficult to add a new cloud to a controller managed by JIMM. If one bootstraps a controller, then adds some clouds to it, you can then add that controller to JIMM and JIMM will retrieve the list of clouds available on that controller without an issue.

But, if you do the above, and then want to add a new cloud to a controller, you are supposed to do that through the jimmctl command add-cloud-to-controller which will add the cloud to the controller and then add it to JIMM's state.

Unfortunately that command fails with the following error (taken from when Field engineering tried this):

jimmctl add-cloud-to-controller main-controller maas-test --cloud=./maas-cloud.yaml
{}
ERROR error reading cloud from file: no registered provider for "maas"

The error "no registered provider" actually comes from Juju here which is called by the jimmctl client here when it validates the cloud config file. It throws the error because there is a concept of registered "providers" in Juju where providers are a computing or storage provider that can back Juju like aws, azure, gce, maas, openstack, etc. The cloud providers are registered against a globalProviders map (contained inside the environs Juju package) via the init() method of each provider. Some examples below:
https://github.com/juju/juju/blob/2.9/provider/ec2/init.go
https://github.com/juju/juju/blob/2.9/provider/azure/init.go
https://github.com/juju/juju/blob/2.9/provider/maas/init.go

In order to ensure that jimmctl also has these providers registered in its instance of the globalProviders map, we can simply import the github.com/juju/juju/provider/all package without using it, but simply for its side effects.

Fixes CSS-5866

Update: Also adds the --force flag to jimmctl add-cloud-to-controller to match juju add-cloud --force

Engineering checklist

Check only items that apply

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Test instructions

Still need to add tests but I added the following lines to func (c *addCloudToControllerCommand) Run(ctxt *cmd.Context) error

		providers := environs.RegisteredProviders()
		fmt.Printf("Providers: %v", providers)

Before the change to import all provider:
Providers: [kubernetes]{}
After:
Providers: [ec2 equinix kubernetes azure maas manual oci openstack vsphere gce lxd]{}

Notes for code reviewers

@kian99 kian99 force-pushed the fix-add-cloud-to-controller branch from 4b40d51 to 178e2c1 Compare October 13, 2023 09:59
Copy link
Collaborator

@alesstimec alesstimec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but.. could we add a unit test for this? a test that calls readCloudFromFile and see if that goes ok?

@kian99
Copy link
Contributor Author

kian99 commented Oct 13, 2023

@alesstimec Yip, that was on my mind, I'll add that.

Copy link
Contributor

@ale8k ale8k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving, but perhaps explain this in standup again for me about the whole provider concept, I sort of understand but I think in standup can clear it up better

@kian99 kian99 requested review from alesstimec and ale8k October 13, 2023 15:22
@kian99
Copy link
Contributor Author

kian99 commented Oct 13, 2023

Added a test but this required a lot more changes to ensure we pass along the force flag which wasn't used before. This is pretty important to add for clients. The reasoning:

  • Juju does a check here to ensure that the cloud you are adding is compatible with the controller's cloud. This is a matrix mapping known compatible clouds but only includes clouds "kubernetes", "lxd", "maas" and "openstack".
  • Clients could have custom cloud names and without the force flag, would never be able to add clouds to their controllers.

@kian99 kian99 force-pushed the fix-add-cloud-to-controller branch from 1f97144 to b1b6b04 Compare October 16, 2023 07:44
@@ -106,6 +107,75 @@ clouds:
c.Assert(controller.CloudRegions[1].CloudRegion.CloudName, gc.Equals, "test-hosted-cloud")
}

func (s *addCloudToControllerSuite) TestAddMaasCloudToController(c *gc.C) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels like we could have one table test for these three test cases

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, think it might make it clearer too

@@ -271,6 +271,41 @@ func (s *cloudSuite) TestAddCloud(c *gc.C) {
})
}

func (s *cloudSuite) TestAddCloudFailsWithIncompatibleClouds(c *gc.C) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.. a table test with two test cases..

@kian99 kian99 force-pushed the fix-add-cloud-to-controller branch from 7895dd0 to ec038cf Compare October 16, 2023 10:42
force bool
cloudByNameFunc func(cloudName string) (*cloud.Cloud, error)
expectedCloudName string
expectedIndex int
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this field but the previous tests were checking that the controller has a new cloud, which was always on the 2nd index since each test ran in an clean database. Now that these are in a single table test, they are a lot faster since the whole test suite doesn't need to be start/stopped every time but the database is shared between the tests. Open to suggestions on this.

- Fixed nil pointer error
- Removed unnecessary test from CLI tests
@kian99
Copy link
Contributor Author

kian99 commented Oct 16, 2023

Will add the comment about why the s.RefreshControllerAddress(c) function is needed separately. Need to follow this up with some extra checks and I don't want to the CI to run again for just 1 additional comment added.

@kian99 kian99 merged commit 2bff54a into canonical:main Oct 16, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants