Code review comment for lp:~seyeongkim/charms/trusty/ceph/lp1411652

I think this change looks generally good to me, minus the couple of changes need in the inline comments and some unit tests.

The OSDs and Mons should have their auth removed so they couldn't access the cluster even if they wanted. If they try and connect because a daemon is started, they'll get denied and that's expected. It'd be nice if the related ceph systemd/upstart services were disabled (not removed unless the package is removed), but I don't see that as a requirement for this particular change.

In current code, what we have is the removal of a ceph unit will completely trash the unit and not clean up anything properly within ceph. This significantly improves things in that it removes almost everything related to this node from the crushmap, the monitor cluster, etc. This is goodness. (Remove the crush bucket for the host to get the last dangling stuff, though it won't have any weight so it shouldn't be *that* big of a deal in the crush algorithm, but certainly could affect retries).

As far as polling for the healthy cluster and for the placement groups to be rebalanced. I think that's a great feature, however I don't think it should be part of this particular merge proposal. The ceph service units have auth access to the cluster, but the ceph-osd units do not (and in a real-world deployment there'd be ~3 to 5 ceph units to >10 ceph-osd units). This would introduce an inconsistency with how the ceph and the ceph-osd units behave with respect to one another. I think this is a change that should be implemented outside of this merge proposal.

So bottom line, I'm marking this as 'Needs Fixing' - but I don't think we need to add the polling or service removal as part of this particular change as it is. Address the points inline below, add unit tests, and we can make an incremental improvement and provide future MPs to address the enhancements requested in the review.