-
Notifications
You must be signed in to change notification settings - Fork 45
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 application deploy and refresh internals #320
Conversation
The operating system needs to be more detailed that current to be accurate. It must take into account juju supported workload operating systems as well. This is challenging as we're using juju 2.9.45 code right now and juju 3.x limits supported workload operating systems. Thus checking the controller model agent version. Include not just the model constraints charm version selection, but also the user defined constraints for the application, e.g. arch.
Remove support for changing the operating system during an update phase. The way this was done could inadvertently install a charm with the wrong operating system on existing units during a refresh. Fix updating the charm revision and channel. This must be done with the same operating system and architecture as the original charm was deployed with. Move changing application constraints to before
This makes the code a bit faster and uses less resources. Especially do not create unused clients.
Methods with multiple clients, share a connection. Once a client calls Close(), other clients with the same connection close too, perhaps before the end of the method.
Tests are failing due to this juju bug: https://bugs.launchpad.net/juju/+bug/2039179 |
In juju 2.9.45, ResolveCharms may return a charm and sugested operating system which do not reflect or support the operating system requested by the user. Given an appropriate error to the user if so, rather than continuing to deploy a charm unsupported by the user specified operating system. Update UT to use charms and charm/operating system combinations which will not hit this bug.
208e8bc
to
ab0e464
Compare
internal/juju/applications.go
Outdated
return nil, jujuerrors.NotSupportedf("deploying bundles") | ||
} | ||
c.Tracef("heather", map[string]interface{}{"resolved origin": resolvedOrigin, "supported series": supportedSeries}) |
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.
delete 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 / some small comments. QA went well. Tried -and failed- to break it with different charms (juju-qa-test, ubuntu, coredns with channels 1.27/stable revision 48 and 1.28/stable revision 121).
Create, import and update confirmed 👍
|
||
charmsAPIClient := apicharms.NewClient(conn) | ||
defer charmsAPIClient.Close() |
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.
Why aren't we closing these anymore?
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 wrote it up in the commit message. All of the api clients share a connection, if you close one client, the connection closes as well. The code only worked because all of the clients were created at once, and the defer Close() called, even if the client wasn't used. When I moved to create and close clients when they were used, the code stopped working. Double checked the juju api code for same.
// It has already been checked against charm series. | ||
// | ||
// Note, we are re-implementing the logic of series_selector in juju code as it's | ||
// a private object. |
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 wish we could reuse the existing logic in juju's deployer (you know the charmSeries
in the series_selector), it'll be extra work to turn this into base as well.
If we'll go with this separate logic (for the front-end part of the charm.SeriesForCharm
), then I think we're gonna need some testing for this here. wdyt?
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.
Unfortunately seriesSelector is private to the deploy code, so we can't use it.
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 take the time to figure out real unit tests for the terraform code. These Acceptance tests are insufficient.
@@ -840,27 +898,18 @@ func (c applicationsClient) UpdateApplication(input *UpdateApplicationInput) err | |||
|
|||
// use the revision, channel, and series info to create the | |||
// corresponding SetCharm info | |||
|
|||
if input.Revision != nil || input.Channel != "" || input.Series != "" { | |||
setCharmConfig, err := c.computeSetCharmConfig(input, appStatus.Series, appStatus.CharmChannel, applicationAPIClient, modelconfigAPIClient, charmsAPIClient) |
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.
little note here. I haven't done the QA yet, so it might be fine, but I noticed here we're ignoring the current channel (from the appStatus
).
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.
We're only looking for a different channel as a user request. There is a question on how to automagic juju refresh in terraform. If we start to include it, then a user couldn't change anything else without upgrading the charm, which they may not intend to do. I want to talk to Thomas about this.
We also need to handle effective channel vs request channel perhaps.
Description
Visual inspection of the code before working to add base support for application resources found a number of issues to fix.
Type of change
QA steps
Please create a variety of applications plans to ensure no change in behavior, modulo the not changing the series on application resource update. Focus on constraints, charm revision/channel, charm refresh, operating system selection.
Try deploying jameinel-ubuntu-lite, this is where I noticed that the provider could not determine the correct operating system to deploy with as it failed to deploy.
The following plan should produce an error based on the juju bug found
Additional notes
JUJU-4711
https://bugs.launchpad.net/juju/+bug/2039179