Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 104 additions & 0 deletions osism/commands/reset.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
# SPDX-License-Identifier: Apache-2.0

import json
import subprocess

from cliff.command import Command
from loguru import logger
from redis.exceptions import RedisError

from osism import utils
from osism.utils.inventory import get_hosts_from_inventory, get_inventory_path


class Facts(Command):
"""Reset (clear) the cached Ansible facts.

By default the whole fact cache is flushed. Use ``--limit`` to clear
only the facts of selected hosts or groups. The command does not
gather new facts; the cache is rebuilt on the next Ansible run that
collects facts.
"""

def get_parser(self, prog_name):
parser = super(Facts, self).get_parser(prog_name)
parser.add_argument(
"-l",
"--limit",
type=str,
help="Limit the reset to selected hosts or groups (Ansible host pattern)",
)
return parser

def take_action(self, parsed_args):
if parsed_args.limit:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if parsed_args.limit: treats -l "" as "no limit" and falls through to _reset_all(), flushing the entire fact cache.

The realistic trigger is a script calling osism reset facts -l "$PATTERN" where $PATTERN expands empty — the operator means to scope the reset but wipes every host instead.

Reject an empty/whitespace-only limit explicitly before branching, e.g.:

def take_action(self, parsed_args):
    if parsed_args.limit is not None and not parsed_args.limit.strip():
        logger.error("--limit must not be empty.")
        return 1
    if parsed_args.limit:
        return self._reset_limited(parsed_args.limit)
    return self._reset_all()

Don't fix this by passing "" through to ansible-inventory instead: I verified against the pinned ansible-core==2.19.3 that ansible-inventory --limit "" returns all hosts (rc=0).

Please add a test asserting -l "" returns non-zero and never calls redis.delete/scan.

return self._reset_limited(parsed_args.limit)
return self._reset_all()

def _reset_all(self):
removed = 0
try:
cursor = 0
while True:
cursor, batch = utils.redis.scan(
cursor, match="ansible_facts*", count=100
)
if batch:
utils.redis.delete(*batch)
removed += len(batch)
if cursor == 0:
break
except RedisError as exc:
logger.error(f"Failed to reset Ansible fact cache: {exc}")
return 1

logger.info(f"Removed cached facts for {removed} host(s)")
return 0

def _reset_limited(self, limit):
try:
result = subprocess.run(
[
"ansible-inventory",
"-i",
get_inventory_path("/ansible/inventory/hosts.yml"),
"--list",
"--limit",
limit,
],
capture_output=True,
text=True,
timeout=30,
)

if result.returncode != 0:
logger.error(
f"Error loading inventory (rc={result.returncode}): "
f"{result.stderr}"
)
return 1
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
except subprocess.TimeoutExpired:
logger.error("Timeout loading inventory.")
return 1

try:
inventory = json.loads(result.stdout)
except json.JSONDecodeError as exc:
logger.error(f"Failed to parse inventory output: {exc}")
return 1

hosts = get_hosts_from_inventory(inventory)

if not hosts:
logger.warning("No hosts matched the given limit.")
return 0

keys = [f"ansible_facts{host}" for host in hosts]
try:
deleted = utils.redis.delete(*keys)
except RedisError as exc:
logger.error(f"Failed to reset Ansible fact cache: {exc}")
return 1

logger.info(f"Removed cached facts for {deleted} host(s)")
return 0
1 change: 1 addition & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ osism.commands:
report memory = osism.commands.report:Memory
reconciler = osism.commands.reconciler:Run
reconciler sync = osism.commands.reconciler:Sync
reset facts = osism.commands.reset:Facts
service = osism.commands.service:Run
set bootstrap = osism.commands.set:Bootstrap
set maintenance = osism.commands.set:Maintenance
Expand Down
196 changes: 196 additions & 0 deletions tests/unit/commands/test_reset.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
# SPDX-License-Identifier: Apache-2.0

"""Tests for the ``osism reset facts`` command.

These cover the two reset paths and their edge cases: flushing the whole
``ansible_facts*`` cache (including the empty-cache no-op), restricting the
reset to the hosts a ``--limit`` pattern resolves to, and the error contracts
for a failed inventory load and an unreachable Redis.
"""

import subprocess
from unittest.mock import MagicMock, patch

import pytest
from redis.exceptions import RedisError

from osism.commands import reset


def _make():
return reset.Facts(MagicMock(), MagicMock())


def _parse(*args):
return _make().get_parser("test").parse_args(list(args))


