add verification of series supported by units to juju upgrade-series #8849

hmlanigan · 2018-06-22T19:43:06Z

Description of change

Part of Managed Series Upgrades: add verification of series supported by units to juju upgrade-series prepare

QA steps

juju bootstrap
export JUJU_DEV_FEATURE_FLAGS=upgrade-series
juju deploy ghost (chosen because it doesn't support bionic)
juju upgrade-series prepare 0 bionic
ERROR series "bionic" not supported by charm, supported series are: xenial,trusty
juju upgrade-series prepare 0 bionic --force

Documentation changes

N/A

Bug reference

N/A

hmlanigan · 2018-06-22T19:44:04Z

upgrade-series prepare validates series is legit for each application on the machine

ExternalReality

LGTM

ExternalReality · 2018-06-26T08:40:58Z

apiserver/facades/client/machinemanager/machinemanager.go

+		return errors.Trace(err)
+	}
+
+	return machine.CreateUpgradeSeriesLock()


Would it be better to lock the machine first? A defer() can be used to unlock the machine in case of error.

Is the concern that something could have been added between the time we verified the units series until the series lock is taken?
Its a fair point, but given that creating the document is also going to start other events being queued, we certainly want to do a pre check. It may be that we want to then also do a post check.

If the VerifyUnitsSeries() fails, we don't want units to run the pre hook, which is triggered by the machine lock existing. CreateUpgradeSeriesLock() should be the next step though, and defer unlock if err happens from there. Updated code, and added a todo list for after the lock is taken.

ExternalReality · 2018-06-26T09:10:50Z

apiserver/facades/client/machinemanager/machinemanager_test.go

+		},
+	})
+}
+


+1, good that these tests are added.

ExternalReality · 2018-06-26T09:17:49Z

state/machine.go

@@ -2030,7 +2030,7 @@ func (m *Machine) UpdateMachineSeries(series string, force bool) error {
 	return errors.Annotatef(err, "cannot update series for %q to %s", m, series)
 }

