Fix bug where the GET /api/v2/resource/<rid> and GET /api/v2/resource endpoints return an incorrect resourcestate if a resource moved back to the available state in a new version of the configurationmodel. (PR #7609)

Fix bug where the `GET /api/v2/resource/<rid>` and `GET /api/v2/resource` endpoints return an incorrect resourcestate if a resource moved back to the available state in a new version of the configurationmodel.

- [ ] ~~Attached issue to pull request~~
- [x] Changelog entry
- [x] Type annotations are present
- [x] Code is clear and sufficiently documented
- [x] No (preventable) type errors (check using make mypy or make mypy-diff)
- [x] Sufficient test cases (reproduces the bug/tests the requested feature)
- [x] Correct, in line with design
- [ ] ~~End user documentation is included or an issue is created for end-user documentation~~
- [ ] ~~If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)~~

Author: arnaudsjs

changelogs/unreleased/fix-move-back-to-available-states.yml

@@ -0,0 +1,6 @@
+---
+description: Fix bug where the `GET /api/v2/resource/<rid>` and `GET /api/v2/resource` endpoints return an incorrect resourcestate if a resource moved back to the available state in a new version of the configurationmodel.
+change-type: patch
+destination-branches: [master, iso7]
+sections:
+  bugfix: "{{description}}"

src/inmanta/data/__init__.py

@@ -4703,34 +4703,39 @@ async def get_current_resource_state(cls, env: uuid.UUID, rid: ResourceIdStr) ->
         Return the state of the given resource in the latest version of the configuration model
         or None if the resource is not present in the latest version.
         """
-        query = f"""
+        query = """
+            WITH latest_released_version AS (
+                SELECT max(version) AS version
+                FROM configurationmodel
+                WHERE environment=$1 AND released
+            )
             SELECT (
-                SELECT r.status
-                FROM resource r
-                WHERE r.environment=$1
-                    AND r.model=(
-                        SELECT max(c.version) AS version
-                        FROM {ConfigurationModel.table_name()} c
-                        WHERE c.environment=$1 AND c.released
-                    )
-                    AND r.resource_id=$2
-            ) AS status,
-            (
-                SELECT rps.last_non_deploying_status
-                FROM resource_persistent_state rps
-                WHERE rps.environment=$1 AND rps.resource_id=$2
-            ) AS last_non_deploying_status
+                CASE
+                    -- The resource_persistent_state.last_non_deploying_status column is only populated for
+                    -- actual deployment operations to prevent locking issues. This case-statement calculates
+                    -- the correct state from the combination of the resource table and the
+                    -- resource_persistent_state table.
+                    WHEN r.status::text IN('deploying', 'undefined', 'skipped_for_undefined')
+                        -- The deploying, undefined and skipped_for_undefined states are not tracked in the
+                        -- resource_persistent_state table.
+                        THEN r.status::text
+                    WHEN rps.last_deployed_attribute_hash != r.attribute_hash
+                        -- The hash changed since the last deploy -> new desired state
+                        THEN r.status::text
+                        -- No override required, use last known state from actual deployment
+                        ELSE rps.last_non_deploying_status::text
+                END
+            ) AS status
+            FROM resource AS r
+                 INNER JOIN resource_persistent_state AS rps ON r.environment=rps.environment AND r.resource_id=rps.resource_id
+                 INNER JOIN configurationmodel AS c ON c.environment=r.environment AND c.version=r.model
+            WHERE r.environment=$1 AND r.model = (SELECT version FROM latest_released_version) AND r.resource_id=$2
             """
         results = await cls.select_query(query, [env, rid], no_obj=True)
         if not results:
             return None
         assert len(results) == 1
