Merge lp://qastaging/~cbjchen/charm-helpers/delete-ceph-keyring into lp://qastaging/charm-helpers

Liang, it might be simpler to add a 'force' parameter to create_keyring() so that an existing keyring may be overwitten.

If we want to trash all trace of ceph when the ceph client relation is broken we could just delete /etc/ceph once ceph packages are uninstalled.

Hi Edward,

Thanks for the review!
Yeah, adding a 'force' parameter to create_keyring() will be simpler. But it might be a bit difficult to decide when to use this parameter, like one has to decide if create relation event is called more than once. The standalone delete function seems to be more naturally fit into in an event driven model - just to be called when ceph client relations is broken.
The ceph packages aren't uninstalled in the case of bug/1359220, only the relation was broken. Also, only the storage.linux.ceph module has the knowledge where exactly the keyring is stored. So glance (nova,cinder) needs this delete function to clean up the stale keyring.
The inline comments will be addressed in the next patch. I will leave the delete function there for now, and if you still have concerns, I will change the code then ;)

Thanks,
Liang

OK fair enough. So if I understand this correctly, you are specifically solving the case where ceph-relation-broken is fired, then ceph client's key changes in the ceph cluster and you want to be able to add an existing client back to the cluster afresh. I can think of two cases where this might happen:

(1) ceph cluster is redeployed (new keys created when ceph-client relation joined)

(2) we need to re-issue ceph client keys

In the case of (1) i think your approach is fine but with (2) you would need to delete the keys from ceph itself otherwise when you rejoin the cluster you will get the same keys again. I would be nice to have both of these covered. For (2) you could use the ceph broker api to issue e.g. a 'regenerate-keys' op to have new keys created and distributed.

Btw if you can resolve the inline comments I think this is lgtm. We can discuss how best to solve the cases above separately.

Hi Edward,
Yeah, you are right. We can discuss hot best to solve the second case in a separate patch.
Meanwhile the inline comments are all addressed. Please take a look. Thanks!

Sorry forgot to remove my -1. LGTM +1

approve