Update pool handler to improve unloading & deleting pools

[rc4]

Updated the Vici wrapper to check if a pool is loaded with get_pools() before attempting to unload it. Prevents error when attempting to unload an already-unloaded pool. I also updated the wrapper to just take the name of the pool to unload instead of needing a dict.

Update the pool EditHandler to:

1. Attempt to unload the pool with vici
2. If the pool was unloaded successfully (or wasn't loaded in the first place), only then try to delete it from the DB
3. If the DB delete failed, re-load the pool into Vici
4. If the DB delete failed because of a ProtectedError (i.e. in use by a connection), print the number/name of connections using said pool.

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @rc4 on file. In order for us to review and merge your code, please contact the project maintainers via info@strongswan.org to get yourself added.

[tobiasbrunner]

@cla-bot check

Thanks.

Updated the Vici wrapper to check if a pool is loaded with get_pools() before attempting to unload it.

Not sure that's a good idea as we probably want to remove the pool from the database (it's what the user requested) even if the daemon currently doesn't have it loaded.

I also updated the wrapper to just take the name of the pool to unload instead of needing a dict.

Hm, isn't it the opposite? Or maybe the description is a bit unclear.

If the pool was unloaded successfully (or wasn't loaded in the first place), only then try to delete it from the DB

As mentioned above, the deletion from the database should probably be performed regardless of whether the daemon has the pool currently loaded or not.

Actually, I think the behavior in delete_pool() should be changed so that the pool is first deleted from the database and only if that worked, attempt to unload it from the daemon (if it's not loaded, we don't really care). Then there would be no reason to reload it via VICI in case of an error. So basically just swap two lines as follows and change the error message a bit (and possibly add your changes to enhance the error message in case of a ProtectedError):

diff --git a/strongMan/apps/pools/views/EditHandler.py b/strongMan/apps/pools/views/EditHandler.py
index 34560d16d3ab..7d27a26d6607 100644
--- a/strongMan/apps/pools/views/EditHandler.py
+++ b/strongMan/apps/pools/views/EditHandler.py
@@ -64,13 +64,13 @@ class EditHandler(object):
     def delete_pool(self, vici):
         vici_poolname = {'name': self.poolname}
         try:
-            vici.unload_pool(vici_poolname)
             self.pool.delete()
+            vici.unload_pool(vici_poolname)
             messages.add_message(self.request, messages.SUCCESS, "Pool deletion successful.")
 
         except ViciException as e:
             messages.add_message(self.request, messages.ERROR,
-                                 'Unload pool failed: ' + str(e))
+                                 'Pool deleted but unload failed: ' + str(e))
         except ProtectedError:
             messages.add_message(self.request, messages.ERROR,
                                  'Pool not deleted. Pool is in use by a connection.')

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[rc4]

Not sure that's a good idea as we probably want to remove the pool from the database (it's what the user requested) even if the daemon currently doesn't have it loaded.

This just prevents the exception when trying to unload an already-unloaded pool. It will still be deleted from the DB if this happens (as there is no exception now).

Hm, isn't it the opposite? Or maybe the description is a bit unclear.

Previously it passed vici_poolname (which was a dict {'name': self.poolname} ) to the unload_pool() wrapper - now the wrapper just takes the pool name as a string and makes the dict itself. I figured this is just simpler to have for a wrapper since the dict doesn't serve much purpose from the caller's perspective.

Actually, I think the behavior in delete_pool() should be changed so that the pool is first deleted from the database and only if that worked, attempt to unload it from the daemon (if it's not loaded, we don't really care). Then there would be no reason to reload it via VICI in case of an error.

I was under the (perhaps mistaken!) impression that the DB should reflect the state of the pools - so, if the pool is already unloaded, or was unloaded successfully - delete from the DB. If the pool can't be unloaded, then keep it in the DB (i.e. it's in-use still and needs to not be in use to be unloaded).

Then, if unloading succeeded, but the deletion from the DB failed, it should be re-loaded so we don't end up with an inconsistent state - but this perhaps okay?

Basically my thought was that there should only be 2 possible outcomes:

1. Pool is unloaded and deleted from the DB
2. Pool is loaded and present in the DB

[tobiasbrunner]

This just prevents the exception when trying to unload an already-unloaded pool. It will still be deleted from the DB if this happens (as there is no exception now).

