From cc14aee9f19e76dab0994561ff178e7d3c5a67c5 Mon Sep 17 00:00:00 2001 From: Arnaud Schoonjans Date: Wed, 15 May 2024 09:22:07 +0200 Subject: [PATCH] Fix bug where the `GET /api/v2/resource/` 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/` 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)~~ --- .../fix-move-back-to-available-states.yml | 6 + src/inmanta/data/__init__.py | 81 +++++---- src/inmanta/data/dataview.py | 15 +- .../server/services/resourceservice.py | 6 +- tests/server/test_resource_details.py | 161 +++++++++++++++++- tests/server/test_resource_list.py | 12 +- 6 files changed, 242 insertions(+), 39 deletions(-) create mode 100644 changelogs/unreleased/fix-move-back-to-available-states.yml diff --git a/changelogs/unreleased/fix-move-back-to-available-states.yml b/changelogs/unreleased/fix-move-back-to-available-states.yml new file mode 100644 index 0000000000..a85461e682 --- /dev/null +++ b/changelogs/unreleased/fix-move-back-to-available-states.yml @@ -0,0 +1,6 @@ +--- +description: Fix bug where the `GET /api/v2/resource/` 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}}" diff --git a/src/inmanta/data/__init__.py b/src/inmanta/data/__init__.py index 65f26328ad..e4ef262185 100644 --- a/src/inmanta/data/__init__.py +++ b/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 diff --git a/src/inmanta/data/dataview.py b/src/inmanta/data/dataview.py index e3c3b3b993..53ad004648 100644 --- a/src/inmanta/data/dataview.py +++ b/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 diff --git a/src/inmanta/server/services/resourceservice.py b/src/inmanta/server/services/resourceservice.py index ceb549ee7e..aeec50681f 100644 --- a/src/inmanta/server/services/resourceservice.py +++ b/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( diff --git a/tests/server/test_resource_details.py b/tests/server/test_resource_details.py index 2f80d66384..8c8d635c9f 100644 --- a/tests/server/test_resource_details.py +++ b/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, + } + ) diff --git a/tests/server/test_resource_list.py b/tests/server/test_resource_list.py index 35a5197fe8..10624407b0 100644 --- a/tests/server/test_resource_list.py +++ b/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])