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.

Sign up for GitHub

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

Jump to bottom

Trigger upgrade-series-prepare hook. #8864

Merged
merged 45 commits into from Jul 13, 2018
Merged

Trigger upgrade-series-prepare hook. #8864

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
1c0e8c7
Trigger upgrade-series-prepare hook.
ExternalReality Jun 27, 2018
0ac2132
API to check status.
ExternalReality Jun 30, 2018
02ce53c
Uniter identifies upgrade-series status changes.
ExternalReality Jul 2, 2018
be3d968
Trigger hook for upgrade series on build.
ExternalReality Jul 2, 2018
c5f67d3
Add uniter callback to update series upgade state.
ExternalReality Jul 3, 2018
678e376
Pull index to query nested array.
ExternalReality Jul 5, 2018
3d71446
thread through initial unit upgrade status.
ExternalReality Jul 6, 2018
9873531
ensure proper unit gets status set.
ExternalReality Jul 6, 2018
025d928
Resolve uniter status
ExternalReality Jul 6, 2018
095e39e
Ensure the uniter transitions prepare states.f
ExternalReality Jul 6, 2018
132b877
Remove redundant param, and more transactionsOp builder tests.
Jul 7, 2018
9e06d5e
Fix test name.
Jul 7, 2018
e7d842a
defer mutex release.
Jul 7, 2018
afd2b0e
Fix runhook tests.
Jul 8, 2018
ce7f0c9
Fix uniter tests.
ExternalReality Jul 9, 2018
418f215
Upgrade charm v6 reference.
ExternalReality Jul 9, 2018
737461e
Fix debug hooks test.
ExternalReality Jul 9, 2018
d31fefe
Fix api error check.
ExternalReality Jul 9, 2018
cd35901
Fix state transistion tests.
ExternalReality Jul 9, 2018
93b5a64
Fix known hooks test.
ExternalReality Jul 9, 2018
92d223c
Fix snapshot tests.
ExternalReality Jul 9, 2018
f7e1ab8
Remomve redundant lock call.
ExternalReality Jul 9, 2018
1ac1597
Fix abort error of set series upgrade transaction.
ExternalReality Jul 9, 2018
facc529
Do not return string api setter method.
ExternalReality Jul 9, 2018
ff4ac60
No need for status mutex in setUpgradeSeriesStatus
ExternalReality Jul 9, 2018
2eda0a1
Remove unused unit state update function.
ExternalReality Jul 9, 2018
7ca5aae
Fix typo in function name.
ExternalReality Jul 9, 2018
27b7ec7
Use more exact results struct.
ExternalReality Jul 10, 2018
cef8aed
Update test due to default change.
ExternalReality Jul 10, 2018
aa6dcff
Use more specific error resutls type.
ExternalReality Jul 10, 2018
e5c98bb
Remove error logging.
ExternalReality Jul 11, 2018
c622313
Move string contstants to core.
ExternalReality Jul 11, 2018
0d31aea
Pass around strings in the api and add validation.
ExternalReality Jul 11, 2018
71f3958
Refine assertions in update transaction.
ExternalReality Jul 11, 2018
44050d1
Test more upgrade series status assumptions.
ExternalReality Jul 11, 2018
600d130
Consume intial upgrade series notification on server side.
ExternalReality Jul 11, 2018
8cee315
Remove dangerous looking named returned value error
ExternalReality Jul 11, 2018
fb9f7cd
Documentation fix.
ExternalReality Jul 11, 2018
550ecd7
Documentation addition.
ExternalReality Jul 11, 2018
3d830be
Rename series lock to better match other state types.
ExternalReality Jul 12, 2018
b99b95a
Documentation fix.
ExternalReality Jul 12, 2018
2ab106c
Documentation fix.
ExternalReality Jul 12, 2018
6c00b4e
Change array search to switch statement.
ExternalReality Jul 12, 2018
a5c59a6
Allow different statuses.
ExternalReality Jul 13, 2018
8cc0cae
Fix redundant conversion.
ExternalReality Jul 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/uniter/unit_test.go
View file
Open in desktop
Expand Up @@ -731,7 +731,7 @@ func (s *unitSuite) TestSetUpgradeSeriesStatus(c *gc.C) {
// Check to see that the upgrade has been set appropriately
status, err := s.apiUnit.UpgradeSeriesStatus()
c.Assert(err, jc.ErrorIsNil)
c.Assert(status, gc.Equals, "preparing") // Don't Forget this
c.Assert(status, gc.Equals, newStatus)
}

