From fb29816b680acd138012b2d9a84678cf6025dc35 Mon Sep 17 00:00:00 2001 From: Nikita Unisikhin Date: Fri, 1 Nov 2024 13:24:26 +0500 Subject: [PATCH] Refactoring --- ch_tools/chadmin/cli/object_storage_group.py | 26 +++---- ch_tools/monrun_checks/ch_orphaned_objects.py | 74 +++++++++++-------- tests/features/monrun.feature | 11 +++ tests/steps/common.py | 14 +++- 4 files changed, 75 insertions(+), 50 deletions(-) diff --git a/ch_tools/chadmin/cli/object_storage_group.py b/ch_tools/chadmin/cli/object_storage_group.py index f74e8f65..f7a35dfc 100644 --- a/ch_tools/chadmin/cli/object_storage_group.py +++ b/ch_tools/chadmin/cli/object_storage_group.py @@ -177,28 +177,15 @@ def _store_state_zk_save( create_zk_nodes(ctx, [path], make_parents=True) state_data = json.dumps( - { - ORPHANED_OBJECTS_SIZE_FIELD: total_size, - ORPHANED_OBJECTS_ERROR_MSG_FIELD: error_msg, - }, - indent=4, + create_orphaned_objects_state(total_size, error_msg), indent=4 ) update_zk_nodes(ctx, [path], state_data.encode("utf-8")) -def _store_state_local_save( - _: Context, total_size: int, error_msg: Optional[str] -) -> None: +def _store_state_local_save(_: Context, total_size: int, error_msg: str) -> None: with open(STATE_LOCAL_PATH, "w", encoding="utf-8") as file: - json.dump( - { - ORPHANED_OBJECTS_SIZE_FIELD: total_size, - ORPHANED_OBJECTS_ERROR_MSG_FIELD: error_msg, - }, - file, - indent=4, - ) + json.dump(create_orphaned_objects_state(total_size, error_msg), file, indent=4) def _print_response(ctx: Context, dry_run: bool, deleted: int, total_size: int) -> None: @@ -224,3 +211,10 @@ def _table_formatter(stats): print_response( ctx, clean_stats, default_format="table", table_formatter=_table_formatter ) + + +def create_orphaned_objects_state(total_size: int, error_msg: str) -> dict: + return { + ORPHANED_OBJECTS_SIZE_FIELD: total_size, + ORPHANED_OBJECTS_ERROR_MSG_FIELD: error_msg, + } diff --git a/ch_tools/monrun_checks/ch_orphaned_objects.py b/ch_tools/monrun_checks/ch_orphaned_objects.py index 6a224578..5e990b60 100644 --- a/ch_tools/monrun_checks/ch_orphaned_objects.py +++ b/ch_tools/monrun_checks/ch_orphaned_objects.py @@ -1,4 +1,5 @@ import json +from typing import Tuple import click @@ -6,6 +7,7 @@ ORPHANED_OBJECTS_ERROR_MSG_FIELD, ORPHANED_OBJECTS_SIZE_FIELD, STATE_LOCAL_PATH, + create_orphaned_objects_state, ) from ch_tools.chadmin.internal.zookeeper import get_zk_node from ch_tools.common.result import CRIT, OK, WARNING, Result @@ -49,34 +51,25 @@ def orphaned_objects_command( ) -> Result: _check_mutually_exclusive(state_local, state_zk_path) - state = dict() - if state_local: - state = _local_get_orphaned_objects_state() - - if state_zk_path: - state = _zk_get_orphaned_objects_state(ctx, state_zk_path) - - total_size = state.get(ORPHANED_OBJECTS_SIZE_FIELD) - error_msg = state.get(ORPHANED_OBJECTS_ERROR_MSG_FIELD) - - msg = "" - if total_size is None: - msg += f'Orphaned objects state not have field "{ORPHANED_OBJECTS_SIZE_FIELD}".' - if error_msg is None: - msg += f'Orphaned objects state not have field "{ORPHANED_OBJECTS_ERROR_MSG_FIELD}".' + state = _get_orphaned_objects_state(ctx, state_local, state_zk_path) - if msg != "": + valid, msg = _orphaned_objects_state_validate(state) + if not valid: return Result(CRIT, msg) + total_size = state[ORPHANED_OBJECTS_SIZE_FIELD] + error_msg = state[ORPHANED_OBJECTS_ERROR_MSG_FIELD] + if error_msg != "": return Result(CRIT, error_msg) - msg = f"Total size: {total_size}" - if isinstance(total_size, int) and total_size >= crit: - return Result(CRIT, msg) - if isinstance(total_size, int) and total_size >= warn: - return Result(WARNING, msg) - return Result(OK, msg) + result_msg = f"Total size: {total_size}" + if total_size >= crit: + return Result(CRIT, result_msg) + if total_size >= warn: + return Result(WARNING, result_msg) + + return Result(OK, result_msg) def _check_mutually_exclusive(state_local, state_zk_path): @@ -91,22 +84,43 @@ def _check_mutually_exclusive(state_local, state_zk_path): ) +def _orphaned_objects_state_validate(state: dict) -> Tuple[bool, str]: + total_size = state.get(ORPHANED_OBJECTS_SIZE_FIELD) + error_msg = state.get(ORPHANED_OBJECTS_ERROR_MSG_FIELD) + + msg = "" + if total_size is None: + msg += f'Orphaned objects state not have field "{ORPHANED_OBJECTS_SIZE_FIELD}".' + if error_msg is None: + msg += f'Orphaned objects state not have field "{ORPHANED_OBJECTS_ERROR_MSG_FIELD}".' + + return msg == "", msg + + +def _get_orphaned_objects_state( + ctx: click.Context, state_local: bool, state_zk_path: str +) -> dict: + state = dict() + + if state_local: + state = _local_get_orphaned_objects_state() + + if state_zk_path: + state = _zk_get_orphaned_objects_state(ctx, state_zk_path) + + return state + + def _local_get_orphaned_objects_state() -> dict: try: with open(STATE_LOCAL_PATH, mode="r", encoding="utf-8") as file: return json.load(file) except Exception as e: - return { - ORPHANED_OBJECTS_SIZE_FIELD: 0, - ORPHANED_OBJECTS_ERROR_MSG_FIELD: str(e), - } + return create_orphaned_objects_state(0, str(e)) def _zk_get_orphaned_objects_state(ctx: click.Context, state_zk_path: str) -> dict: try: return json.loads(get_zk_node(ctx, state_zk_path)) except Exception as e: - return { - ORPHANED_OBJECTS_SIZE_FIELD: 0, - ORPHANED_OBJECTS_ERROR_MSG_FIELD: str(e), - } + return create_orphaned_objects_state(0, str(e)) diff --git a/tests/features/monrun.feature b/tests/features/monrun.feature index fed382d9..b96db5f5 100644 --- a/tests/features/monrun.feature +++ b/tests/features/monrun.feature @@ -502,3 +502,14 @@ Feature: ch-monitoring tool """ 1;Unknown error: One of these options must be provided: --state-local, --state-zk-path """ + + Scenario: Check clickhouse orphaned objects with not empty error_msg + When we create file /tmp/object_storage_cleanup_state.json with data "{ \"orphaned_objects_size\": 0, \"error_msg\": \"ERROR\" }" + And we execute command on clickhouse01 + """ + ch-monitoring orphaned-objects --state-local + """ + Then we get response + """ + 2;ERROR + """ diff --git a/tests/steps/common.py b/tests/steps/common.py index 4413fe19..b6e98301 100644 --- a/tests/steps/common.py +++ b/tests/steps/common.py @@ -88,15 +88,21 @@ def step_get_response_not_contains(context, entry): assert_that(context.response, is_not(contains_string(entry))) # type: ignore +@when("we create file {file_path} with data '{data}'") +def step_create_file(context, file_path, data): + container = docker.get_container(context, "clickhouse01") + result = container.exec_run( + ["bash", "-c", f'echo "{data}" > {file_path}'], user="root" + ) + assert result.exit_code == 0 + + @then("we get file {file_path}") def step_get_file(context, file_path): container = docker.get_container(context, "clickhouse01") - context.command = context.text.strip() result = container.exec_run(["bash", "-c", f"cat {file_path}"], user="root") context.response = result.output.decode().strip() - context.exit_code = result.exit_code - if context.exit_code != 0: - assert_that("", equal_to(context.text)) + assert result.exit_code == 0 assert_that(context.response, equal_to(context.text))