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

Fix application deploy and refresh internals #320

Merged
merged 6 commits into from
Oct 18, 2023

Conversation

hmlanigan
Copy link
Member

@hmlanigan hmlanigan commented Oct 6, 2023

Description

Visual inspection of the code before working to add base support for application resources found a number of issues to fix.

  • When refreshing, update application constraints before adding new units, otherwise the new units will not use the new constraints.
  • During juju deploy, check the application constraints for architecture, fallback to using the model constraint value if not provided.
  • Improve operating system selection for deploying a charm if one is not provided. Including validating against juju supported workload operating systems if a juju 3.x controller.
  • Updating an applications's charm revision or channel requires revalidation of the charm with new data against the operating system and architecture deployed with. As well as ensuring they exist.
  • When refreshing an application, use the current application charm origin to start. Do not deduce a new one. This could lead to the wrong charm in updates as a charm name can change, the origin holds an ID which is immutable and will guarantee the same charm is used after deploy.
  • Do not change a charm series after deployed. That could cause incompatible charm blobs to be used.
  • Do not create juju clients if they will not be used.
  • Close a juju api connection, not each individual client created using the connection. Once a client is closed, the shared connection is also closed, potentially preventing use by other clients.

Type of change

  • Logic changes in resources (the API interaction with Juju has been changed)

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

terraform {
  required_providers {
    juju = {
      version = ">= 0.9.0"
      source  = "juju/juju"
    }
  }
}

provider "juju" {
}

resource "juju_model" "testmodel" {
  name = "focal-test"
}

resource "juju_application" "ubuntu" {
  model = juju_model.testmodel.name
  charm {
    name = "postgresql"
    series = "focal"
  }
}

Additional notes

JUJU-4711
https://bugs.launchpad.net/juju/+bug/2039179

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.
@hmlanigan hmlanigan requested a review from cderici October 6, 2023 20:45
@hmlanigan
Copy link
Member Author

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.
@hmlanigan hmlanigan force-pushed the fix-application-refresh branch from 208e8bc to ab0e464 Compare October 16, 2023 20:26
return nil, jujuerrors.NotSupportedf("deploying bundles")
}
c.Tracef("heather", map[string]interface{}{"resolved origin": resolvedOrigin, "supported series": supportedSeries})
Copy link
Member Author

Choose a reason for hiding this comment

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

delete me

Copy link

@cderici cderici left a 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()
Copy link

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?

Copy link
Member Author

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.
Copy link

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

internal/juju/applications.go Outdated Show resolved Hide resolved
@@ -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)
Copy link

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).

Copy link
Member Author

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.

@hmlanigan hmlanigan merged commit 098a83a into juju:main Oct 18, 2023
@hmlanigan hmlanigan deleted the fix-application-refresh branch October 18, 2023 15:42
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.

2 participants