Merge pull request #559 from nolar/no-warnings-on-patching-unexistent…

…-objects

Hide warnings on fake inconsistencies when patching the nonexistent objects

kopf/clients/patching.py

@@ -15,7 +15,7 @@ async def patch_obj(
         name: Optional[str] = None,
         body: Optional[bodies.Body] = None,
         context: Optional[auth.APIContext] = None,  # injected by the decorator
-) -> bodies.RawBody:
+) -> Optional[bodies.RawBody]:
     """
     Patch a resource of specific kind.
 
@@ -31,6 +31,11 @@ async def patch_obj(
     or in the status to patch; if neither had fields for patching, the result
     is an empty body. The result should only be used to check against the patch:
     if there was nothing to patch, it does not matter if the fields are absent.
+
+    Returns ``None`` if the underlying object is absent, as detected by trying
+    to patch it and failing with HTTP 404. This can happen if the object was
+    deleted in the operator's handlers or externally during the processing,
+    so that the framework was unaware of these changes until the last moment.
     """
     if context is None:
         raise RuntimeError("API instance is not injected by the decorator.")
@@ -56,8 +61,9 @@ async def patch_obj(
     # Patch & reconstruct the actual body as reported by the server. The reconstructed body can be
     # partial or empty -- if the body/status patches are empty. This is fine: it is only used
     # to verify that the patched fields are matching the patch. No patch? No mismatch!
-    patched_body = bodies.RawBody()
     try:
+        patched_body = bodies.RawBody()
+
         if body_patch:
             response = await context.session.patch(
                 url=resource.get_url(server=context.server, namespace=namespace, name=name),
@@ -77,10 +83,10 @@ async def patch_obj(
             await errors.check_response(response)
             patched_body['status'] = await response.json()
 
+        return patched_body
+
     except aiohttp.ClientResponseError as e:
         if e.status == 404:
-            pass
+            return None
         else:
             raise
-
-    return patched_body

kopf/reactor/effects.py

@@ -118,13 +118,15 @@ async def patch_and_check(
     if patch:
         logger.debug(f"Patching with: {patch!r}")
         resulting_body = await patching.patch_obj(resource=resource, patch=patch, body=body)
-        inconsistencies = diffs.diff(dict(patch), dict(resulting_body), scope=diffs.DiffScope.LEFT)
+        inconsistencies = diffs.diff(patch, resulting_body, scope=diffs.DiffScope.LEFT)
         inconsistencies = diffs.Diff(
             diffs.DiffItem(op, field, old, new)
             for op, field, old, new in inconsistencies
             if old or new or field not in KNOWN_INCONSISTENCIES
         )
-        if inconsistencies:
+        if resulting_body is None:
+            logger.debug(f"Patching was skipped: the object does not exist anymore.")
+        elif inconsistencies:
             logger.warning(f"Patching failed with inconsistencies: {inconsistencies}")
 
 

kopf/structs/diffs.py

@@ -165,9 +165,7 @@ def diff_iter(
         yield DiffItem(DiffOperation.ADD, path, a, b)
     elif b is None:
         yield DiffItem(DiffOperation.REMOVE, path, a, b)
-    elif type(a) != type(b):
-        yield DiffItem(DiffOperation.CHANGE, path, a, b)
-    elif isinstance(a, collections.abc.Mapping):
+    elif isinstance(a, collections.abc.Mapping) and isinstance(b, collections.abc.Mapping):
         a_keys = frozenset(a.keys())
         b_keys = frozenset(b.keys())
         for key in (b_keys - a_keys if DiffScope.RIGHT in scope else ()):

tests/diffs/test_calculation.py

@@ -1,6 +1,8 @@
+import collections.abc
+
 import pytest
 
-from kopf.structs.diffs import DiffScope, diff
+from kopf.structs.diffs import DiffOperation, DiffScope, diff
 
 
 @pytest.mark.parametrize('scope', list(DiffScope))
@@ -131,6 +133,29 @@ def test_dicts_with_subkeys_changed(scope):
     assert d == (('change', ('main', 'key'), 'old', 'new'),)
 
 
+def test_custom_mappings_are_recursed():
+
+    class SampleMapping(collections.abc.Mapping):
+        def __init__(self, data=(), **kwargs) -> None:
+            super().__init__()
+            self._items = dict(data, **kwargs)
+        def __len__(self) -> int: return len(self._items)
+        def __iter__(self): return iter(self._items)
+        def __getitem__(self, item: str) -> str: return self._items[item]
+
+    class MappingA(SampleMapping): pass
+    class MappingB(SampleMapping): pass
+
+    a = MappingA(a=100, b=200)
+    b = MappingB(b=300, c=400)
+    d = diff(a, b)
+    assert (DiffOperation.REMOVE, ('a',), 100, None) in d
+    assert (DiffOperation.CHANGE, ('b',), 200, 300) in d
+    assert (DiffOperation.ADD, ('c',), None, 400) in d
+    assert (DiffOperation.CHANGE, (), a, b) not in d
+
+
+# A few examples of slightly more realistic non-abstracted use-cases below:
 def test_dicts_adding_label():
     body_before_labelling = {'metadata': {}}
     body_after_labelling  = {'metadata': {'labels': 'LABEL'}}

tests/k8s/test_patching.py

@@ -269,7 +269,7 @@ async def test_ignores_absent_objects(
     body = {'metadata': {'namespace': namespace, 'name': 'name1'}}
     reconstructed = await patch_obj(resource=resource, body=body, patch=patch)
 
-    assert reconstructed == {}
+    assert reconstructed is None
 
 
 @pytest.mark.parametrize('namespace', [None, 'ns1'], ids=['without-namespace', 'with-namespace'])

tests/reactor/test_patching_inconsistencies.py

@@ -126,3 +126,30 @@ async def test_patching_with_inconsistencies(
         "Patching with:",
         "Patching failed with inconsistencies:",
     ])
+
+
+async def test_patching_with_disappearance(
+        resource, settings, caplog, assert_logs, version_api,
+        aresponses, hostname, resp_mocker):
+    caplog.set_level(logging.DEBUG)
+
+    patch = {'spec': {'x': 'y'}, 'status': {'s': 't'}}  # irrelevant
+    url = resource.get_url(namespace='ns1', name='name1')
+    patch_mock = resp_mocker(return_value=aresponses.Response(status=404))
+    aresponses.add(hostname, url, 'patch', patch_mock)
+
+    body = Body({'metadata': {'namespace': 'ns1', 'name': 'name1'}})
+    logger = LocalObjectLogger(body=body, settings=settings)
+    await patch_and_check(
+        resource=resource,
+        body=body,
+        patch=Patch(patch),
+        logger=logger,
+    )
+
+    assert_logs([
+        "Patching with:",
+        "Patching was skipped: the object does not exist anymore",
+    ], prohibited=[
+        "inconsistencies"
+    ])