Adding update-series command by application. #7677

hmlanigan · 2017-07-28T01:14:27Z

Description of change

The update-series command will allow a user to juju of a new series for an application or a machine. This initial commit covers the application case where updating the series of an application allows for new units of the application to use the new series.

The new series must be in the supported series list of the related charm. As well, charms subordinate
to the one specified must also support the new series. A --force flag allows this behavior to be over ridden.

QA steps

Deploy two charms, one a subordinate. It's recommended to have 1 series supported by the principal charm not supported by the subordinate.
juju update-series ; where the series is supported by both the principal and subordinates. This should succeed.
juju add-unit ; the new unit should be provisioned with the series used in step 2.
juju update-series ; where the series is supported by the principal and not the subordinate. This should fail.
juju update-series --force; where the series is supported by the principal and not the subordinate. This should succeed.
juju update-series ; where the series is not supported by the principal. This should fail.
juju update-series --force; where the series is not supported by the principal. This should succeed.

At any time, the series to be used by an application can be found in the output of "juju status --format yaml"

Documentation changes

Yes, the online list of commands needs to be updated.

Bug reference

This is a planned project, however a feature request bug was filed as well:
https://bugs.launchpad.net/juju/+bug/1704135

wallyworld

Some initial comments....

wallyworld · 2017-07-28T11:13:34Z

apiserver/allfacades.go

