Ensure storage constraints are removed when an application is deleted

[wallyworld]

Description of change

When removing an application, ensure that artefacts such as storage constraints are also removed.

QA steps

$ juju bootstrap aws
$ juju deploy postgresql --storage pgdata="ebs,10G"
$ juju remove-application postgresql

wait for postgresql app to disappear from status after it cleans itself up

$ juju deploy postgresql --storage pgdata="ebs,10G"

Bug reference

https://bugs.launchpad.net/juju/+bug/1687878

[jameinel]

lgtm

[axw]

There's no explanation as to why the existing code isn't doing what it should, and I can't see a logical reason for why stopping finalAppCharmRemoveOps from being called in some instances and making an unconditional call to it from removeOps would resolve the problem. The ops returned by finalAppCharmRemoveOps do not contain any assertions. Presumably the issue is that something is that the "isFinal" block isn't executing, which indicates the refcount is >1. Why would it be?

There's a comment that states there's a workaround. Why isn't it working?

[wallyworld]

The existing code doesn't work because a single txn slice contains 2 dec ref operations. This is what the code comment refers to. The workaround mentioned in removeOps simply didn't exist - the code comment was wrong. The code comment also mentions that finalAppCharmRemoveOps should be idempotent. It might be, but it schedules a cleanup op and we really don't want to incur the overhead of doing that unnecessarily.
The 2 dec ref operations in a singe slice simply won't work properly to ensure that the final cleanup is queued. This is because both see the original ref count value; the second dec ref op in the slice does not get to see the value after it has been decremented by the first such op.
This PR solves the problem by explicitly queuing the final cleanup ops when we know they're needed (and when it would be possible miss out running them due to getting 2 dec ref ops in the txn slice)..

[anastasiamac]

FWIIW, having been in this area and pondering about it while destroying models with empty applications, this actually a pretty nice approach to resolving the issue of trying to determine when and if to add final clean up to collection of operations to remove an application, a unit, etc..

[axw]

The existing code doesn't work because a single txn slice contains 2 dec ref operations.

But why are there 2 dec ref operations? How does the act of removing an application incur 2 "remove application" operations? That seems wrong.

EDIT: in case it's not clear, I do understand the issue of 2 dec refs in the one transaction. To address that in the long term, we should look at introducing a transaction builder which will retain information in memory about things like ref counts, whether an entity was just created, etc. The transaction builder would create the initial transaction op for a decref, and then subsequent decrefs would change the -1 to a -2, etc. What I don't understand is why this applies to removing an application. It does not seem intuitive or even logical.

[axw]

After speaking with Ian, the issue is apparently related to the cleanup and removal of units, rather than just removing the application. So LGTM (or Looks Adequate To Me at least ;)), as long as we get the unit test fixed.

[wallyworld]

$$merge$$

[jujubot]

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