Code review comment for lp:~rvb/maas/agent-bug-1239488-acquire

> 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 = {
>    ...
>    NODE_STATUS.ALLOCATED: [
>        NODE_STATUS.READY,
>        NODE_STATUS.RETIRED,
>        NODE_STATUS.MISSING,
>        ],
>
> 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.

I think this is a very good point. I'd rather tackle that in a follow-up branch though. This is of no consequence now because we don't use NODE_STATUS.RETIRED or NODE_STATUS.MISSING.

> [2]
>
> +    def test_POST_release_resets_agent_name(self):
> ...
> +    def test_POST_acquire_sets_agent_name(self):
> ...
> +    def test_POST_acquire_agent_name_defaults_to_empty_string(self):
>
> 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.

Well, I also test the changed behavior of the model's code. See the changes to test_node.py.

test_POST_release_resets_agent_name is a test I /could/ remove… but I don't think a little bit of duplication hurts if it means thorough testing of the expected behavior.