Merge lp:~rvb/maas/agent-bug-1239488-acquire into lp:~maas-committers/maas/trunk
Proposed by
Raphaël Badin
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Raphaël Badin | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 1709 | ||||
Proposed branch: | lp:~rvb/maas/agent-bug-1239488-acquire | ||||
Merge into: | lp:~maas-committers/maas/trunk | ||||
Prerequisite: | lp:~rvb/maas/agent-bug-1239488 | ||||
Diff against target: |
137 lines (+52/-8) 5 files modified
src/maasserver/api.py (+8/-2) src/maasserver/models/node.py (+3/-1) src/maasserver/models/tests/test_node.py (+11/-5) src/maasserver/tests/test_api_node.py (+9/-0) src/maasserver/tests/test_api_nodes.py (+21/-0) |
||||
To merge this branch: | bzr merge lp:~rvb/maas/agent-bug-1239488-acquire | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Approve | ||
Review via email:
|
Commit message
Change API's acquire() and release() so that they set/reset the agent_name field.
To post a comment you must log in.
Looks good, but I have some questions. Approved, but please consider
them before landing.
[1]
Node.agent_name is only ever set in Node.acquire() and cleared in
Node.release(). Node.acquire() sets the node's status to ALLOCATED,
and Node.release() to READY. However, a node /could/ also transition
to RETIRED or MISSING from ALLOCATED:
NODE_TRANSITIONS = { STATUS. ALLOCATED: [
NODE_STATUS. READY,
NODE_STATUS. RETIRED,
NODE_STATUS. MISSING,
...
NODE_
],
I'm not sure we're doing this anywhere, but my point is that nothing
is managing agent_name - or token or owner, other attributes set and
reset by acquire() and release() - in the event that a status
transition like this occurs.
Can we instead sort out these invariants in Node.clean()? For example,
Node.agent_name would always be "" if status != ALLOCATED.
[2]
+ def test_POST_ release_ resets_ agent_name( self): acquire_ sets_agent_ name(self) : acquire_ agent_name_ defaults_ to_empty_ string( self):
...
+ def test_POST_
...
+ def test_POST_
These are fine, but they're testing model code indirectly too. There
are no unit tests for the changes to the Node model. I think we ought
to nail down the behaviour of agent_name to the model code.