-        if any(results[0][column_name] is None for column_name in ["status", "last_non_deploying_status"]):
-            return None
-        if results[0]["status"] == "deploying":
-            return const.ResourceState("deploying")
-        else:
-            return const.ResourceState(results[0]["last_non_deploying_status"])
+        return const.ResourceState(results[0]["status"])
 
     @classmethod
     async def set_deployed_multi(
@@ -5054,14 +5059,26 @@ async def get_resource_details(cls, env: uuid.UUID, resource_id: m.ResourceIdStr
         def status_sub_query(resource_table_name: str) -> str:
             return f"""
             (CASE
+               -- The resource_persistent_state.last_non_deploying_status column is only populated for
+               -- actual deployment operations to prevent locking issues. This case-statement calculates
+               -- the correct state from the combination of the resource table and the
+               -- resource_persistent_state table.
                WHEN (SELECT {resource_table_name}.model < MAX(configurationmodel.version)
                        FROM configurationmodel
                        WHERE configurationmodel.released=TRUE
-                       AND environment = $1)
+                       AND environment = $1
+                 )
+                 -- Resource is no longer present in latest released configurationmodel
                  THEN 'orphaned'
-               WHEN {resource_table_name}.status::text = 'deploying'
-                 then 'deploying'
-               ELSE ps.last_non_deploying_status::text
+               WHEN {resource_table_name}.status::text IN('deploying', 'undefined', 'skipped_for_undefined')
+                 -- The deploying, undefined and skipped_for_undefined states are not tracked in the
+                 -- resource_persistent_state table.
+                 THEN {resource_table_name}.status::text
+               WHEN ps.last_deployed_attribute_hash != {resource_table_name}.attribute_hash
+                 -- The hash changed since the last deploy -> new desired state
+                 THEN {resource_table_name}.status::text
+                 -- No override required, use last known state from actual deployment
+                 ELSE ps.last_non_deploying_status::text
              END
             ) as status
             """
@@ -5156,11 +5173,13 @@ async def get_resource_deploy_summary(cls, environment: uuid.UUID) -> m.Resource
         inner_query = f"""
         SELECT r.resource_id as resource_id,
         (
-            CASE WHEN (r.status = 'deploying')
-                THEN
-                    r.status::text
-                ELSE
-                    rps.last_non_deploying_status::text
+            CASE WHEN r.status IN ('deploying', 'undefined', 'skipped_for_undefined')
+                     THEN r.status::text
+                 WHEN rps.last_deployed_attribute_hash != r.attribute_hash
+                     -- The hash changed since the last deploy -> new desired state
+                     THEN r.status::text
+                 ELSE
+                     rps.last_non_deploying_status::text
             END
         ) as status
         FROM {cls.table_name()} as r

src/inmanta/data/dataview.py

@@ -551,12 +551,21 @@ def get_base_query(self) -> SimpleQueryBuilder:
                         rps.environment,
                         (
                             CASE
+                                -- The resource_persistent_state.last_non_deploying_status column is only populated for
+                                -- actual deployment operations to prevent locking issues. This case-statement calculates
+                                -- the correct state from the combination of the resource table and the
+                                -- resource_persistent_state table.
                                 WHEN r.model < (SELECT version FROM latest_version)
                                     THEN 'orphaned'
-                                WHEN (r.status = 'deploying')
+                                WHEN r.status::text IN('deploying', 'undefined', 'skipped_for_undefined')
+                                    -- The deploying, undefined and skipped_for_undefined states are not tracked in the
+                                    -- resource_persistent_state table.
                                     THEN r.status::text
-                                ELSE
-                                    rps.last_non_deploying_status::text
+                                WHEN rps.last_deployed_attribute_hash != r.attribute_hash
+                                    -- The hash changed since the last deploy -> new desired state
+                                    THEN r.status::text
+                                    -- No override required, use last known state from actual deployment
+                                    ELSE rps.last_non_deploying_status::text
                             END
                         ) as status
                     FROM versioned_resource_state AS rps

src/inmanta/server/services/resourceservice.py

@@ -997,7 +997,11 @@ def convert_legacy_state(
                             if not is_increment_notification:
                                 if status == ResourceState.deployed:
                                     extra_fields["last_success"] = resource_action.started
-                                if status != ResourceState.deploying:
+                                if status not in {
+                                    ResourceState.deploying,
+                                    ResourceState.undefined,
+                                    ResourceState.skipped_for_undefined,
+                                }:
                                     extra_fields["last_non_deploying_status"] = const.NonDeployingResourceState(status)
 
                             await res.update_persistent_state(

tests/server/test_resource_details.py

@@ -18,16 +18,17 @@
 
 import datetime
 import itertools
+import uuid
 from collections import defaultdict
-from typing import Any
+from typing import Any, Optional
 from uuid import UUID
 
 import pytest
 from dateutil.tz import UTC
 
-from inmanta import const, data
+from inmanta import const, data, util
 from inmanta.const import ResourceState
-from inmanta.data.model import ResourceVersionIdStr
+from inmanta.data.model import ResourceIdStr, ResourceVersionIdStr
 from inmanta.util import parse_timestamp
 
 
@@ -491,3 +492,157 @@ async def test_resource_details(server, client, env_with_resources):
     assert result.result["data"]["status"] == "orphaned"
     await assert_matching_attributes(result.result["data"], resources[env.id][orphaned][0])
     assert result.result["data"]["requires_status"] == {"std::File[internal,path=/tmp/orphaned_req]": "orphaned"}
+
+
+async def test_move_to_available_state(server, environment, client, clienthelper, agent):
+    """
+    Verify that the endpoints, that return the state of a resource, return the correct state
+    when a resource moved back to the available state. This state is not written back to the
+    resource_persistent_state table and should be determined based on the content of the
+    resource table.
+    """
+    # Create model version1
+    version1 = await clienthelper.get_version()
+    result = await client.put_version(
+        tid=environment,
+        version=version1,
+        resources=[
+            {
+                "id": f"std::testing::NullResource[agent1,name=test1],v={version1}",
+                "val": "val1",
+                "requires": [],
+            },
+            {
+                "id": f"std::testing::NullResource[agent1,name=test2],v={version1}",
+                "val": "val2",
+                "requires": [],
+            },
+            {
+                "id": f"std::testing::NullResource[agent1,name=test3],v={version1}",
+                "val": "val3",
+                "requires": [],
+            },
+            {
+                "id": f"std::testing::NullResource[agent1,name=test4],v={version1}",
+                "val": "val4",
+                "requires": [],
+            },
+            {
+                "id": f"std::testing::NullResource[agent1,name=test5],v={version1}",
+                "val": "val5",
+                "requires": [f"std::testing::NullResource[agent1,name=test4],v={version1}"],
+            },
+        ],
+        resource_state={"std::testing::NullResource[agent1,name=test4]": const.ResourceState.undefined},
+        compiler_version=util.get_compiler_version(),
+    )
+    assert result.code == 200, result.result
+
+    # Release version
+    result = await client.release_version(tid=environment, id=version1)
+    assert result.code == 200
+
+    # Move resources
+    #    * std::testing::NullResource[agent1,name=test1]
+    #    * std::testing::NullResource[agent1,name=test2]
+    # to deployed state
+    aclient = agent._client
+    for i in range(1, 3):
+        action_id = uuid.uuid4()
+        result = await aclient.resource_deploy_start(
+            tid=environment,
+            rvid=f"std::testing::NullResource[agent1,name=test{i}],v={version1}",
+            action_id=action_id,
+        )
+        assert result.code == 200
+        result = await aclient.resource_deploy_done(
+            tid=environment,
+            rvid=f"std::testing::NullResource[agent1,name=test{i}],v={version1}",
+            action_id=action_id,
+            status=const.ResourceState.deployed,
+        )
+        assert result.code == 200, result.result
+
+    # Create a new version containing:
+    #    * an updated desired state for resource std::testing::NullResource[agent1,name=test1]
+    #    * A version of the std::testing::NullResource[agent1,name=test4] that is no longer undefined but available.
+    version2 = await clienthelper.get_version()
+    result = await client.put_version(
+        tid=environment,
+        version=version2,
+        resources=[
+            {
+                "id": f"std::testing::NullResource[agent1,name=test1],v={version2}",
+                "val": "val1_updated",
+                "requires": [],
+            },
+            {
+                "id": f"std::testing::NullResource[agent1,name=test2],v={version2}",
+                "val": "val2",
+                "requires": [],
+            },
+            {
+                "id": f"std::testing::NullResource[agent1,name=test3],v={version2}",
+                "val": "val3",
+                "requires": [],
+            },
+            {
+                "id": f"std::testing::NullResource[agent1,name=test4],v={version2}",
+                "val": "val4",
+                "requires": [],
+            },
+            {
+                "id": f"std::testing::NullResource[agent1,name=test5],v={version2}",
+                "val": "val5",
+                "requires": [f"std::testing::NullResource[agent1,name=test4],v={version2}"],
+            },
+        ],
+        resource_state={},
+        compiler_version=util.get_compiler_version(),
+    )
+    assert result.code == 200, result.result
+
+    async def assert_states(expected_states: dict[ResourceIdStr, const.ResourceState]) -> None:
+        # Verify behavior of resource_details() endpoint.
+        for rid, state in expected_states.items():
+            result = await client.resource_details(tid=environment, rid=rid)
+            assert result.code == 200
+            assert (
+                result.result["data"]["status"] == state.value
+            ), f"Got state {result.result['data']['status']} for resource {rid}, expected {state.value}"
+
+        # Verify behavior of get_current_resource_state() endpoint
+        resource_state: Optional[ResourceState]
+        for rid, state in expected_states.items():
+            resource_state = await data.Resource.get_current_resource_state(env=uuid.UUID(environment), rid=rid)
+            assert resource_state == state
+
+        # Verify behavior of resource_list() endpoint
+        result = await client.resource_list(tid=environment)
+        assert result.code == 200
+        actual_states = {r["resource_id"]: const.ResourceState(r["status"]) for r in result.result["data"]}
+        assert expected_states == actual_states
+
+    await assert_states(
+        {
+            ResourceIdStr("std::testing::NullResource[agent1,name=test1]"): const.ResourceState.deployed,
+            ResourceIdStr("std::testing::NullResource[agent1,name=test2]"): const.ResourceState.deployed,
+            ResourceIdStr("std::testing::NullResource[agent1,name=test3]"): const.ResourceState.available,
+            ResourceIdStr("std::testing::NullResource[agent1,name=test4]"): const.ResourceState.undefined,
+            ResourceIdStr("std::testing::NullResource[agent1,name=test5]"): const.ResourceState.skipped_for_undefined,
+        }
+    )
+
+    # Release version2
+    result = await client.release_version(tid=environment, id=version2)
+    assert result.code == 200
+
+    await assert_states(
+        {
+            ResourceIdStr("std::testing::NullResource[agent1,name=test1]"): const.ResourceState.available,
+            ResourceIdStr("std::testing::NullResource[agent1,name=test2]"): const.ResourceState.deployed,
+            ResourceIdStr("std::testing::NullResource[agent1,name=test3]"): const.ResourceState.available,
+            ResourceIdStr("std::testing::NullResource[agent1,name=test4]"): const.ResourceState.available,
+            ResourceIdStr("std::testing::NullResource[agent1,name=test5]"): const.ResourceState.available,
+        }
+    )

tests/server/test_resource_list.py

@@ -183,7 +183,17 @@ async def create_resource(
             await res.insert()
             await res.update_persistent_state(
                 last_deploy=datetime.now(tz=UTC),
-                last_non_deploying_status=status if status not in [ResourceState.available, ResourceState.deploying] else None,
+                last_non_deploying_status=(
+                    status
+                    if status
+                    not in [
+                        ResourceState.available,
+                        ResourceState.deploying,
+                        ResourceState.undefined,
+                        ResourceState.skipped_for_undefined,
+                    ]
+                    else None
+                ),
             )
 
     await create_resource("agent1", "/etc/file1", "std::File", ResourceState.available, [1, 2, 3])