-func (m *Machine) verifyUnitsSeries(unitNames []string, series string, force bool) ([]*Unit, error) {
+func (m *Machine) VerifyUnitsSeries(unitNames []string, series string, force bool) ([]*Unit, error) {


Does this need to be exported? If it is only exported for test, then perhaps you can move the tests into the internal test package instead.

it's exported for use by the machinemanager in the apiserver

jameinel

A couple small things, but I think it is mostly good.

jameinel · 2018-06-26T13:35:29Z

apiserver/facades/client/machinemanager/machinemanager.go

+		return errors.Trace(err)
+	}
+
+	return machine.CreateUpgradeSeriesLock()


Is the concern that something could have been added between the time we verified the units series until the series lock is taken?
Its a fair point, but given that creating the document is also going to start other events being queued, we certainly want to do a pre check. It may be that we want to then also do a post check.

jameinel · 2018-06-26T13:58:05Z

apiserver/facades/client/machinemanager/state.go

@@ -96,6 +98,11 @@ type Unit interface {
 	UnitTag() names.UnitTag
 }

+func (m machineShim) VerifyUnitsSeries(unitNames []string, series string, force bool) error {
+	_, err := m.Machine.VerifyUnitsSeries(unitNames, series, force)


Given the underlying function has a return value, why is it that we don't need to do anything with that value? Would it be a list of things we should recurse on?

Normally it'd return a slice of unit tags, both principals and subordinates. It depend on how the next pieces are implemented, where it's useful at this point. Posed question in google doc

jameinel · 2018-06-26T13:59:40Z

cmd/juju/machine/upgradeseries_test.go

@@ -41,7 +41,7 @@ func (s *UpgradeSeriesSuite) runUpgradeSeriesCommandWithConfirmation(c *gc.C, co
 	// mock remote API
 	mockController := gomock.NewController(c)
 	mockUpgradeSeriesAPI := mocks.NewMockUpgradeMachineSeriesAPI(mockController)
-	mockUpgradeSeriesAPI.EXPECT().UpgradeSeriesPrepare(gomock.Any()).AnyTimes()
+	mockUpgradeSeriesAPI.EXPECT().UpgradeSeriesPrepare(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()


I'm surprised we don't care what parameters are being passed. I would think the machine Number and series and whether or not we are forcing is quite relevant to what we want to test.

I concur, @ExternalReality had differing thoughts, conversation in pr 8822.

@hmlanigan, @jameinel Due to the nature of these tests we don't need to spy on the UpgradeSeriesPrepareMethod method so we use the mock as a stub.

At the risk of sounding didactic I would add that many times spying & mocking are not needed when all that is really needed is a stub. Where spying and mocking are concerned with invocations and arguments; stubs only "stub out" an external dependency. So here the tests are saying "I don't care what you're doing UpgradeSeriesPerpare, I already trust that you work since you are tested somewhere else. Now get out of my way!"

Here is what Bob Martin's Little Mocker has to say about spys vs stubs:

That [spys] sounds like coupling.

It is! You have to be careful. The more you spy, the tighter you couple your tests to the implementation of your system. And that leads to fragile tests.

So in general its not a good idea to spy if all you need is a stub. Concretely, these test only make assertions about the proper handling of command line arguments and options and command line behavior that is solely based on those CLI argument and options. And as such the external dependencies can be stubbed rather than spied or mocked.

jameinel · 2018-06-26T14:04:02Z

apiserver/facades/client/machinemanager/machinemanager_test.go

+			Code:    params.CodeIncompatibleSeries,
+			Message: "series \"xenial\" not supported by charm, supported series are: yakkety,zesty",
+		},
+	})


Of the tests that I see here, the only path that doesn't seem to be covered is if VerifyUnitsSeries fails. Is that being done separately?

VerifyUnitsSeries() failure is covered by TestUpgradeSeriesPrepareIncompatibleSeries()

SimonRichardson · 2018-06-27T16:43:55Z

testing/gomockmatchers/booltype.go

+}
+
+func (o *BoolType) Matches(x interface{}) bool {
+	return o.b == x


Should this be the following.
This should make sure that value you're expecting should infact be a type of "bool"

func (o *BoolType) Matches(x interface{}) bool { if b, ok := x.(bool) { return o.b == b } return false }

or

func (o *BoolType) Matches(x interface{}) { return o.b == x.(bool) }

@SimonRichardson thx making the change - i'm going to put the test changes in a different pr, so i can get the other stuff landed.

hmlanigan · 2018-06-27T18:44:48Z

$$merge$$

hmlanigan · 2018-06-27T19:07:45Z

$$merge$$

hmlanigan force-pushed the verify branch 2 times, most recently from bf3a719 to cf62e1a Compare June 22, 2018 20:14

ExternalReality approved these changes Jun 26, 2018

View reviewed changes

jameinel approved these changes Jun 26, 2018

View reviewed changes

hmlanigan force-pushed the verify branch 2 times, most recently from a9f4f22 to b400b38 Compare June 27, 2018 16:36

SimonRichardson reviewed Jun 27, 2018

View reviewed changes

add verification of series supported by units to juju upgrade-series

da87174

hmlanigan force-pushed the verify branch from b400b38 to da87174 Compare June 27, 2018 18:38

jujubot merged commit 4cc1696 into juju:develop Jun 27, 2018

hmlanigan deleted the verify branch June 27, 2018 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add verification of series supported by units to juju upgrade-series #8849

add verification of series supported by units to juju upgrade-series #8849

hmlanigan commented Jun 22, 2018

hmlanigan commented Jun 22, 2018

ExternalReality left a comment

ExternalReality Jun 26, 2018

jameinel Jun 26, 2018

hmlanigan Jun 26, 2018

ExternalReality Jun 26, 2018

ExternalReality Jun 26, 2018

hmlanigan Jun 26, 2018

jameinel left a comment

jameinel Jun 26, 2018

jameinel Jun 26, 2018

hmlanigan Jun 26, 2018

jameinel Jun 26, 2018

hmlanigan Jun 26, 2018

ExternalReality Jun 27, 2018 •

edited

jameinel Jun 26, 2018

hmlanigan Jun 26, 2018

SimonRichardson Jun 27, 2018 •

edited

hmlanigan Jun 27, 2018

hmlanigan commented Jun 27, 2018

hmlanigan commented Jun 27, 2018

add verification of series supported by units to juju upgrade-series #8849

add verification of series supported by units to juju upgrade-series #8849

Conversation

hmlanigan commented Jun 22, 2018

Description of change

QA steps

Documentation changes

Bug reference

hmlanigan commented Jun 22, 2018

ExternalReality 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

jameinel 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

ExternalReality Jun 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonRichardson Jun 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmlanigan commented Jun 27, 2018

hmlanigan commented Jun 27, 2018

ExternalReality Jun 27, 2018 •

edited

SimonRichardson Jun 27, 2018 •

edited