@@ -123,6 +123,7 @@ func AllFacades() *facade.Registry {
 	reg("Application", 2, application.NewFacade)
 	reg("Application", 3, application.NewFacade)
 	reg("Application", 4, application.NewFacade)
+	reg("Application", 5, application.NewFacade) // UpdateSeries


This is not the preferred way to do it - the fact that it was done this way previously was a mistake. See StorageProvisionerAPIv3 and StorageProvisionerAPIv4 for the right way.

Will do.

There is a side question on this. Version 5 is already used in develop. What is the correct version to use here?

wallyworld · 2017-07-28T11:15:42Z

apiserver/application/application.go

+	}
+
+	if !app.IsPrincipal() {
+		return errors.New(fmt.Sprintf("%s is a subordinate application, series update is not supported.", args.ApplicationName))


wallyworld · 2017-07-28T11:16:05Z

apiserver/application/application.go

+	if err != nil {
+		return errors.Trace(err)
+	}
+	// all units will have the same subordinates, therefore we only


wallyworld · 2017-07-28T11:17:14Z

apiserver/application/application.go

+	// all units will have the same subordinates, therefore we only
+	// need to check one.
+	unit := units[0]
+	if unit != nil {


When would unit ever be nil? I don't think we need this check. For safety, we should error though if len(units) == 0

wallyworld · 2017-07-28T11:21:00Z

apiserver/application/application.go

+			}
+			_, seriesSupportedErr := charm.SeriesForCharm(args.Series, subordinateCharm.Meta().Series)
+			if seriesSupportedErr != nil && !args.Force {
+				return errors.New(fmt.Sprintf("series %s is not supported by the subordinates of the application in use. Use --force to override.", args.Series))


Errorf.
Also, the apiserver layer is not the place to put error messages. You need to define a new error type and have the caller react to that. The API is called by more than just the CLI. eg Juju Python library. There's no such thing as --force there.
See apiserver/params/apierror.go

wallyworld · 2017-07-28T11:22:42Z

apiserver/application/application_test.go

@@ -1480,6 +1480,91 @@ func (s *applicationSuite) TestApplicationUpdateInvalidApplication(c *gc.C) {
 	c.Assert(err, gc.ErrorMatches, `application "no-such-application" not found`)
 }

+func (s *applicationSuite) TestApplicationUpdateSeries(c *gc.C) {


New tests should be added to application_unit_test.go
The tests in application_test.go use JujuConnSuite which we are trying to move away from

removed changes to application_test.go and added unit tests to application_unit_tests.go

wallyworld · 2017-07-28T11:25:05Z

cmd/juju/application/updateseries.go

+}
+
+var updateSeriesDoc = `
+It is recommended to update the series on existing units before running this


I don't think we need to necessarily recommend this? There's no real harm in having existing units on trusty and new ones on xenial. It's really outside of Juju's domain and more of a charm thing.

I was thinking in terms of when updating the series of a machine is requested, not a the application data. e.g.: ..."running this command to update a machines' series.

wallyworld · 2017-07-28T11:26:02Z

cmd/juju/application/updateseries.go

+command.
+
+By updating the application series, future units of the application to use
+the new series.  The series for subordinate applications is also updated.


Let's not mention subordinates here. There's no real point as people don't use or pay attention to the series there. They don't care - they are just interested in the principal.

wallyworld · 2017-07-28T11:29:57Z

cmd/juju/application/updateseries.go

+the new series.  The series for subordinate applications is also updated.
+
+The update is disallowed unless the --force flag is used if the series is not
+explicitly supported:


"... if the requested series is not explicitly supported by the application's charm and any subordinates, as well as any other charms which may be deployed to the same machine."

You can add a comment that the user may wish to upgrade the charm to a later revision that does support their preferred series.

wallyworld · 2017-07-28T11:30:15Z

cmd/juju/application/updateseries.go

+
+Examples:
+	juju update-series <application> <series>
+	juju update-series <machine> <series>


Include a --force example

wallyworld

A good iteration on the first commit with some good progress towards how things need to be. There's a few issues with the apiserver layer to sort out. In the state layer, we need tests for the concurrency checks, and there's also logic and asserts missing.

wallyworld · 2017-08-07T12:46:11Z

api/application/client.go

@@ -219,6 +219,11 @@ func (c *Client) Update(args params.ApplicationUpdate) error {
 	return c.facade.FacadeCall("Update", args, nil)
 }

+// UpdateApplicationSeries updates the application series.
+func (c *Client) UpdateApplicationSeries(args params.ApplicationUpdateSeries) error {
+	return c.facade.FacadeCall("UpdateApplicationSeries", args, nil)


This should be a bulk call. The api layer typically takes a singular argument like here. But what is passed over the wire to the apiserver facade is a set of bulk args. There's lot of examples to cargo cult, but it would be something like:

type UpdateApplicationSeriesArg struct {
ApplicationName string
Force bool
etc
}

type UpateApplicationSeriesArgs struct {
Args []UpdateApplicationSeriesArg
}

and then

args := params.UpateApplicationSeriesArgs {
Args: []params.UpdateApplicationSeriesArg{arg}
}
var results params.ErrorResults
err := c.facade.FacadeCall("UpdateApplicationSeries", args, results)
if err != nil {
return err
}
return results.OneError()

As I said, lots of examples to copy off, but the above pseudo code should get you started.
On the server side, the pattern is to define a func to do one arg at a time, and loop over the supplied args in the args parameter, setting the results.Error for each one.

wallyworld · 2017-08-07T12:49:50Z

apiserver/application/application.go

+	if err := api.checkCanWrite(); err != nil {
+		return err
+	}
+	if !args.Force {


Even if force is true, if changes are blocked, it should still error. Force is only to bypass incompatible series, not change blocks

Rechecked the examples I was using. Removed the Force part.

wallyworld · 2017-08-07T12:51:12Z

apiserver/application/application.go

+	if err != nil {
+		return errors.Trace(err)
+	}
+	if args.Series == "" {


This should be checked first as it is cheap to check. We should not waste cpu cycles, db access etc loading the application if a simple arg check would fail

wallyworld · 2017-08-07T12:53:25Z

apiserver/application/application.go

+	}
+	if err = verifySupportedSeries(app, args.Series, args.Force); err != nil {
+		if params.IsCodeIncompatibleSeries(err) {
+			err = errors.Annotatef(err, "series %s is not supported by the application in use. Use --force to override.", args.Series)


No! No CLI related messages at the apiserver layer. These facades are called by external client libraries as well, eg juju python lib. You don't need the params.IsCodeIncompatibleSeries(err) check here at all.

As well, all errors returned from the apiserver facade methods need to be wrapped like so:
return common.ServerError(err)

wrapped appropriate errors in common.ServerError. Removed checks highlighted in comment.

wallyworld · 2017-08-07T12:55:49Z

apiserver/application/application.go

+		return err
+	}
+
+	if !app.IsPrincipal() {


This check should be further up - why go to all the trouble of checking for supported series if we won't update anything anyway because of the app being a subordinate.

wallyworld · 2017-08-07T13:04:34Z

apiserver/application/application_unit_test.go

 }

 var _ = gc.Suite(&ApplicationSuite{})

+func (s *ApplicationSuite) TearDownTest(c *gc.C) {
+	for _, a := range s.applications {


This implies that we are leaking applications between tests. s.application should start out empty or with newly cnstructed mocks.

wallyworld · 2017-08-07T13:06:53Z

state/application.go

+		Assert: bson.D{{"charmurl", chURL}},
+	}
+
+	var ops []txn.Op


The ops here should also include the ones to update any subordinates, instead of doing that in the apiserver layer.

wallyworld · 2017-08-07T13:09:01Z

state/application.go

+	if err != nil {
+		return errors.Trace(err)
+	}
+	var unitOps []txn.Op


These ops all need to be built up inside a buildTxn() function, with matching business logic to perform the same checks as what the asserts do.
(as an aside, there's no model is alive check and there needs to be).

buildTxn() function added

wallyworld · 2017-08-07T13:11:51Z

state/application_test.go

@@ -922,6 +922,16 @@ func (s *ApplicationSuite) TestUpdateConfigSettings(c *gc.C) {
 	}
 }

+func (s *ApplicationSuite) TestUpdateApplicationSeries(c *gc.C) {


We need tests that check the behaviour when things change concurrently (eg adding subordinates). This is done using the various txn hook helpers See various uses of state.SetBeforeHooks, state.SetAfterHooks, state.SetRetryHooks

starting to add these tests.

wallyworld · 2017-08-07T13:16:26Z

state/application.go

+		unitOps = append(unitOps, txn.Op{
+			C:      unitsC,
+			Id:     u.doc.DocID,
+			Assert: bson.D{{"subordinates", subNames}},


If this sufficient? Maybe it is! I thought maybe it would have been a bit more complicated, but I am probably wrong. A test to check all this is needed to be sure (using the txn hook helpers).

hmlanigan · 2017-08-10T21:02:12Z

!!build!!

hmlanigan · 2017-08-10T22:31:24Z

!!build!!

hmlanigan · 2017-08-10T22:55:14Z

!!build!!

wallyworld

Good progress, not quite done yet, some more comments

wallyworld · 2017-08-10T22:39:50Z

api/application/client.go

+	if err != nil {
+		return errors.Trace(err)
+	}
+	return errors.Trace(results.OneError())


Typically we don't need trace here. Don't hurt but not really needed.

wallyworld · 2017-08-10T22:41:47Z

apiserver/application/application.go

@@ -78,6 +94,11 @@ func NewFacade(ctx facade.Context) (*API, error) {
 	)
 }

+// NewAPIv5 creates a new server-side API v5 facade.
+func NewAPIv5(v4 *API) *APIv5 {


This isn't really needed - just use "return &APIv5{v4}" directly in the NewFacadeV5() method

wallyworld · 2017-08-10T22:45:52Z

apiserver/application/application.go

+	}
+	for i, arg := range args.Args {
+		err := api.updateOneApplicationSeries(arg)
+		results.Results[i].Error = err


Typically the pattern is not to assume the "doOne" method will return a params.Error and so use common.ServerError(err) here. If err is already a params.Error, it will just work.

wallyworld · 2017-08-10T23:07:14Z

apiserver/application/application.go

+
+		// Find our subordinate applications, to verify their series as well,
+		for _, subUnitName := range unit.SubordinateNames() {
+			subordinateApplicationName := strings.Split(subUnitName, "/")[0]


There could be a helper for this - might pay to check

wallyworld · 2017-08-10T23:09:14Z

apiserver/application/application_unit_test.go

+		charm:    &ch,
+	}
+
+	s.applications["foo"] = &app


why do we need both s.applications and s.backend.applications (and charms)

The tests were originally setup that way, however it appears that neither s.applications nor s.charms are not needed, removed. Looking at the other mock structs in the ApplicationSuite, endpoints and relation are not needed either, removed those as well.

wallyworld · 2017-08-10T23:39:07Z

state/application.go

@@ -1005,6 +1005,108 @@ func (a *Application) SetCharm(cfg SetCharmConfig) (err error) {
 	return nil
 }

+// UpdateApplicationSeries updates the series for the Application.
+func (a *Application) UpdateApplicationSeries(series string, force bool) (err error) {
+	acopy := &Application{a.st, a.doc}


not sure why we need a copy here

wallyworld · 2017-08-10T23:39:50Z

state/application.go

+
+		var ops []txn.Op
+
+		appOps, err := a.addApplicationUpdateSeriesOps(series, force)


just make this first one ops and subsequent things append to that

wallyworld · 2017-08-10T23:41:21Z

state/application.go

+			ops = append(ops, txn.Op{
+				C:      unitsC,
+				Id:     u.doc.DocID,
+				Assert: txn.DocExists,


why is this is a separate txn.Op? just include with the assert above

wallyworld · 2017-08-10T23:42:36Z

state/application.go

+	buildTxn := func(attempt int) ([]txn.Op, error) {
+		a := acopy
+		if attempt > 0 {
+			// If we've tried once already and failed, re-evaluate the criteria.


On attempt > 0, we need to check that someone hasn't already updated the series to what we want; in that case we return ErrNoOperations. And there needs to be a test for that.
And we need to check for that in business logic outside the buildTxn func

wallyworld · 2017-08-10T23:44:48Z

state/application.go

+
+func (a *Application) addApplicationUpdateSeriesOps(series string, force bool) ([]txn.Op, error) {
+	var ops []txn.Op
+	err := a.VerifySupportedSeries(series, force)


This is done outside the ops funcs. And then the ops func build up asserts to match.

wallyworld · 2017-08-11T00:15:35Z

state/application_test.go

+	app, subApp := s.setupMultiSeriesUnitWithSubordinate(c)
+	err := app.UpdateApplicationSeries("trusty", false)
+	c.Assert(err, jc.ErrorIsNil)
+	app.Refresh()


need to check error, here and elsewhere

wallyworld · 2017-08-11T00:20:20Z

state/application_test.go

+	defer state.SetTestHooks(c, s.State,
+		jujutxn.TestHook{
+			Before: func() {
+				ops := []txn.Op{{


We typically just manipulate state directly via state.State api calls. Using the txn runner works but should be a last resort. eg could just use app.SetCharm(blah)

wallyworld · 2017-08-11T00:25:55Z

state/application_test.go

+	c.Assert(subApp.Series(), gc.Equals, "xenial")
+}
+
+func (s *ApplicationSuite) TestUpdateApplicationSeriesURLChange(c *gc.C) {


This doesn't quite look right.
There should be 2 variations.

the charm changes to one where the series is not supported and so an incompatible series error occurs

the charm changes but the series is still supported so all good

I'm not sure why the after hooks are needed. They are typically used to verify that a particular txn run attempt has done to the db what is expected, ie read only checks. We don't need that here because we check via api calls that things are as expected.

After hooks removed

wallyworld · 2017-08-11T00:26:59Z

state/application_test.go

+	c.Assert(app.Series(), gc.Equals, "trusty")
+}
+
+func (s *ApplicationSuite) TestUpdateApplicationSeriesUnitCountChange(c *gc.C) {


Same comment applies here etc - as per other test

wallyworld · 2017-08-15T05:39:16Z

apiserver/application/application.go

@@ -339,6 +354,71 @@ func (api *API) Update(args params.ApplicationUpdate) error {
 	return nil
 }

+// UpdateApplicationSeries updates the application series.  Series for


We don't use double spaces after a full stop. There's at least a couple of places.

wallyworld

There's a few things to fix before landing but otherwise everything looks ok.

wallyworld · 2017-08-15T05:42:19Z

apiserver/application/application.go

+	}
+	app, err := api.backend.Application(arg.ApplicationName)
+	if err != nil {
+		return err


In most cases, we use errors.Trace(err) for standard err returns

wallyworld · 2017-08-15T05:43:48Z

apiserver/application/application.go

+		}
+	}
+	if arg.Series == app.Series() {
+		// HML do something and add test


You'd just return nil here? I think that will suffice.

wallyworld · 2017-08-15T06:00:30Z

apiserver/application/application_unit_test.go

+			Series:          "zesty",
+		}},
+	}
+	results, err := s.api.UpdateApplicationSeries(args)


I see that below we verify that UpdateApplicationSeries with the passed in series is called so that's ok. Ideally the test would use different application names rather than "foo" for both args though.

wallyworld · 2017-08-15T06:03:53Z

apiserver/application/application_unit_test.go

+	results, err := s.api.UpdateApplicationSeries(args)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(len(results.Results), gc.Equals, 1)
+	c.Assert(results.Results[0], jc.DeepEquals, params.ErrorResult{})


I meant the value in params.ErrorResult.
It's nil but we expect a fail?

wallyworld · 2017-08-15T06:09:40Z

cmd/juju/application/updateseries.go

+		}
+		c.series = args[1]
+	case 1:
+		if names.IsValidMachine(args[0]) {


if names.IsValidMachine(args[0]) || names.IsValidApplication(args[0]) {
}

?

I know that if IsValidMachine is true with only 1 arg, then the series is missing. Otherwise it's unknown if the series or application name is missing because IsValidApplication doesn't seem to know the difference between an application and a series.

wallyworld · 2017-08-15T06:33:16Z

state/application.go

+		Assert: isAliveDoc,
+		Update: bson.D{{"$set", bson.D{{"series", series}}}},
+	}}
+	ops = append(ops, txn.Op{


No need for all these appends, just to it as a slice with many elements

combined into 1 txn and removed the function.

wallyworld · 2017-08-15T06:35:16Z

state/application.go

+	ops = append(ops, txn.Op{
+		C:      applicationsC,
+		Id:     a.doc.DocID,
+		Assert: bson.D{{"unitcount", a.doc.UnitCount}},


Same with this asset - add it to the first txn.Op.
So in fact we only need the one txn.Op in the return result. That op will have 3 assert values appended together

same as above

wallyworld · 2017-08-15T06:40:17Z

state/application_test.go

+	ch := state.AddTestingCharmMultiSeries(c, s.State, "multi-series")
+	app := state.AddTestingServiceForSeries(c, s.State, "precise", "multi-series", ch)
+	err := app.UpdateApplicationSeries("precise", false)
+	c.Assert(err, gc.ErrorMatches, "no transaction operations are available")


The "no transaction operations are available" error should not get exposed - the txn runner sees it and returns nil.
If this is required here then there's something wrong elsewhere.

This test was to see an error when the series on the app matches the requested change before even trying to build the txn. However now the underlying code has been changed per other comments. The test now doesn't expect and error.

wallyworld · 2017-08-15T06:44:55Z

state/application_test.go

+				err := app.SetCharm(cfg)
+				c.Assert(err, jc.ErrorIsNil)
+			},
+			After: nil,


You can just drop the After: nil

wallyworld · 2017-08-15T06:47:08Z

state/application_test.go

+	assertApplicationSeriesUpdate(c, subApp, "trusty")
+}
+
+func (s *ApplicationSuite) TestUpdateApplicationSeriesSecondSubordinate(c *gc.C) {


I think we still need a test where an added subordinate is not compatible

hmlanigan · 2017-08-16T14:31:18Z

!!build!!

hmlanigan · 2017-08-16T14:41:38Z

!!build!!

hmlanigan · 2017-08-16T18:34:55Z

!!build!!

hmlanigan · 2017-08-16T19:25:06Z

!!build!!

hmlanigan · 2017-08-16T21:51:08Z

$$merge$$

jujubot · 2017-08-16T21:52:05Z

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

wallyworld requested changes Jul 28, 2017

View reviewed changes

hmlanigan force-pushed the update-series branch 5 times, most recently from b3e8cb7 to bc32899 Compare August 4, 2017 18:45

hmlanigan changed the title ~~WIP: Adding update-series command by application.~~ Adding update-series command by application. Aug 4, 2017

hmlanigan force-pushed the update-series branch from bc32899 to afa054d Compare August 4, 2017 19:47

wallyworld requested changes Aug 7, 2017

View reviewed changes

hmlanigan force-pushed the update-series branch 4 times, most recently from 0718e0d to 2a576fd Compare August 10, 2017 20:41

hmlanigan force-pushed the update-series branch from 2a576fd to 5a93f6e Compare August 10, 2017 22:30

wallyworld requested changes Aug 10, 2017

View reviewed changes

wallyworld reviewed Aug 11, 2017

View reviewed changes

hmlanigan force-pushed the update-series branch 3 times, most recently from 5d5898d to b9b89e9 Compare August 14, 2017 23:00

wallyworld reviewed Aug 15, 2017

View reviewed changes

wallyworld approved these changes Aug 15, 2017

View reviewed changes

hmlanigan force-pushed the update-series branch 3 times, most recently from 0f28886 to 5c0b46c Compare August 15, 2017 23:24

Adding update-series command to cmd/juju/application.

6df95bd

hmlanigan force-pushed the update-series branch from 5c0b46c to 6df95bd Compare August 16, 2017 14:24

hmlanigan changed the base branch from 2.2 to develop August 16, 2017 14:30

Merge branch 'develop' into update-series

133e300

jujubot merged commit 725c0fc into juju:develop Aug 16, 2017

hmlanigan deleted the update-series branch September 6, 2017 00:06


		var ops []txn.Op

		appOps, err := a.addApplicationUpdateSeriesOps(series, force)

Adding update-series command by application. #7677

Adding update-series command by application. #7677

Conversation

hmlanigan commented Jul 28, 2017 • edited

Description of change

QA steps

Documentation changes

Bug reference

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

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

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

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

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

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 Aug 10, 2017

hmlanigan commented Aug 10, 2017

hmlanigan commented Aug 10, 2017

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

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

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

Choose a reason for hiding this comment

hmlanigan commented Jul 28, 2017 •

edited