Ah, right, that code is in the actual VICI wrapper. But that might not be the best location for it (it's mostly intended as simple wrapper around the VICI socket, and this adds overhead and might not be what a caller expects - there is only one caller I suppose, but still). I guess the check could be moved to delete_pool() (there is already a wrapper for get_pools() that takes a dict).

Previously it passed vici_poolname (which was a dict {'name': self.poolname} ) to the unload_pool() wrapper

Right, I somehow saw this as the entrypoint into the whole thing.

I was under the (perhaps mistaken!) impression that the DB should reflect the state of the pools - so, if the pool is already unloaded, or was unloaded successfully - delete from the DB. If the pool can't be unloaded, then keep it in the DB (i.e. it's in-use still and needs to not be in use to be unloaded).

I had another look and there are actually two "in-use" states that can prevent deleting/unloading a pool. In the database, pools can't be deleted if they are still referenced by a connection. However, that's something the daemon doesn't know about (there is no hard link between configs and pools, this happens dynamically at runtime). So it's possible that unloading a pool works, while connections are still referencing it (that's what I based my suggestion on above). On the other hand, and that's what I overlooked, in the daemon, a pool can't be unloaded if there are currently online leases assigned to established IKE SAs. That's in turn something strongMan isn't aware of. Connections can actually be unloaded/deleted even though there are currently active SAs in the daemon (that's not the case with the pools). So it's possible that the database constraint isn't in effect, because the connection is already deleted, but a pool still can't be unloaded because of online leases by still established SAs.

So yes, my suggestion above is not ideal in regards to the latter case as the pool can't be unloaded later if it's already gone from the database. I guess strongMan could check for active leases itself by passing e.g. {'name': self.poolname, 'leases': 'yes'} to get_pools(). However, that's obviously prone to races (a lease might just get handed out after this check and then still prevent unloading the pool). Your patch might work better, but there could be a race as well, i.e. the pool might not be loaded while an SA is getting established (that's kind of recoverable, the client just has to reconnect).

But maybe using a with transaction.atomic() block could work, where we first delete the pool in the DB (if it fails, no unload via VICI is attempted) and then, after checking whether it is loaded (to avoid that error), try to unload it (if that fails, the transaction with the delete is rolled back). Something like this:

diff --git a/strongMan/apps/pools/views/EditHandler.py b/strongMan/apps/pools/views/EditHandler.py
index 34560d16d3ab..226212acca20 100644
--- a/strongMan/apps/pools/views/EditHandler.py
+++ b/strongMan/apps/pools/views/EditHandler.py
@@ -6,6 +6,7 @@ from ..forms import AddOrEditForm
 from strongMan.apps.pools.models import Pool
 from strongMan.helper_apps.vici.wrapper.exception import ViciException
 from strongMan.helper_apps.vici.wrapper.wrapper import ViciWrapper
+from django.db import transaction
 from django.db.models import ProtectedError
 
 
@@ -64,9 +65,11 @@ class EditHandler(object):
     def delete_pool(self, vici):
         vici_poolname = {'name': self.poolname}
         try:
-            vici.unload_pool(vici_poolname)
-            self.pool.delete()
-            messages.add_message(self.request, messages.SUCCESS, "Pool deletion successful.")
+            with transaction.atomic():
+                self.pool.delete()
+                if self.poolname in vici.get_pools(vici_poolname):
+                    vici.unload_pool(vici_poolname)
+                messages.add_message(self.request, messages.SUCCESS, "Pool deletion successful.")
 
         except ViciException as e:
             messages.add_message(self.request, messages.ERROR,

[rc4]

But maybe using a with transaction.atomic() block could work

That's a good idea - I was wondering about an approach like this but I'm admittedly pretty new to django so wasn't sure how transactions worked with it. I like this approach more.

I also took the liberty of renaming the wrapper parameter from pool_name to pool to hopefully make it a little clearer that it takes a dict and not a string (since I undid the wrapper changes and moved the loaded check to the EditHandler).

How does this look?

[tobiasbrunner]

Looks good, thanks for the update. Could you please squash the commits. Maybe put the renaming of the argument in one and the changes in delete_pool in another (with some updated commit messages, in particular, start with an uppercase letter). I've also added some comments.

[tobiasbrunner]

Please put these arguments on the same line.

[tobiasbrunner]

This should still fit on a single line (we use a max line length of 109 in this project, see .flake8).

[tobiasbrunner]

This should also fit on one line, or you can leave it as it was before (i.e. the message aligned with the opening bracket).

[tobiasbrunner]

There is a stray comma at the end of this line. And to align a bit more with the rest of the code, maybe put the closing bracket on this line instead of a separate one. I'm fine with the indentation and not having arguments on the first line (seems to be what PEP 8 recommends anyway when not aligning with the opening bracket).