@pytest.fixture
def mock_redis():
"""Provide a mock Redis client wherever the command resolves it.

``osism.utils.redis`` is a lazily-initialised module attribute that opens
a real connection on first access, so patch both the attribute and its
initialiser to keep the test offline.
"""
client = MagicMock()
with patch("osism.utils._init_redis", return_value=client), patch(
"osism.commands.reset.utils.redis", client, create=True
):
yield client


# --- flush-all path ---


def test_facts_flushes_all_keys_when_no_limit(mock_redis, loguru_logs):
mock_redis.scan.return_value = (
0,
[b"ansible_factsnode1", b"ansible_factsnode2"],
)

rc = _make().take_action(_parse())

assert rc == 0
mock_redis.scan.assert_called_once_with(0, match="ansible_facts*", count=100)
mock_redis.delete.assert_called_once_with(
b"ansible_factsnode1", b"ansible_factsnode2"
)
assert any("2 host(s)" in r["message"] for r in loguru_logs)


def test_facts_succeeds_and_skips_delete_when_cache_empty(mock_redis, loguru_logs):
mock_redis.scan.return_value = (0, [])

rc = _make().take_action(_parse())

assert rc == 0
mock_redis.delete.assert_not_called()
infos = [r for r in loguru_logs if r["level"] == "INFO"]
assert any("0 host(s)" in r["message"] for r in infos)


def test_facts_returns_nonzero_on_redis_error(mock_redis, loguru_logs):
mock_redis.scan.side_effect = RedisError("connection refused")

rc = _make().take_action(_parse())

assert rc == 1
mock_redis.delete.assert_not_called()
errors = [r for r in loguru_logs if r["level"] == "ERROR"]
assert any("Failed to reset Ansible fact cache" in r["message"] for r in errors)


# --- limited path ---


def test_facts_limit_deletes_only_selected_hosts(mock_redis):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Add coverage for Redis errors in the --limit code path.

The flush-all path already tests RedisError handling (test_facts_returns_nonzero_on_redis_error), but the limited path (_reset_limited) doesn’t. Please add a --limit test that sets mock_redis.delete.side_effect = RedisError(...) and asserts that the command returns 1, stops deleting further keys, and logs the expected error, so both reset strategies have equivalent, verified error handling.

Suggested implementation:

def test_facts_limit_deletes_only_selected_hosts(mock_redis):
    mock_redis.delete.return_value = 1
    ok = MagicMock()
    ok.returncode = 0
    ok.stdout = "{}"

    with patch(
        "osism.commands.reset.get_inventory_path",
        return_value="/ansible/inventory/hosts.yml",
    ), patch("osism.commands.reset.subprocess.run", return_value=ok), patch(
        "osism.commands.reset.get_hosts_from_inventory",
        return_value=["node1", "node2"],
    ):
        # invoke the limited reset path and ensure only the selected hosts are deleted
        from osism.commands.reset import facts

        rc = facts(mock_redis, limit=["node1", "node2"])

        assert rc == 0
        # ensure we only delete the keys for the selected hosts
        mock_redis.delete.assert_has_calls(
            [
                call("ansible_facts:node1"),
                call("ansible_facts:node2"),
            ],
            any_order=False,
        )
        assert mock_redis.delete.call_count == 2


def test_facts_limit_returns_nonzero_on_redis_error_and_stops_deleting(
    mock_redis, loguru_logs
):
    # first delete raises, subsequent deletes must not be attempted
    mock_redis.delete.side_effect = RedisError("boom")
    ok = MagicMock()
    ok.returncode = 0
    ok.stdout = "{}"

    with patch(
        "osism.commands.reset.get_inventory_path",
        return_value="/ansible/inventory/hosts.yml",
    ), patch("osism.commands.reset.subprocess.run", return_value=ok), patch(
        "osism.commands.reset.get_hosts_from_inventory",
        return_value=["node1", "node2"],
    ):
        from osism.commands.reset import facts

        rc = facts(mock_redis, limit=["node1", "node2"])

    # command should indicate failure
    assert rc == 1
    # only a single delete attempt should have been made
    assert mock_redis.delete.call_count == 1

    # error should be logged in the same way as the flush-all path
    errors = [r for r in loguru_logs if r["level"] == "ERROR"]
    assert any(
        "Failed to reset Ansible fact cache" in r["message"] for r in errors
    )
  1. This change assumes call and RedisError are already imported in this test module (they likely are, given the existing flush-all tests). If not, add:
    • from redis import RedisError
    • from unittest.mock import call
  2. The invocation facts(mock_redis, limit=["node1", "node2"]) assumes the reset command exposes a facts function that accepts a Redis client and a limit argument. If the real interface differs (e.g. a Click entrypoint or a different function name/signature), adjust the call accordingly while preserving the semantics:
    • The "success" test must exercise the limited reset path and assert two deletes for the selected hosts.
    • The new error-handling test must configure mock_redis.delete.side_effect = RedisError(...), assert that the return code is 1, that only one delete is attempted, and that the same error message as the flush-all path is logged.
  3. If the actual Redis key format for per-host facts is different (e.g. ansible_facts:host:{name}), update the keys in the assert_has_calls expectations to match the real implementation so the test validates the correct keys are being deleted.

