Merge pull request #288 from Jc2k/finalizer_refactor

Remove Reason.ACQUIRE and Reason.RELEASE

kopf/reactor/causation.py

@@ -51,8 +51,6 @@ class Reason(str, enum.Enum):
     NOOP = 'noop'
     FREE = 'free'
     GONE = 'gone'
-    ACQUIRE = 'acquire'
-    RELEASE = 'release'
 
     def __str__(self) -> str:
         return str(self.value)
@@ -70,8 +68,6 @@ def __str__(self) -> str:
     Reason.NOOP,
     Reason.FREE,
     Reason.GONE,
-    Reason.ACQUIRE,
-    Reason.RELEASE,
 )
 ALL_REASONS = HANDLER_REASONS + REACTOR_REASONS
 
@@ -153,7 +149,6 @@ def detect_resource_changing_cause(
         *,
         event: bodies.Event,
         diff: Optional[diffs.Diff] = None,
-        requires_finalizer: bool = True,
         initial: bool = False,
         **kwargs: Any,
 ) -> ResourceChangingCause:
@@ -183,19 +178,6 @@ def detect_resource_changing_cause(
     if finalizers.is_deleted(body):
         return ResourceChangingCause(reason=Reason.DELETE, **kwargs)
 
-    # For a fresh new object, first block it from accidental deletions without our permission.
-    # The actual handler will be called on the next call.
-    # Only return this cause if the resource requires finalizers to be added.
-    if requires_finalizer and not finalizers.has_finalizers(body):
-        return ResourceChangingCause(reason=Reason.ACQUIRE, **kwargs)
-
-    # Check whether or not the resource has finalizers, but doesn't require them. If this is
-    # the case, then a resource may not be able to be deleted completely as finalizers may
-    # not be removed by the operator under normal operation. We remove the finalizers first,
-    # and any handler that should be called will be done on the next call.
-    if not requires_finalizer and finalizers.has_finalizers(body):
-        return ResourceChangingCause(reason=Reason.RELEASE, **kwargs)
-
     # For an object seen for the first time (i.e. just-created), call the creation handlers,
     # then mark the state as if it was seen when the creation has finished.
     # Creation never mixes with resuming, even if an object is detected on startup (first listing).

kopf/reactor/handling.py

@@ -208,7 +208,6 @@ async def resource_handler(
             diff=diff,
             memo=memory.user_data,
             initial=memory.noticed_by_listing and not memory.fully_handled_once,
-            requires_finalizer=registry.requires_finalizer(resource=resource, body=body),
         )
         delay = await handle_resource_changing_cause(
             lifecycle=lifecycle,
@@ -293,6 +292,19 @@ async def handle_resource_changing_cause(
     done = None
     skip = None
 
+    requires_finalizer = registry.requires_finalizer(resource=cause.resource, body=cause.body)
+    has_finalizer = finalizers.has_finalizers(body=cause.body)
+
+    if requires_finalizer and not has_finalizer:
+        logger.debug("Adding the finalizer, thus preventing the actual deletion.")
+        finalizers.append_finalizers(body=body, patch=patch)
+        return None
+
+    if not requires_finalizer and has_finalizer:
+        logger.debug("Removing the finalizer, as there are no handlers requiring it.")
+        finalizers.remove_finalizers(body=body, patch=patch)
+        return None
+
     # Regular causes invoke the handlers.
     if cause.reason in causation.HANDLER_REASONS:
         title = causation.TITLES.get(cause.reason, repr(cause.reason))
@@ -344,21 +356,6 @@ async def handle_resource_changing_cause(
     if cause.reason == causation.Reason.NOOP:
         logger.debug("Something has changed, but we are not interested (the essence is the same).")
 
-    # For the case of a newly created object, or one that doesn't have the correct
-    # finalizers, lock it to this operator. Not all newly created objects will
-    # produce an 'ACQUIRE' causation event. This only happens when there are
-    # mandatory deletion handlers registered for the given object, i.e. if finalizers
-    # are required.
-    if cause.reason == causation.Reason.ACQUIRE:
-        logger.debug("Adding the finalizer, thus preventing the actual deletion.")
-        finalizers.append_finalizers(body=body, patch=patch)
-
-    # Remove finalizers from an object, since the object currently has finalizers, but
-    # shouldn't, thus releasing the locking of the object to this operator.
-    if cause.reason == causation.Reason.RELEASE:
-        logger.debug("Removing the finalizer, as there are no handlers requiring it.")
-        finalizers.remove_finalizers(body=body, patch=patch)
-
     # The delay is then consumed by the main handling routine (in different ways).
     return delay
 

tests/causation/test_detection.py

@@ -142,7 +142,6 @@ def test_for_gone(kwargs, event, finalizers, deletion_ts, requires_finalizer):
     event['object']['metadata'].update(deletion_ts)
     cause = detect_resource_changing_cause(
         event=event,
-        requires_finalizer=requires_finalizer,
         **kwargs)
     assert cause.reason == Reason.GONE
     check_kwargs(cause, kwargs)
@@ -158,7 +157,6 @@ def test_for_free(kwargs, event, finalizers, deletion_ts, requires_finalizer):
     event['object']['metadata'].update(deletion_ts)
     cause = detect_resource_changing_cause(
         event=event,
-        requires_finalizer=requires_finalizer,
         **kwargs)
     assert cause.reason == Reason.FREE
     check_kwargs(cause, kwargs)
@@ -174,44 +172,11 @@ def test_for_delete(kwargs, event, finalizers, deletion_ts, requires_finalizer):
     event['object']['metadata'].update(deletion_ts)
     cause = detect_resource_changing_cause(
         event=event,
-        requires_finalizer=requires_finalizer,
         **kwargs)
     assert cause.reason == Reason.DELETE
     check_kwargs(cause, kwargs)
 
 
-@requires_finalizer
-@no_finalizers
-@no_deletions
-@regular_events
-def test_for_acquire(kwargs, event, finalizers, deletion_ts, requires_finalizer):
-    event = {'type': event, 'object': {'metadata': {}}}
-    event['object']['metadata'].update(finalizers)
-    event['object']['metadata'].update(deletion_ts)
-    cause = detect_resource_changing_cause(
-        event=event,
-        requires_finalizer=requires_finalizer,
-        **kwargs)
-    assert cause.reason == Reason.ACQUIRE
-    check_kwargs(cause, kwargs)
-
-
-@doesnt_require_finalizer
-@our_finalizers
-@no_deletions
-@regular_events
-def test_for_release(kwargs, event, finalizers, deletion_ts, requires_finalizer):
-    event = {'type': event, 'object': {'metadata': {}}}
-    event['object']['metadata'].update(finalizers)
-    event['object']['metadata'].update(deletion_ts)
-    cause = detect_resource_changing_cause(
-        event=event,
-        requires_finalizer=requires_finalizer,
-        **kwargs)
-    assert cause.reason == Reason.RELEASE
-    check_kwargs(cause, kwargs)
-
-
 @requires_finalizer
 @absent_lastseen
 @our_finalizers
@@ -225,7 +190,6 @@ def test_for_create(kwargs, event, finalizers, deletion_ts, annotations, content
     event['object']['metadata'].update(annotations)
     cause = detect_resource_changing_cause(
         event=event,
-        requires_finalizer=requires_finalizer,
         **kwargs)
     assert cause.reason == Reason.CREATE
     check_kwargs(cause, kwargs)
@@ -241,7 +205,6 @@ def test_for_create_skip_acquire(kwargs, event, finalizers, deletion_ts, require
     event['object']['metadata'].update(deletion_ts)
     cause = detect_resource_changing_cause(
         event=event,
-        requires_finalizer=requires_finalizer,
         **kwargs)
     assert cause.reason == Reason.CREATE
     check_kwargs(cause, kwargs)
@@ -260,7 +223,6 @@ def test_for_no_op(kwargs, event, finalizers, deletion_ts, annotations, content,
     event['object']['metadata'].update(annotations)
     cause = detect_resource_changing_cause(
         event=event,
-        requires_finalizer=requires_finalizer,
         **kwargs)
     assert cause.reason == Reason.NOOP
     check_kwargs(cause, kwargs)
@@ -279,7 +241,6 @@ def test_for_update(kwargs, event, finalizers, deletion_ts, annotations, content
     event['object']['metadata'].update(annotations)
     cause = detect_resource_changing_cause(
         event=event,
-        requires_finalizer=requires_finalizer,
         diff=True,
         **kwargs)
     assert cause.reason == Reason.UPDATE

tests/handling/conftest.py

@@ -210,6 +210,7 @@ def new_detect_fn(**kwargs):
         body.setdefault('metadata', {}).setdefault('namespace', 'some-namespace')
         body.setdefault('metadata', {}).setdefault('name', 'some-name')
         body.setdefault('metadata', {}).setdefault('uid', 'some-uid')
+        body.setdefault('metadata', {}).setdefault('finalizers', ['kopf.zalando.org/KopfFinalizerMarker'])
 
         return cause
 

tests/handling/test_cause_handling.py

@@ -13,44 +13,6 @@
 EVENT_TYPES = [None, 'ADDED', 'MODIFIED', 'DELETED']
 
 
-@pytest.mark.parametrize('event_type', EVENT_TYPES)
-async def test_acquire(registry, handlers, resource, cause_mock, event_type,
-                   caplog, assert_logs, k8s_mocked):
-    caplog.set_level(logging.DEBUG)
-
-    cause_mock.reason = Reason.ACQUIRE
-
-    event_queue = asyncio.Queue()
-    await resource_handler(
-        lifecycle=kopf.lifecycles.all_at_once,
-        registry=registry,
-        resource=resource,
-        memories=ResourceMemories(),
-        event={'type': event_type, 'object': cause_mock.body},
-        replenished=asyncio.Event(),
-        event_queue=event_queue,
-    )
-
-    assert not handlers.create_mock.called
-    assert not handlers.update_mock.called
-    assert not handlers.delete_mock.called
-
-    assert k8s_mocked.asyncio_sleep.call_count == 0
-    assert k8s_mocked.sleep_or_wait.call_count == 0
-    assert k8s_mocked.patch_obj.call_count == 1
-    assert event_queue.empty()
-
-    patch = k8s_mocked.patch_obj.call_args_list[0][1]['patch']
-    assert 'metadata' in patch
-    assert 'finalizers' in patch['metadata']
-    assert FINALIZER in patch['metadata']['finalizers']
-
-    assert_logs([
-        "Adding the finalizer",
-        "Patching with",
-    ])
-
-
 @pytest.mark.parametrize('event_type', EVENT_TYPES)
 async def test_create(registry, handlers, resource, cause_mock, event_type,
                       caplog, assert_logs, k8s_mocked):
@@ -172,54 +134,6 @@ async def test_delete(registry, handlers, resource, cause_mock, event_type,
     ])
 
 
-@pytest.mark.parametrize('event_type', EVENT_TYPES)
-async def test_release(registry, resource, handlers, cause_mock, event_type,
-                       caplog, k8s_mocked, assert_logs):
-    caplog.set_level(logging.DEBUG)
-    cause_mock.reason = Reason.RELEASE
-    cause_mock.body.setdefault('metadata', {})['finalizers'] = [FINALIZER]
-
-    # register handlers (no deletion handlers)
-    registry.register_resource_changing_handler(
-        group=resource.group,
-        version=resource.version,
-        plural=resource.plural,
-        reason=Reason.RESUME,
-        fn=lambda **_: None,
-        requires_finalizer=False,
-    )
-
-    event_queue = asyncio.Queue()
-    await resource_handler(
-        lifecycle=kopf.lifecycles.all_at_once,
-        registry=registry,
-        resource=resource,
-        memories=ResourceMemories(),
-        event={'type': event_type, 'object': cause_mock.body},
-        replenished=asyncio.Event(),
-        event_queue=event_queue,
-    )
-
-    assert not handlers.create_mock.called
-    assert not handlers.update_mock.called
-    assert not handlers.delete_mock.called
-
-    assert k8s_mocked.asyncio_sleep.call_count == 0
-    assert k8s_mocked.sleep_or_wait.call_count == 0
-    assert k8s_mocked.patch_obj.call_count == 1
-    assert event_queue.empty()
-
-    patch = k8s_mocked.patch_obj.call_args_list[0][1]['patch']
-    assert 'metadata' in patch
-    assert 'finalizers' in patch['metadata']
-    assert [] == patch['metadata']['finalizers']
-
-    assert_logs([
-        "Removing the finalizer",
-        "Patching with",
-    ])
-
-
 #
 # Informational causes: just log, and do nothing else.
 #

tests/handling/test_no_handlers.py

@@ -16,6 +16,7 @@ async def test_skipped_with_no_handlers(
         caplog, assert_logs, k8s_mocked):
     caplog.set_level(logging.DEBUG)
     cause_mock.reason = cause_type
+    cause_mock.body['metadata']['finalizers'] = []
 
     assert not registry.has_resource_changing_handlers(resource=resource)  # prerequisite
     registry.register_resource_changing_handler(