/juju

If a change is no-op, then don't create a transaction. #7180

Merged
merged 8 commits into from Apr 3, 2017

Conversation

Reviewers

@axw axw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jameinel @jujubot @axw
@jameinel
Owner

jameinel commented Mar 30, 2017 •
Edited 1 time

Description of change

Change SetLinkLayerDevices and SetDevicesAddresses so that both of them check if there are actually any changes to be done, and only if we'll actually change something do we create a transaction.

QA steps

  $ juju bootstrap lxd --debug
  $ juju add-machine
  $ juju ssh 0
  $$ sudo service jujud-machine-0 restart

  $ juju ssh -m controller 0
  $ dialmgo
  $ db.txns.find({"o.c": {"$in": ["linklayerdevices", "ip.addresses"]}}).sort({"_id": -1}).limit(5).pretty()

  $ juju debug-log -m controller --include machine-0 -include-module juju.state

monitor the transaction log, it should not be filling up with transactions that are ultimately just asserting that a bunch of devices still exist. The debug-log should also include lines like:

  DEBUG juju.state no changes to LinkLayerDevices for machine "0"
  DEBUG juju.state no changes to DevicesAddresses for machine "0"

  db.txns.find({"o.c": {$exists: 0}}).pretty()

should return no documents. Before the last commit we were creating transactions that affected no collections, which was a bit silly.

Documentation changes

None.

Bug reference

lp:1677619
lp:1676427

jameinel added some commits Mar 30, 2017

@jameinel
If a change is no-op, then don't create a transaction. 
Change SetLinkLayerDevices and SetDevicesAddresses so that both of them check if there are actually any changes to be done, and only if we'll actually change something do we create a transaction.
c01e9ae
@jameinel
Move the check for model active to always checking, fix a test that a… 
…ssumed we could set things in multiple arguments.

It wasn't a valid check, because it happened to work because it always set the values,
because it wasn't properly seeing if there was a change.
But if we do it sequentially we find the right values.
6f10d59

@jameinel jameinel changed the title from WIP: If a change is no-op, then don't create a transaction. to If a change is no-op, then don't create a transaction. Mar 30, 2017

@jameinel
Owner

jameinel commented Mar 30, 2017

This currently does create no-op transactions that look like:

{
        "_id" : ObjectId("58dd1fe3ab41ca0c3219d73d"),
        "s" : 6,
        "o" : [ ],
        "n" : "89fc8647",
        "r" : [ ]
}

would it be better to raise an exception that there is nothing to do so that these don't get created?
@jameinel
Owner

jameinel commented Mar 30, 2017

This has been fixed in the last version.
@axw

axw reviewed Mar 31, 2017
View changes

Looks fine, but I have some concern about dropping the model life assertion. I don't think it needs to be there in the first place, in which case we should remove both parts. But if we do care and must check in memory, we should be mirroring that in an assert.

state/machine_linklayerdevices.go
+ if err := checkModelActive(m.st); err != nil {
+ return nil, errors.Trace(err)
+ }
+ if err := m.isStillAlive(); err != nil {
@axw

axw Mar 31, 2017

Member

it would be a little more efficient to use the m.doc.Life value here, and only reload the machine doc on attempt>0. I suspect that's what was intended originally, except the initial check was left out

@jameinel

jameinel Apr 3, 2017

Owner

Done.

state/machine_linklayerdevices.go
@@ -206,18 +206,27 @@ func (m *Machine) SetLinkLayerDevices(devicesArgs ...LinkLayerDeviceArgs) (err e
}
}
+ // We've checked the model is alive directly, and we assert the machine is alive, we don't need to also
+ // assert the model is alive, because then the machine would be dying as well.
@axw

axw Mar 31, 2017

Member

This statement is not true. A model can be dying, but still have alive machines. They will eventually be destroyed on account of cleanups.

If we don't really care about the model life here, should we remove the one above too? The doc comment for the method does not explain why we check the model life. Do you know?