mock_redis.delete.return_value = 1
ok = MagicMock()
ok.returncode = 0
ok.stdout = "{}"

with patch(
"osism.commands.reset.get_inventory_path",
return_value="/ansible/inventory/hosts.yml",
), patch("osism.commands.reset.subprocess.run", return_value=ok), patch(
"osism.commands.reset.get_hosts_from_inventory",
return_value=["node1", "node2"],
):
rc = _make().take_action(_parse("-l", "control"))

assert rc == 0
mock_redis.scan.assert_not_called()
mock_redis.delete.assert_called_once_with(
"ansible_factsnode1", "ansible_factsnode2"
)


def test_facts_limit_returns_nonzero_when_inventory_fails(mock_redis, loguru_logs):
failed = MagicMock()
failed.returncode = 1
failed.stderr = "boom"

with patch(
"osism.commands.reset.get_inventory_path",
return_value="/ansible/inventory/hosts.yml",
), patch("osism.commands.reset.subprocess.run", return_value=failed):
rc = _make().take_action(_parse("-l", "control"))

assert rc == 1
mock_redis.delete.assert_not_called()
errors = [r for r in loguru_logs if r["level"] == "ERROR"]
assert any("Error loading inventory" in r["message"] for r in errors)
assert any("boom" in r["message"] for r in errors)


def test_facts_limit_returns_nonzero_on_invalid_inventory_json(mock_redis, loguru_logs):
ok = MagicMock()
ok.returncode = 0
ok.stdout = "{not valid json"

with patch(
"osism.commands.reset.get_inventory_path",
return_value="/ansible/inventory/hosts.yml",
), patch("osism.commands.reset.subprocess.run", return_value=ok):
rc = _make().take_action(_parse("-l", "control"))

assert rc == 1
mock_redis.delete.assert_not_called()
errors = [r for r in loguru_logs if r["level"] == "ERROR"]
assert any("Failed to parse inventory output" in r["message"] for r in errors)


def test_facts_limit_returns_nonzero_when_inventory_times_out(mock_redis, loguru_logs):
with patch(
"osism.commands.reset.get_inventory_path",
return_value="/ansible/inventory/hosts.yml",
), patch(
"osism.commands.reset.subprocess.run",
side_effect=subprocess.TimeoutExpired("ansible-inventory", 30),
):
rc = _make().take_action(_parse("-l", "control"))

assert rc == 1
mock_redis.delete.assert_not_called()
errors = [r for r in loguru_logs if r["level"] == "ERROR"]
assert any("Timeout loading inventory." in r["message"] for r in errors)


def test_facts_limit_warns_and_succeeds_when_no_hosts_match(mock_redis, loguru_logs):
ok = MagicMock()
ok.returncode = 0
ok.stdout = "{}"

with patch(
"osism.commands.reset.get_inventory_path",
return_value="/ansible/inventory/hosts.yml",
), patch("osism.commands.reset.subprocess.run", return_value=ok), patch(
"osism.commands.reset.get_hosts_from_inventory", return_value=[]
):
rc = _make().take_action(_parse("-l", "control"))

assert rc == 0
mock_redis.delete.assert_not_called()
warnings = [r for r in loguru_logs if r["level"] == "WARNING"]
assert any("No hosts matched the given limit." in r["message"] for r in warnings)


def test_facts_limit_returns_nonzero_on_redis_error(mock_redis, loguru_logs):
mock_redis.delete.side_effect = RedisError("connection refused")
ok = MagicMock()
ok.returncode = 0
ok.stdout = "{}"

with patch(
"osism.commands.reset.get_inventory_path",
return_value="/ansible/inventory/hosts.yml",
), patch("osism.commands.reset.subprocess.run", return_value=ok), patch(
"osism.commands.reset.get_hosts_from_inventory",
return_value=["node1", "node2"],
):
rc = _make().take_action(_parse("-l", "control"))

assert rc == 1
errors = [r for r in loguru_logs if r["level"] == "ERROR"]
assert any("Failed to reset Ansible fact cache" in r["message"] for r in errors)