SEBA-462 Implement UnloadModels cleanup behavior
Change-Id: If6495255fe9705140755a33c6c017a5f3b23da2d
diff --git a/VERSION b/VERSION
index f1cd7fa..fa2d855 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-2.1.49
+2.1.50
diff --git a/containers/chameleon/Dockerfile.chameleon b/containers/chameleon/Dockerfile.chameleon
index a608e77..5c3e90c 100644
--- a/containers/chameleon/Dockerfile.chameleon
+++ b/containers/chameleon/Dockerfile.chameleon
@@ -13,7 +13,7 @@
# limitations under the License.
# xosproject/chameleon
-FROM xosproject/xos-base:2.1.49
+FROM xosproject/xos-base:2.1.50
# xos-base already has protoc and dependencies installed
diff --git a/containers/xos/Dockerfile.client b/containers/xos/Dockerfile.client
index a6a229b..3b00725 100644
--- a/containers/xos/Dockerfile.client
+++ b/containers/xos/Dockerfile.client
@@ -13,7 +13,7 @@
# limitations under the License.
# xosproject/xos-client
-FROM xosproject/xos-libraries:2.1.49
+FROM xosproject/xos-libraries:2.1.50
# Install XOS client
diff --git a/containers/xos/Dockerfile.libraries b/containers/xos/Dockerfile.libraries
index eeb22a8..6ab827f 100644
--- a/containers/xos/Dockerfile.libraries
+++ b/containers/xos/Dockerfile.libraries
@@ -13,7 +13,7 @@
# limitations under the License.
# xosproject/xos-libraries
-FROM xosproject/xos-base:2.1.49
+FROM xosproject/xos-base:2.1.50
# Add libraries
COPY lib /opt/xos/lib
diff --git a/containers/xos/Dockerfile.synchronizer-base b/containers/xos/Dockerfile.synchronizer-base
index 06d9fa0..5a64c55 100644
--- a/containers/xos/Dockerfile.synchronizer-base
+++ b/containers/xos/Dockerfile.synchronizer-base
@@ -13,7 +13,7 @@
# limitations under the License.
# xosproject/xos-synchronizer-base
-FROM xosproject/xos-client:2.1.49
+FROM xosproject/xos-client:2.1.50
COPY xos/synchronizers/new_base /opt/xos/synchronizers/new_base
COPY xos/xos/logger.py /opt/xos/xos/logger.py
diff --git a/containers/xos/Dockerfile.xos-core b/containers/xos/Dockerfile.xos-core
index 4b30bdf..c9045ef 100644
--- a/containers/xos/Dockerfile.xos-core
+++ b/containers/xos/Dockerfile.xos-core
@@ -13,7 +13,7 @@
# limitations under the License.
# xosproject/xos-core
-FROM xosproject/xos-libraries:2.1.49
+FROM xosproject/xos-libraries:2.1.50
# Install XOS
ADD xos /opt/xos
diff --git a/xos/coreapi/dynamicbuild.py b/xos/coreapi/dynamicbuild.py
index cc55fac..958aec4 100644
--- a/xos/coreapi/dynamicbuild.py
+++ b/xos/coreapi/dynamicbuild.py
@@ -29,8 +29,13 @@
class DynamicBuilder(object):
- NOTHING_TO_DO = 0
- SOMETHING_CHANGED = 1
+ # These constants must agree with what is declared in dnyamicload.proto
+ SUCCESS = 0
+ SUCCESS_NOTHING_CHANGED = 1
+ ERROR = 101
+ ERROR_LIVE_MODELS = 102
+ ERROR_DELETION_IN_PROGRESS = 103
+ TRYAGAIN = 201
def __init__(self, base_dir=DEFAULT_BASE_DIR):
self.services_dir = os.path.join(base_dir, "dynamic_services")
@@ -46,7 +51,7 @@
def pre_validate_file(self, item):
# various filename validations
# alphas (upper and lower), digits, underscore, dash, and period.
- if not re.match("^[\w.-]+$", item.filename):
+ if not re.match("^[\w.-]+$", item.filename): # noqa: W605
raise Exception("illegal character in filename %s" % item.filename)
def pre_validate_python(self, item):
@@ -119,7 +124,7 @@
"Models are already up-to-date; skipping dynamic load.",
name=request.name,
)
- return self.NOTHING_TO_DO
+ return self.SUCCESS_NOTHING_CHANGED
self.pre_validate_models(request)
@@ -132,12 +137,26 @@
log.info("Finished LoadModels request", name=request.name)
- return self.SOMETHING_CHANGED
+ return self.SUCCESS
- def handle_unloadmodels_request(self, request):
+ def handle_unloadmodels_request(self, request, models):
(manifest, manifest_fn) = self.load_manifest_from_request(request)
- # TODO: Check version number to make sure this is not a downgrade ?
+ for (name, model) in models.items():
+ if model.objects.exists():
+ if request.cleanup_behavior == request.REQUIRE_CLEAN:
+ return self.ERROR_LIVE_MODELS
+ elif request.cleanup_behavior == request.AUTOMATICALLY_CLEAN:
+ # Deleting the model will add it to model.deleted_objects automatically, and it will be caught
+ # by the next loop and return a TRYAGAIN as necessary.
+ model.objects.all().delete()
+
+ for (name, model) in models.items():
+ if model.deleted_objects.exists():
+ if request.cleanup_behavior == request.REQUIRE_CLEAN:
+ return self.ERROR_DELETION_IN_PROGRESS
+ elif request.cleanup_behavior == request.AUTOMATICALLY_CLEAN:
+ return self.TRYAGAIN
hash = self.generate_request_hash(request, state="unload")
if hash == manifest.get("hash"):
@@ -147,7 +166,7 @@
"Models are already up-to-date; skipping dynamic unload.",
name=request.name,
)
- return self.NOTHING_TO_DO
+ return self.SUCCESS_NOTHING_CHANGED
manifest = self.save_models(request, state="unload", hash=hash)
@@ -158,7 +177,7 @@
log.info("Finished UnloadModels request", name=request.name)
- return self.SOMETHING_CHANGED
+ return self.SUCCESS
def generate_request_hash(self, request, state):
# TODO: could we hash the request rather than individually hashing the subcomponents of the request?
diff --git a/xos/coreapi/protos/dynamicload.proto b/xos/coreapi/protos/dynamicload.proto
index 38b55fc..192ee42 100644
--- a/xos/coreapi/protos/dynamicload.proto
+++ b/xos/coreapi/protos/dynamicload.proto
@@ -47,14 +47,25 @@
message LoadModelsReply {
enum LoadModelsStatus {
SUCCESS = 0;
- ERROR = 1;
+ SUCCESS_NOTHING_CHANGED = 1; // Succeeded by not doing anything
+ ERROR = 101; // Unspecified Error
+ ERROR_LIVE_MODELS = 102; // Live models are present during unload request
+ ERROR_DELETION_IN_PROGRESS = 103; // Soft-deleted models are present, waiting for delete steps
+ TRYAGAIN = 201; // Caller should try again
}
LoadModelsStatus status = 1;
};
message UnloadModelsRequest {
+ enum CleanupBehavior {
+ REQUIRE_CLEAN = 0; // Return an error if live data is present (DEFAULT)
+ AUTOMATICALLY_CLEAN = 1; // Automatically delete live data, return an in-progress response as necessary
+ PURGE = 2; // Forcibly purge database tables without running delete steps
+ };
+
string name = 1;
string version = 2;
+ CleanupBehavior cleanup_behavior = 3;
};
message ServiceModelStatus {
diff --git a/xos/coreapi/test_dynamicbuild.py b/xos/coreapi/test_dynamicbuild.py
index 6982747..3f464d1 100644
--- a/xos/coreapi/test_dynamicbuild.py
+++ b/xos/coreapi/test_dynamicbuild.py
@@ -17,7 +17,7 @@
import shutil
import tempfile
import unittest
-from mock import patch
+from mock import patch, MagicMock, Mock
from xosconfig import Config
@@ -39,7 +39,12 @@
class DynamicUnloadRequest:
+ REQUIRE_CLEAN = 0
+ AUTOMATICALLY_CLEAN = 1
+ PURGE = 2
+
def __init__(self, **kwargs):
+ self.cleanup_behavior = self.REQUIRE_CLEAN
for (k, v) in kwargs.items():
setattr(self, k, v)
@@ -216,7 +221,7 @@
run_xosgenx_service.assert_called()
remove_service.assert_not_called()
- self.assertEqual(result, self.builder.SOMETHING_CHANGED)
+ self.assertEqual(result, self.builder.SUCCESS)
self.assertTrue(os.path.exists(self.builder.manifest_dir))
self.assertTrue(
@@ -268,7 +273,7 @@
run_xosgenx_service.assert_called()
remove_service.assert_not_called()
- self.assertEqual(result, self.builder.SOMETHING_CHANGED)
+ self.assertEqual(result, self.builder.SUCCESS)
self.assertTrue(os.path.exists(self.builder.manifest_dir))
self.assertTrue(
@@ -295,7 +300,9 @@
)
self.assertEqual(manifest.get("state"), "load")
- self.assertEqual(manifest.get("migrations"), [{u'filename': u'migration1.py'}, {u'filename': u'migration2.py'}])
+ self.assertEqual(
+ manifest.get("migrations"),
+ [{u'filename': u'migration1.py'}, {u'filename': u'migration2.py'}])
def test_handle_unloadmodels_request(self):
with patch.object(
@@ -310,14 +317,15 @@
wraps=self.builder.remove_service,
) as remove_service:
result = self.builder.handle_unloadmodels_request(
- self.example_unload_request
+ self.example_unload_request,
+ {}
)
save_models.assert_called()
run_xosgenx_service.assert_not_called()
remove_service.assert_called()
- self.assertEqual(result, self.builder.SOMETHING_CHANGED)
+ self.assertEqual(result, self.builder.SUCCESS)
self.assertTrue(os.path.exists(self.builder.manifest_dir))
self.assertTrue(
@@ -333,12 +341,95 @@
)
self.assertEqual(manifest.get("state"), "unload")
+ def test_handle_unloadmodels_request_live_models(self):
+ with patch.object(
+ dynamicbuild.DynamicBuilder, "save_models", wraps=self.builder.save_models
+ ) as save_models, patch.object(
+ dynamicbuild.DynamicBuilder,
+ "remove_service",
+ wraps=self.builder.remove_service,
+ ) as remove_service:
+ someobject = Mock()
+ somemodel = MagicMock()
+ somemodel.objects.all.return_value = [someobject]
+ somemodel.objects.exists.return_value = True
+
+ # Test with cleanup_behavior = REQUIRE_CLEAN
+
+ result = self.builder.handle_unloadmodels_request(
+ self.example_unload_request,
+ {"somemodel": somemodel, }
+ )
+
+ save_models.assert_not_called()
+ remove_service.assert_not_called()
+
+ self.assertEqual(result, self.builder.ERROR_LIVE_MODELS)
+
+ # Test with cleanup_behavior = AUTOMATICALLY_CLEAN
+
+ self.example_unload_request.cleanup_behavior = self.example_unload_request.AUTOMATICALLY_CLEAN
+ somemodel.objects.exists.return_value = False
+ somemodel.deleted_objects.all.return_value = [someobject]
+
+ result = self.builder.handle_unloadmodels_request(
+ self.example_unload_request,
+ {"somemodel": somemodel, }
+ )
+
+ save_models.assert_not_called()
+ remove_service.assert_not_called()
+
+ self.assertEqual(result, self.builder.TRYAGAIN)
+
+ # Test with cleanup_behavior = PURGE
+
+ self.example_unload_request.cleanup_behavior = self.example_unload_request.PURGE
+ somemodel.objects.exists.return_value = False
+ somemodel.deleted_objects.all.return_value = [someobject]
+
+ result = self.builder.handle_unloadmodels_request(
+ self.example_unload_request,
+ {"somemodel": somemodel, }
+ )
+
+ save_models.assert_called()
+ remove_service.assert_called()
+
+ self.assertEqual(result, self.builder.SUCCESS)
+
+ def test_handle_unloadmodels_request_softdeleted_models(self):
+ with patch.object(
+ dynamicbuild.DynamicBuilder, "save_models", wraps=self.builder.save_models
+ ) as save_models, patch.object(
+ dynamicbuild.DynamicBuilder,
+ "remove_service",
+ wraps=self.builder.remove_service,
+ ) as remove_service:
+ someobject = Mock()
+ somemodel = MagicMock()
+ somemodel.objects.all.return_value = []
+ somemodel.objects.exists.return_value = False
+ somemodel.deleted_objects.all.return_value = [someobject]
+
+ # Test with cleanup_behavior = REQUIRE_CLEAN
+
+ result = self.builder.handle_unloadmodels_request(
+ self.example_unload_request,
+ {"somemodel": somemodel, }
+ )
+
+ save_models.assert_not_called()
+ remove_service.assert_not_called()
+
+ self.assertEqual(result, self.builder.ERROR_DELETION_IN_PROGRESS)
+
def test_handle_loadmodels_request_twice(self):
result = self.builder.handle_loadmodels_request(self.example_request)
- self.assertEqual(result, self.builder.SOMETHING_CHANGED)
+ self.assertEqual(result, self.builder.SUCCESS)
result = self.builder.handle_loadmodels_request(self.example_request)
- self.assertEqual(result, self.builder.NOTHING_TO_DO)
+ self.assertEqual(result, self.builder.SUCCESS_NOTHING_CHANGED)
def test_save_models(self):
manifest = self.builder.save_models(self.example_request, state="load")
diff --git a/xos/coreapi/xos_dynamicload_api.py b/xos/coreapi/xos_dynamicload_api.py
index b02af06..e153ff0 100644
--- a/xos/coreapi/xos_dynamicload_api.py
+++ b/xos/coreapi/xos_dynamicload_api.py
@@ -26,6 +26,7 @@
self.thread_pool = thread_pool
self.server = server
self.django_apps = None
+ self.django_apps_by_name = {}
def stop(self):
pass
@@ -37,17 +38,33 @@
"""
self.django_apps = django_apps
+ # Build up some dictionaries used by the API handlers. We can build these once at initialization time because
+ # when apps are onboarded, the core is always restarted.
+
+ self.django_apps_by_name = {}
+ self.django_app_models = {}
+ if self.django_apps:
+ for app in self.django_apps.get_app_configs():
+ self.django_apps_by_name[app.name] = app
+
+ # Build up a dictionary of all non-decl models.
+ django_models = {}
+ for (k, v) in app.models.items():
+ if not k.endswith("_decl"):
+ django_models[k] = v
+ self.django_app_models[app.name] = django_models
+
@track_request_time("DynamicLoad", "LoadModels")
def LoadModels(self, request, context):
try:
builder = DynamicBuilder()
result = builder.handle_loadmodels_request(request)
- if result == builder.SOMETHING_CHANGED:
+ if result == builder.SUCCESS:
self.server.delayed_shutdown(5)
response = dynamicload_pb2.LoadModelsReply()
- response.status = response.SUCCESS
+ response.status = result
REQUEST_COUNT.labels(
"xos-core", "DynamicLoad", "LoadModels", grpc.StatusCode.OK
).inc()
@@ -61,17 +78,54 @@
).inc()
raise e
+ def map_error_code(self, status, context):
+ """ Map the DynamicLoad status into an appropriate gRPC status code
+ and include an error description if appropriate.
+
+ Chameleon supports limited mapping to http status codes.
+ Picked a best-fit:
+ OK = 200
+ INVALID_ARGUMENT = 400
+ ALREADY_EXISTS = 409
+ """
+
+ code_names = {
+ DynamicBuilder.SUCCESS: "SUCCESS",
+ DynamicBuilder.SUCCESS_NOTHING_CHANGED: "SUCCESS_NOTHING_CHANGED",
+ DynamicBuilder.ERROR: "ERROR",
+ DynamicBuilder.ERROR_LIVE_MODELS: "ERROR_LIVE_MODELS",
+ DynamicBuilder.ERROR_DELETION_IN_PROGRESS: "ERROR_DELETION_IN_PROGRESS",
+ DynamicBuilder.TRYAGAIN: "TRYAGAIN"}
+
+ code_map = {
+ DynamicBuilder.SUCCESS: grpc.StatusCode.OK,
+ DynamicBuilder.SUCCESS_NOTHING_CHANGED: grpc.StatusCode.OK,
+ DynamicBuilder.ERROR: grpc.StatusCode.INVALID_ARGUMENT,
+ DynamicBuilder.ERROR_LIVE_MODELS: grpc.StatusCode.ALREADY_EXISTS,
+ DynamicBuilder.ERROR_DELETION_IN_PROGRESS: grpc.StatusCode.ALREADY_EXISTS,
+ DynamicBuilder.TRYAGAIN: grpc.StatusCode.OK}
+
+ code = code_map.get(status, DynamicBuilder.ERROR)
+
+ context.set_code(code)
+ if code != grpc.StatusCode.OK:
+ # In case of error, send helpful text back to the caller
+ context.set_details(code_names.get(status, "UNKNOWN"))
+
@track_request_time("DynamicLoad", "UnloadModels")
def UnloadModels(self, request, context):
try:
builder = DynamicBuilder()
- result = builder.handle_unloadmodels_request(request)
+ result = builder.handle_unloadmodels_request(request,
+ self.django_app_models.get("services." + request.name, []))
- if result == builder.SOMETHING_CHANGED:
+ if result == builder.SUCCESS:
self.server.delayed_shutdown(5)
+ self.map_error_code(result, context)
+
response = dynamicload_pb2.LoadModelsReply()
- response.status = response.SUCCESS
+ response.status = result
REQUEST_COUNT.labels(
"xos-core", "DynamicLoad", "UnloadModels", grpc.StatusCode.OK
).inc()
@@ -87,11 +141,6 @@
@track_request_time("DynamicLoad", "GetLoadStatus")
def GetLoadStatus(self, request, context):
- django_apps_by_name = {}
- if self.django_apps:
- for app in self.django_apps.get_app_configs():
- django_apps_by_name[app.name] = app
-
try:
builder = DynamicBuilder()
manifests = builder.get_manifests()
@@ -106,7 +155,7 @@
item.state = manifest.get("state", "unspecified")
if item.state == "load":
- django_app = django_apps_by_name.get("services." + item.name)
+ django_app = self.django_apps_by_name.get("services." + item.name)
if django_app:
item.state = "present"
# TODO: Might be useful to return a list of models as well
@@ -115,7 +164,7 @@
item = response.services.add()
item.name = "core"
item.version = autodiscover_version_of_main()
- if "core" in django_apps_by_name:
+ if "core" in self.django_apps_by_name:
item.state = "present"
else:
item.state = "load"