func (s *unitSuite) CreateUpgradeSeriesLock(c *gc.C) {
Expand Down
79 changes: 57 additions & 22 deletions state/machine.go
View file
Open in desktop
Expand Up @@ -2205,29 +2205,62 @@ func (m *Machine) RemoveUpgradeSeriesLock() error {

// UpgradeSeriesStatus returns the status of a series upgrade.
func (m *Machine) UpgradeSeriesStatus() (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

why pass back a string? the two status types are typed.

return "preparing", nil
coll, closer := m.st.db().GetCollection(machineUpgradeSeriesLocksC)
Copy link
Member

Choose a reason for hiding this comment

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

can we move 2208-2215 into a helper method

Copy link
Member

Choose a reason for hiding this comment

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

can lines 2187-2198 go in a helper function to return the lock data? They are used by IsLocked() and will be used for future work as well.

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 would want to wait a bit to pull this out into a helper, perhaps we can wait until more of a tell-don't-ask pattern appears rather than making a helper that facilitates an "ask for lock, then do something with it" pattern.

This comment does actually point out another issue, and that is IsLocked doesn't really need an Instance of the lock. Since it isn't doing anything with it.

defer closer()

var lock upgradeSeriesLock
err := coll.FindId(m.Id()).One(&lock)
if err != nil {
return "", errors.Trace(err)
}
s := fmt.Sprintf("%d", lock.PrepareUnits[0].Status)
Copy link
Member

Choose a reason for hiding this comment

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

Suggest we rename to UpgradeSeriesPrepareStatus() and return the lock.PrepareStatus, and have a new Unit.UpgradeSeriesPrepareStatus() for the individual units?

return s, nil
}

// SetUpgradeSeriesStatus sets the status of a series upgrade.
func (m *Machine) SetUpgradeSeriesStatus(unitName string, status string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

SetUpgradeSeriesPrepareUnitStatus()?

// buildTxn := func(attempt int) ([]txn.Op, error) {
// if attempt > 0 {
// if err := m.Refresh(); err != nil {
// return nil, errors.Trace(err)
// }
// }
// locked, err := m.IsLocked()
// if err != nil {
// return nil, errors.Trace(err)
// }
// if !locked {
// return nil, errors.BadRequestf("Machine %q is not locked for upgrade", m)
// }
// if err = m.isStillAlive(); err != nil {
// return nil, errors.Trace(err)
// }
// return setUpgradeSeriesTxnOps(m.doc.Id), nil
// }
buildTxn := func(attempt int) ([]txn.Op, error) {
if attempt > 0 {
if err := m.Refresh(); err != nil {
return nil, errors.Trace(err)
}
}
locked, err := m.IsLocked()
Copy link
Member

Choose a reason for hiding this comment

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

Calling IsLocked() is here appears to be redundant given the code at 2227-2233 with an extra error.

if err != nil {
return nil, errors.Trace(err)
}
if !locked {
return nil, errors.BadRequestf("Machine %q is not locked for upgrade", m)
}
if err = m.isStillAlive(); err != nil {
return nil, errors.Trace(err)
}

coll, closer := m.st.db().GetCollection(machineUpgradeSeriesLocksC)
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should use UpgradeSeriesStatus above to short circuit the check.

unitStatus, err := m.UpgradeSeriesStatus(unitName)
if err != nil {
  return nil, errors.Trace(err)
}
if unitStatus == status {
return nil, jujutxn.ErrNoOperations
}
// pass in the current status to assert against, and the status to set it to.
return setUpgradeSeriesTxnOps(m.Id(), unitStatus, status), nil
}

defer closer()
var lock upgradeSeriesLock
err = coll.FindId(m.Id()).One(&lock)
if err != nil {
return nil, fmt.Errorf("cannot get upgrade series lock for machine %v: %v", m.Id(), err)
}
docIndex := -1
for i, unitStatus := range lock.PrepareUnits {
if unitStatus.Id == unitName {
docIndex = i
}
}
if docIndex == -1 {
return nil, fmt.Errorf("cannot get upgrade series lock for unit %q of machine %v: %v", unitName, m.Id(), err)
}
return setUpgradeSeriesTxnOps(m.doc.Id, unitName, docIndex, status), nil
}
err := m.st.db().Run(buildTxn)
if err != nil {
err = onAbort(err, ErrDead)
logger.Errorf("cannot complete series upgrade for machine %q: %v", m, err)
Copy link
Member

Choose a reason for hiding this comment

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

"cannot set unit prepare upgrade series for"

return "", err
}

return "", nil
}

Expand Down Expand Up @@ -2258,7 +2291,8 @@ func removeUpgradeSeriesLockTxnOps(machineDocId string) []txn.Op {
}
}

func setUpgradeSeriesTxnOps(machineDocId string) []txn.Op {
func setUpgradeSeriesTxnOps(machineDocId, unitName string, unitIndex int, status string) []txn.Op {
unitField := fmt.Sprintf("prepareunits.%d.status", unitIndex)
return []txn.Op{
{
C: machinesC,
Expand All @@ -2268,8 +2302,9 @@ func setUpgradeSeriesTxnOps(machineDocId string) []txn.Op {
{
C: machineUpgradeSeriesLocksC,
Copy link
Member

Choose a reason for hiding this comment

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

would we want to take the unit id and assert that
prepareunits.%d.id
is what we expect?
making a change to the "nth" entry in an array always seems a bit concerning without it. I realize the array shouldn't be changing order, but certainly I would think we want to associate the status to the Id rather than the offset.

Id: machineDocId,
Assert: txn.DocMissing,
// Update: bson.D{{"$set", bson.D{{"keep-instance", keepInstance}}}},
Assert: txn.DocExists,
Update: bson.D{
{"$set", bson.D{{unitField, status}}}},
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be better to have the unit fields keyed off the unit tag rather than an offset. This would make it easier to make the sested set. Also you should assert that the field matches the expected value.

},
}
}
Expand Down