@jameinel

jameinel Apr 3, 2017

Owner

I think it was back in the "I'm updating something, make sure my model is alive". These aren't actually creating new resources, so I don't think it is critical.

I'm personally ok with a "don't start work that is about to be dropped" check of model, but certainly I don't believe we need the TXN assert.

@axw

axw Apr 3, 2017

Member

I think you're right, and we should only care about checking model life when creating new resources.

IMO, the in-memory life check should be dropped too. It's of dubious value when there's no accompanying txn op, since it's no longer ensuring integrity - so it's really just an optimisation. This makes things more difficult to understand, because it's not clear why we would do it in only memory from the context (hence this question).

FWIW, I've taken to the pattern of:

// checkSomething checks something in memory, and then return txn.Ops that assert the same
func checkSomething() ([]txn.Op, error)

That way it's clear that the ops will be ignored or not, more likely with comments to justify.

state/machine_linklayerdevices.go
+ if len(setDevicesOps) == 0 {
+ // No need to assert only that the machine is alive
+ logger.Debugf("no changes to LinkLayerDevices for machine %q", m.Id())
+ return nil, errNoChanges
@axw

axw Mar 31, 2017

Member

return nil, jujutxn.ErrNoOperations

then you can drop the check below

@jameinel

jameinel Apr 3, 2017

Owner

thanks, I expected something like that, but there wasn't anything like that around this code.

@jameinel
review feedback. 
Check directly if the machine is still alive, instead of hitting the db immediately.
We also don't need our own 'noChanges' as jujutxn.ErrNoOperations already exists.
d7bba08
@axw

axw approved these changes Apr 3, 2017
View changes

state/machine_linklayerdevices.go
@@ -206,18 +206,27 @@ func (m *Machine) SetLinkLayerDevices(devicesArgs ...LinkLayerDeviceArgs) (err e
}
}
+ // We've checked the model is alive directly, and we assert the machine is alive, we don't need to also
+ // assert the model is alive, because then the machine would be dying as well.
@axw

axw Mar 31, 2017

Member

This statement is not true. A model can be dying, but still have alive machines. They will eventually be destroyed on account of cleanups.

If we don't really care about the model life here, should we remove the one above too? The doc comment for the method does not explain why we check the model life. Do you know?

@jameinel

jameinel Apr 3, 2017

Owner

I think it was back in the "I'm updating something, make sure my model is alive". These aren't actually creating new resources, so I don't think it is critical.

I'm personally ok with a "don't start work that is about to be dropped" check of model, but certainly I don't believe we need the TXN assert.

@axw

axw Apr 3, 2017

Member

I think you're right, and we should only care about checking model life when creating new resources.

IMO, the in-memory life check should be dropped too. It's of dubious value when there's no accompanying txn op, since it's no longer ensuring integrity - so it's really just an optimisation. This makes things more difficult to understand, because it's not clear why we would do it in only memory from the context (hence this question).

FWIW, I've taken to the pattern of:

// checkSomething checks something in memory, and then return txn.Ops that assert the same
func checkSomething() ([]txn.Op, error)

That way it's clear that the ops will be ignored or not, more likely with comments to justify.

state/machine_linklayerdevices.go
@@ -600,13 +615,13 @@ func (m *Machine) SetDevicesAddresses(devicesAddresses ...LinkLayerDeviceAddress
return nil, errors.Trace(err)
}
+ if err := checkModelActive(m.st); err != nil {
@axw

axw Apr 3, 2017

Member

as above

@jameinel
Owner

jameinel commented Apr 3, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Apr 3, 2017

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

jujubot commented Apr 3, 2017

Build failed: Generating tarball failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10611
@jameinel
Owner

jameinel commented Apr 3, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Apr 3, 2017

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

@jujubot jujubot merged commit ab4ee25 into juju:develop Apr 3, 2017

@jameinel jameinel deleted the jameinel:2.1-set-devices-1677619 branch Apr 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment