From 179187940736e1cf4a83baff53e825fa8d135f14 Mon Sep 17 00:00:00 2001 From: Peter Romancik Date: Thu, 30 Apr 2026 14:49:27 +0200 Subject: [PATCH 1/8] add lib command for saving cfgsync configs --- pcs/Makefile.am | 1 + pcs/lib/commands/pcs_cfgsync.py | 225 ++++++- pcs/lib/pcs_cfgsync/const.py | 3 +- pcs/lib/pcs_cfgsync/fetcher.py | 16 +- pcs/lib/pcs_cfgsync/tools.py | 13 + .../tier0/lib/commands/test_pcs_cfgsync.py | 569 ++++++++++++++++++ pcs_test/tools/command_env/config_raw_file.py | 65 ++ 7 files changed, 859 insertions(+), 33 deletions(-) create mode 100644 pcs/lib/pcs_cfgsync/tools.py diff --git a/pcs/Makefile.am b/pcs/Makefile.am index e88448561..0fb30d2b1 100644 --- a/pcs/Makefile.am +++ b/pcs/Makefile.am @@ -444,6 +444,7 @@ EXTRA_DIST = \ lib/pcs_cfgsync/__init__.py \ lib/pcs_cfgsync/save_sync.py \ lib/pcs_cfgsync/sync_files.py \ + lib/pcs_cfgsync/tools.py \ lib/pcs_cfgsync/validations.py \ lib/permissions/checker.py \ lib/permissions/config/exporter.py \ diff --git a/pcs/lib/commands/pcs_cfgsync.py b/pcs/lib/commands/pcs_cfgsync.py index 65d1dc649..145b17b8d 100644 --- a/pcs/lib/commands/pcs_cfgsync.py +++ b/pcs/lib/commands/pcs_cfgsync.py @@ -1,20 +1,27 @@ -from typing import Mapping, cast +from typing import Mapping, Optional, cast +from pcs import settings from pcs.common import reports from pcs.common.file import RawFileError +from pcs.common.file_type_codes import FileTypeCode from pcs.common.pcs_cfgsync_dto import SyncConfigsDto from pcs.lib.env import LibraryEnvironment, LibraryError from pcs.lib.file.instance import FileInstance from pcs.lib.file.raw_file import raw_file_error_report -from pcs.lib.interface.config import ParserErrorException +from pcs.lib.interface.config import ( + FacadeInterface, + ParserErrorException, + SyncVersionFacadeInterface, +) from pcs.lib.node import get_existing_nodes_names from pcs.lib.pcs_cfgsync.actions import UPDATE_SYNC_OPTIONS_ACTIONS from pcs.lib.pcs_cfgsync.config.facade import Facade as CfgsyncCtlFacade -from pcs.lib.pcs_cfgsync.const import SYNCED_CONFIGS +from pcs.lib.pcs_cfgsync.const import CONFIGS_WITH_BACKUPS, SYNCED_CONFIGS from pcs.lib.pcs_cfgsync.sync_files import ( sync_pcs_settings_in_cluster, update_pcs_settings_locally, ) +from pcs.lib.pcs_cfgsync.tools import get_file_hash from pcs.lib.pcs_cfgsync.validations import validate_update_sync_options from pcs.lib.permissions.config.facade import FacadeV2 as PcsSettingsFacade @@ -55,6 +62,131 @@ def get_configs(env: LibraryEnvironment, cluster_name: str) -> SyncConfigsDto: return SyncConfigsDto(current_cluster_name, configs) +def set_configs( + env: LibraryEnvironment, + cluster_name: str, + configs: dict[FileTypeCode, str], + force_flags: reports.types.ForceFlags = (), +) -> None: + """ + Save configuration files locally. + + cluster_name -- expected cluster name. End with an error if the requested + node is not in the cluster with the expected name. + configs -- contents of files to be saved + force_flags -- list of flags codes + """ + # the node is either in cluster (has_corosync_conf) and the cluster names + # must match, or the node is not in cluster and there is no cluster name to + # check against - this happens when this command is used on a node that is + # being added to a cluster. + if env.has_corosync_conf: + local_cluster_name = env.get_corosync_conf().get_cluster_name() + if local_cluster_name != cluster_name: + env.report_processor.report( + reports.ReportItem.error( + reports.messages.NodeReportsUnexpectedClusterName( + cluster_name + ) + ) + ) + if env.report_processor.has_errors: + raise LibraryError() + + # continue even when we cannot read the cfgsync_ctl file, only report + # warnings and then use default values + cfgsync_ctl_facade, report_list = __read_cfgsync_ctl(report_warnings=True) + env.report_processor.report_list(report_list) + + for file_type in sorted(configs): + if file_type not in SYNCED_CONFIGS: + env.report_processor.report( + reports.ReportItem.warning( + reports.messages.PcsCfgsyncConfigUnsupported(file_type) + ) + ) + continue + + file_instance = FileInstance.for_common(file_type) + raw_text = configs[file_type] + + try: + remote_file = cast( + SyncVersionFacadeInterface, + file_instance.raw_to_facade(raw_text.encode("utf-8")), + ) + except ParserErrorException as e: + env.report_processor.report_list( + file_instance.parser_exception_to_report_list(e) + + [ + reports.ReportItem.error( + reports.messages.PcsCfgsyncConfigSaveError(file_type) + ) + ] + ) + continue + + # Backwards compatibility: original Ruby implementation ignored file + # read errors, and treated them the same as if the file was nonexistent + # This allows us to replace invalid local files. + # + # So we report warnings, and treat the file the same as nonexistent. + local_file: Optional[SyncVersionFacadeInterface] + local_file, report_list = __read_local_file( + file_instance, report_warnings=True + ) + env.report_processor.report_list(report_list) + + if local_file is None: + report_list = __accept_and_write_file( + file_instance, remote_file, create_backup=False + ) + env.report_processor.report_list(report_list) + continue + + local_file_hash = get_file_hash(file_instance, local_file) + remote_file_hash = get_file_hash(file_instance, remote_file) + if ( + remote_file.data_version == local_file.data_version + and remote_file_hash == local_file_hash + ): + # The file is the same, we don't want to backup and save it + # again, just report success + env.report_processor.report( + reports.ReportItem.info( + reports.messages.PcsCfgsyncConfigAccepted(file_type) + ) + ) + continue + + remote_is_newer = ( + remote_file.data_version > local_file.data_version + or ( + local_file.data_version == remote_file.data_version + and remote_file_hash > local_file_hash + ) + ) + if remote_is_newer or reports.codes.FORCE in force_flags: + report_list = __accept_and_write_file( + file_instance, + remote_file, + create_backup=file_type in CONFIGS_WITH_BACKUPS, + backup_count=cfgsync_ctl_facade.file_backup_count, + ) + env.report_processor.report_list(report_list) + continue + + # no condition for accepting the file was fulfilled, so we reject + env.report_processor.report( + reports.ReportItem.warning( + reports.messages.PcsCfgsyncConfigRejected(file_type) + ) + ) + + if env.report_processor.has_errors: + raise LibraryError() + + def update_sync_options( env: LibraryEnvironment, options: Mapping[str, str] ) -> None: @@ -69,21 +201,8 @@ def update_sync_options( ).has_errors: raise LibraryError() - cfgsync_ctl_instance = FileInstance.for_pcs_cfgsync_ctl() - if not cfgsync_ctl_instance.raw_file.exists(): - cfgsync_ctl_facade = CfgsyncCtlFacade.create() - else: - try: - cfgsync_ctl_facade = cast( - CfgsyncCtlFacade, cfgsync_ctl_instance.read_to_facade() - ) - except RawFileError as e: - env.report_processor.report(raw_file_error_report(e)) - except ParserErrorException as e: - env.report_processor.report_list( - cfgsync_ctl_instance.parser_exception_to_report_list(e) - ) - if env.report_processor.has_errors: + cfgsync_ctl_facade, report_list = __read_cfgsync_ctl() + if env.report_processor.report_list(report_list).has_errors: raise LibraryError() for option_name, option_value in options.items(): @@ -92,7 +211,7 @@ def update_sync_options( ) try: - cfgsync_ctl_instance.write_facade( + FileInstance.for_pcs_cfgsync_ctl().write_facade( cfgsync_ctl_facade, can_overwrite=True ) except RawFileError as e: @@ -101,6 +220,74 @@ def update_sync_options( raise LibraryError() +def __read_local_file[T: FacadeInterface]( + file_instance: FileInstance, + report_warnings: bool, +) -> tuple[Optional[T], reports.ReportItemList]: + report_list: reports.ReportItemList = [] + + if not file_instance.raw_file.exists(): + return None, report_list + + try: + return cast(T, file_instance.read_to_facade()), report_list + except RawFileError as e: + report_list.append( + raw_file_error_report(e, is_forced_or_warning=report_warnings) + ) + except ParserErrorException as e: + report_list.extend( + file_instance.parser_exception_to_report_list( + e, is_forced_or_warning=report_warnings + ) + ) + return None, report_list + + +def __read_cfgsync_ctl( + report_warnings: bool = False, +) -> tuple[CfgsyncCtlFacade, reports.ReportItemList]: + local_file: Optional[CfgsyncCtlFacade] + local_file, report_list = __read_local_file( + FileInstance.for_pcs_cfgsync_ctl(), report_warnings + ) + if local_file is None: + local_file = CfgsyncCtlFacade.create() + return local_file, report_list + + +def __accept_and_write_file( + file_instance: FileInstance, + file: FacadeInterface, + create_backup: bool, + backup_count: int = settings.pcs_cfgsync_file_backup_count_default, +) -> reports.ReportItemList: + report_list: reports.ReportItemList = [] + file_type = file_instance.toolbox.file_type_code + try: + if create_backup: + file_instance.raw_file.backup() + file_instance.raw_file.remove_old_backups(backup_count) + # if any of the backup methods raise RawFileError, we do not + # want to try overwriting the file + file_instance.write_facade(file, can_overwrite=True) + report_list.append( + reports.ReportItem.info( + reports.messages.PcsCfgsyncConfigAccepted(file_type) + ) + ) + except RawFileError as e: + report_list.extend( + [ + raw_file_error_report(e), + reports.ReportItem.error( + reports.messages.PcsCfgsyncConfigSaveError(file_type) + ), + ] + ) + return report_list + + # Internal use only diff --git a/pcs/lib/pcs_cfgsync/const.py b/pcs/lib/pcs_cfgsync/const.py index ee886ca92..7085ae12d 100644 --- a/pcs/lib/pcs_cfgsync/const.py +++ b/pcs/lib/pcs_cfgsync/const.py @@ -1,3 +1,4 @@ from pcs.common.file_type_codes import PCS_KNOWN_HOSTS, PCS_SETTINGS_CONF -SYNCED_CONFIGS = [PCS_KNOWN_HOSTS, PCS_SETTINGS_CONF] +SYNCED_CONFIGS = (PCS_KNOWN_HOSTS, PCS_SETTINGS_CONF) +CONFIGS_WITH_BACKUPS = (PCS_SETTINGS_CONF,) diff --git a/pcs/lib/pcs_cfgsync/fetcher.py b/pcs/lib/pcs_cfgsync/fetcher.py index 52440d30c..dd4b05b18 100644 --- a/pcs/lib/pcs_cfgsync/fetcher.py +++ b/pcs/lib/pcs_cfgsync/fetcher.py @@ -1,6 +1,5 @@ from collections import defaultdict from dataclasses import replace -from hashlib import sha1 from typing import Iterable, Optional, cast from pcs.common.file_type_codes import FileTypeCode @@ -13,12 +12,12 @@ from pcs.lib.file.instance import FileInstance from pcs.lib.file.raw_file import RawFileError, raw_file_error_report from pcs.lib.interface.config import ( - FacadeInterface, ParserErrorException, SyncVersionFacadeInterface, ) from .const import SYNCED_CONFIGS +from .tools import get_file_hash class ConfigFetcher: @@ -85,7 +84,7 @@ def fetch( configs_to_update[file_type] = newest_config elif local_config.data_version > newest_config.data_version: pass - elif _file_hash(instance, local_config) != _file_hash( + elif get_file_hash(instance, local_config) != get_file_hash( instance, newest_config ): configs_to_update[file_type] = newest_config @@ -134,15 +133,6 @@ def _parse_received_configs( return result -def _file_hash(file_instance: FileInstance, facade: FacadeInterface) -> str: - # sha1 is used to be compatible with the old ruby implementation. - # The hash is only used to compare and sort the files, not for security - # reasons - return sha1( - file_instance.facade_to_raw(facade), usedforsecurity=False - ).hexdigest() - - def _find_newest_config( file_instance: FileInstance, configs: Iterable[SyncVersionFacadeInterface] ) -> Optional[SyncVersionFacadeInterface]: @@ -155,7 +145,7 @@ def _find_newest_config( for cfg in configs: if cfg.data_version == max_version: - file_hash = _file_hash(file_instance, cfg) + file_hash = get_file_hash(file_instance, cfg) cfg_hash[file_hash] = cfg hash_count[file_hash] += 1 diff --git a/pcs/lib/pcs_cfgsync/tools.py b/pcs/lib/pcs_cfgsync/tools.py new file mode 100644 index 000000000..a94e87897 --- /dev/null +++ b/pcs/lib/pcs_cfgsync/tools.py @@ -0,0 +1,13 @@ +from hashlib import sha1 + +from pcs.lib.file.instance import FileInstance +from pcs.lib.interface.config import FacadeInterface + + +def get_file_hash(file_instance: FileInstance, facade: FacadeInterface) -> str: + # sha1 is used to be compatible with the old ruby implementation. + # The hash is only used to compare and sort the files, not for security + # reasons + return sha1( + file_instance.facade_to_raw(facade), usedforsecurity=False + ).hexdigest() diff --git a/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py b/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py index 8389ead2b..401c5ac6e 100644 --- a/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py +++ b/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py @@ -10,6 +10,7 @@ from pcs_test.tools.command_env import get_env_tools from pcs_test.tools.fixture_pcs_cfgsync import ( fixture_expected_save_sync_reports, + fixture_known_hosts_file_content, fixture_pcs_settings_file_content, fixture_save_sync_new_version_conflict, fixture_save_sync_new_version_error, @@ -565,3 +566,571 @@ def test_sync_error(self): expected_result="error", ) ) + + +class SetConfigs(TestCase): + CLUSTER_NAME = "test99" + DATA_VERSION = 5 + CFGSYNC_CTL_BACKUP_COUNT = 123 + + KNOWN_HOSTS_V5 = fixture_known_hosts_file_content(DATA_VERSION) + KNOWN_HOSTS_V6 = fixture_known_hosts_file_content(DATA_VERSION + 1) + PCS_SETTINGS_V5 = fixture_pcs_settings_file_content(DATA_VERSION) + PCS_SETTINGS_V6 = fixture_pcs_settings_file_content(DATA_VERSION + 1) + + REPORT_KNOWN_HOSTS_SAVE_SUCCESS = fixture.info( + reports.codes.PCS_CFGSYNC_CONFIG_ACCEPTED, + file_type_code=file_type_codes.PCS_KNOWN_HOSTS, + ) + + def setUp(self): + self.env_assist, self.config = get_env_tools(self) + + def _fixture_not_in_cluster(self): + self.config.raw_file.exists( + file_type_codes.COROSYNC_CONF, + settings.corosync_conf_file, + exists=False, + name="corosync_conf.exists", + ) + + def _fixture_in_cluster(self): + self.config.raw_file.exists( + file_type_codes.COROSYNC_CONF, + settings.corosync_conf_file, + name="corosync_conf.exists", + ) + self.config.corosync_conf.load() + + def _fixture_cfgsync_ctl_not_exists(self): + self.config.raw_file.exists( + file_type_codes.PCS_CFGSYNC_CTL, + settings.pcs_cfgsync_ctl_location, + exists=False, + name="cfgsync_ctl.exists", + ) + + def _fixture_cfgsync_ctl_exists(self): + self.config.raw_file.exists( + file_type_codes.PCS_CFGSYNC_CTL, + settings.pcs_cfgsync_ctl_location, + name="cfgsync_ctl.exists", + ) + self.config.raw_file.read( + file_type_codes.PCS_CFGSYNC_CTL, + settings.pcs_cfgsync_ctl_location, + json.dumps({"file_backup_count": self.CFGSYNC_CTL_BACKUP_COUNT}), + name="cfgsync_ctl.read", + ) + + def test_cluster_name_mismatch(self): + self._fixture_in_cluster() + self.env_assist.assert_raise_library_error( + lambda: lib.set_configs( + self.env_assist.get_env(), + "wrong-cluster-name", + {}, + ) + ) + self.env_assist.assert_reports( + [ + fixture.error( + reports.codes.NODE_REPORTS_UNEXPECTED_CLUSTER_NAME, + cluster_name="wrong-cluster-name", + ) + ] + ) + + def test_not_in_cluster_skips_name_check(self): + self._fixture_not_in_cluster() + self._fixture_cfgsync_ctl_not_exists() + lib.set_configs(self.env_assist.get_env(), "any-cluster-name", {}) + + def test_unsupported_file_types(self): + self._fixture_not_in_cluster() + self._fixture_cfgsync_ctl_not_exists() + lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + { + file_type_codes.COROSYNC_CONF: "some content", + "foo": "some content", + }, + ) + self.env_assist.assert_reports( + [ + fixture.warn( + reports.codes.PCS_CFGSYNC_CONFIG_UNSUPPORTED, + file_type_code=file_type_codes.COROSYNC_CONF, + ), + fixture.warn( + reports.codes.PCS_CFGSYNC_CONFIG_UNSUPPORTED, + file_type_code="foo", + ), + ] + ) + + def test_local_file_not_exist_accepted(self): + self._fixture_not_in_cluster() + self._fixture_cfgsync_ctl_not_exists() + self.config.raw_file.exists( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + exists=False, + name="known_hosts.exists", + ) + self.config.raw_file.write( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + self.KNOWN_HOSTS_V5.encode(), + can_overwrite=True, + name="known_hosts.write", + ) + lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + {file_type_codes.PCS_KNOWN_HOSTS: self.KNOWN_HOSTS_V5}, + ) + self.env_assist.assert_reports([self.REPORT_KNOWN_HOSTS_SAVE_SUCCESS]) + + def test_same_version_and_hash_accepted_without_write(self): + self._fixture_not_in_cluster() + self._fixture_cfgsync_ctl_not_exists() + self.config.raw_file.exists( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + name="known_hosts.exists", + ) + self.config.raw_file.read( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + content=self.KNOWN_HOSTS_V5.encode(), + name="known_hosts.read", + ) + lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + {file_type_codes.PCS_KNOWN_HOSTS: self.KNOWN_HOSTS_V5}, + ) + self.env_assist.assert_reports([self.REPORT_KNOWN_HOSTS_SAVE_SUCCESS]) + + def test_remote_newer_version_accepted_with_backup(self): + self._fixture_not_in_cluster() + self._fixture_cfgsync_ctl_exists() + self.config.raw_file.exists( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + name="pcs_settings.exists", + ) + self.config.raw_file.read( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + content=self.PCS_SETTINGS_V5.encode(), + name="pcs_settings.read", + ) + self.config.raw_file.backup( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + name="pcs_settings.backup", + ) + self.config.raw_file.remove_old_backups( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + backup_count=self.CFGSYNC_CTL_BACKUP_COUNT, + name="pcs_settings.remove_old_backups", + ) + self.config.raw_file.write( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + self.PCS_SETTINGS_V6.encode(), + can_overwrite=True, + name="pcs_settings.write", + ) + lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + {file_type_codes.PCS_SETTINGS_CONF: self.PCS_SETTINGS_V6}, + ) + self.env_assist.assert_reports( + [ + fixture.info( + reports.codes.PCS_CFGSYNC_CONFIG_ACCEPTED, + file_type_code=file_type_codes.PCS_SETTINGS_CONF, + ) + ] + ) + + def test_local_newer_version_rejected(self): + self._fixture_not_in_cluster() + self._fixture_cfgsync_ctl_not_exists() + self.config.raw_file.exists( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + name="known_hosts.exists", + ) + self.config.raw_file.read( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + content=self.KNOWN_HOSTS_V6.encode(), + name="known_hosts.read", + ) + lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + {file_type_codes.PCS_KNOWN_HOSTS: self.KNOWN_HOSTS_V5}, + ) + self.env_assist.assert_reports( + [ + fixture.warn( + reports.codes.PCS_CFGSYNC_CONFIG_REJECTED, + file_type_code=file_type_codes.PCS_KNOWN_HOSTS, + ) + ] + ) + + def test_force_flag_overrides_rejection(self): + self._fixture_not_in_cluster() + self._fixture_cfgsync_ctl_not_exists() + self.config.raw_file.exists( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + name="known_hosts.exists", + ) + self.config.raw_file.read( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + content=self.KNOWN_HOSTS_V6.encode(), + name="known_hosts.read", + ) + # no backup, because known-hosts should not have backups + self.config.raw_file.write( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + self.KNOWN_HOSTS_V5.encode(), + can_overwrite=True, + name="known_hosts.write", + ) + lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + {file_type_codes.PCS_KNOWN_HOSTS: self.KNOWN_HOSTS_V5}, + force_flags=(reports.codes.FORCE,), + ) + self.env_assist.assert_reports([self.REPORT_KNOWN_HOSTS_SAVE_SUCCESS]) + + def test_remote_parse_error(self): + self._fixture_not_in_cluster() + self._fixture_cfgsync_ctl_not_exists() + self.env_assist.assert_raise_library_error( + lambda: lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + {file_type_codes.PCS_KNOWN_HOSTS: "{"}, + ) + ) + self.env_assist.assert_reports( + [ + fixture.error( + reports.codes.PARSE_ERROR_JSON_FILE, + file_type_code=file_type_codes.PCS_KNOWN_HOSTS, + line_number=1, + column_number=2, + position=1, + reason="Expecting property name enclosed in double quotes", + full_msg=( + "Expecting property name enclosed in double quotes: " + "line 1 column 2 (char 1)" + ), + file_path=settings.pcsd_known_hosts_location, + ), + fixture.error( + reports.codes.PCS_CFGSYNC_CONFIG_SAVE_ERROR, + file_type_code=file_type_codes.PCS_KNOWN_HOSTS, + ), + ] + ) + + def test_local_file_read_error(self): + self._fixture_not_in_cluster() + self._fixture_cfgsync_ctl_exists() + self.config.raw_file.exists( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + name="pcs_settings.exists", + ) + self.config.raw_file.read( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + exception_msg="Read error", + name="pcs_settings.read", + ) + # no backup, because the file is considered to be "nonexistent" + self.config.raw_file.write( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + self.PCS_SETTINGS_V6.encode(), + can_overwrite=True, + name="pcs_settings.write", + ) + lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + {file_type_codes.PCS_SETTINGS_CONF: self.PCS_SETTINGS_V6}, + ) + self.env_assist.assert_reports( + [ + fixture.warn( + reports.codes.FILE_IO_ERROR, + file_type_code=file_type_codes.PCS_SETTINGS_CONF, + operation="read", + reason="Read error", + file_path=settings.pcsd_settings_conf_location, + ), + fixture.info( + reports.codes.PCS_CFGSYNC_CONFIG_ACCEPTED, + file_type_code=file_type_codes.PCS_SETTINGS_CONF, + ), + ] + ) + + def test_cfgsync_ctl_read_error_continues_as_warning(self): + self._fixture_not_in_cluster() + self.config.raw_file.exists( + file_type_codes.PCS_CFGSYNC_CTL, + settings.pcs_cfgsync_ctl_location, + name="cfgsync_ctl.exists", + ) + self.config.raw_file.read( + file_type_codes.PCS_CFGSYNC_CTL, + settings.pcs_cfgsync_ctl_location, + exception_msg="Read error", + name="cfgsync_ctl.read", + ) + self.config.raw_file.exists( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + exists=False, + name="known_hosts.exists", + ) + self.config.raw_file.write( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + self.KNOWN_HOSTS_V5.encode(), + can_overwrite=True, + name="known_hosts.write", + ) + lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + {file_type_codes.PCS_KNOWN_HOSTS: self.KNOWN_HOSTS_V5}, + ) + self.env_assist.assert_reports( + [ + fixture.warn( + reports.codes.FILE_IO_ERROR, + file_type_code=file_type_codes.PCS_CFGSYNC_CTL, + operation="read", + reason="Read error", + file_path=settings.pcs_cfgsync_ctl_location, + ), + self.REPORT_KNOWN_HOSTS_SAVE_SUCCESS, + ] + ) + + def test_cfgsync_ctl_parse_error_continues_as_warning(self): + self._fixture_not_in_cluster() + self.config.raw_file.exists( + file_type_codes.PCS_CFGSYNC_CTL, + settings.pcs_cfgsync_ctl_location, + name="cfgsync_ctl.exists", + ) + self.config.raw_file.read( + file_type_codes.PCS_CFGSYNC_CTL, + settings.pcs_cfgsync_ctl_location, + "{", + name="cfgsync_ctl.read", + ) + self.config.raw_file.exists( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + exists=False, + name="known_hosts.exists", + ) + self.config.raw_file.write( + file_type_codes.PCS_KNOWN_HOSTS, + settings.pcsd_known_hosts_location, + self.KNOWN_HOSTS_V5.encode(), + can_overwrite=True, + name="known_hosts.write", + ) + lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + {file_type_codes.PCS_KNOWN_HOSTS: self.KNOWN_HOSTS_V5}, + ) + self.env_assist.assert_reports( + [ + fixture.warn( + reports.codes.PARSE_ERROR_JSON_FILE, + file_type_code=file_type_codes.PCS_CFGSYNC_CTL, + line_number=1, + column_number=2, + position=1, + reason="Expecting property name enclosed in double quotes", + full_msg=( + "Expecting property name enclosed in double quotes: " + "line 1 column 2 (char 1)" + ), + file_path=settings.pcs_cfgsync_ctl_location, + ), + self.REPORT_KNOWN_HOSTS_SAVE_SUCCESS, + ] + ) + + def test_backup_error(self): + self._fixture_not_in_cluster() + self._fixture_cfgsync_ctl_not_exists() + self.config.raw_file.exists( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + name="pcs_settings.exists", + ) + self.config.raw_file.read( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + content=self.PCS_SETTINGS_V5.encode(), + name="pcs_settings.read", + ) + self.config.raw_file.backup( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + exception_msg="Backup error", + name="pcs_settings.backup", + ) + self.env_assist.assert_raise_library_error( + lambda: lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + {file_type_codes.PCS_SETTINGS_CONF: self.PCS_SETTINGS_V6}, + ) + ) + self.env_assist.assert_reports( + [ + fixture.error( + reports.codes.FILE_IO_ERROR, + file_type_code=file_type_codes.PCS_SETTINGS_CONF, + operation="backup", + reason="Backup error", + file_path=settings.pcsd_settings_conf_location, + ), + fixture.error( + reports.codes.PCS_CFGSYNC_CONFIG_SAVE_ERROR, + file_type_code=file_type_codes.PCS_SETTINGS_CONF, + ), + ] + ) + + def test_old_backup_removal_error(self): + self._fixture_not_in_cluster() + self._fixture_cfgsync_ctl_not_exists() + self.config.raw_file.exists( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + name="pcs_settings.exists", + ) + self.config.raw_file.read( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + content=self.PCS_SETTINGS_V5.encode(), + name="pcs_settings.read", + ) + self.config.raw_file.backup( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + name="pcs_settings.backup", + ) + self.config.raw_file.remove_old_backups( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + backup_count=settings.pcs_cfgsync_file_backup_count_default, + exception_msg="Old backup removal error", + name="pcs_settings.remove_old_backups", + ) + self.env_assist.assert_raise_library_error( + lambda: lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + {file_type_codes.PCS_SETTINGS_CONF: self.PCS_SETTINGS_V6}, + ) + ) + self.env_assist.assert_reports( + [ + fixture.error( + reports.codes.FILE_IO_ERROR, + file_type_code=file_type_codes.PCS_SETTINGS_CONF, + operation="remove_backup", + reason="Old backup removal error", + file_path=settings.pcsd_settings_conf_location, + ), + fixture.error( + reports.codes.PCS_CFGSYNC_CONFIG_SAVE_ERROR, + file_type_code=file_type_codes.PCS_SETTINGS_CONF, + ), + ] + ) + + def test_multiple_files(self): + self._fixture_not_in_cluster() + self._fixture_cfgsync_ctl_exists() + self.config.raw_file.exists( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + name="pcs_settings.exists", + ) + self.config.raw_file.read( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + content=self.PCS_SETTINGS_V6.encode(), + name="pcs_settings.read", + ) + + self.env_assist.assert_raise_library_error( + lambda: lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + { + file_type_codes.PCS_SETTINGS_CONF: self.PCS_SETTINGS_V5, + file_type_codes.PCS_KNOWN_HOSTS: "{", + "foo": "some content", + }, + ) + ) + self.env_assist.assert_reports( + [ + fixture.error( + reports.codes.PARSE_ERROR_JSON_FILE, + file_type_code=file_type_codes.PCS_KNOWN_HOSTS, + line_number=1, + column_number=2, + position=1, + reason="Expecting property name enclosed in double quotes", + full_msg=( + "Expecting property name enclosed in double quotes: " + "line 1 column 2 (char 1)" + ), + file_path=settings.pcsd_known_hosts_location, + ), + fixture.warn( + reports.codes.PCS_CFGSYNC_CONFIG_REJECTED, + file_type_code=file_type_codes.PCS_SETTINGS_CONF, + ), + fixture.error( + reports.codes.PCS_CFGSYNC_CONFIG_SAVE_ERROR, + file_type_code=file_type_codes.PCS_KNOWN_HOSTS, + ), + fixture.warn( + reports.codes.PCS_CFGSYNC_CONFIG_UNSUPPORTED, + file_type_code="foo", + ), + ] + ) diff --git a/pcs_test/tools/command_env/config_raw_file.py b/pcs_test/tools/command_env/config_raw_file.py index 2d38a6546..4fa6be085 100644 --- a/pcs_test/tools/command_env/config_raw_file.py +++ b/pcs_test/tools/command_env/config_raw_file.py @@ -1,7 +1,11 @@ +from pcs import settings + from pcs_test.tools.command_env.mock_raw_file import ( + RawFileBackupCall, RawFileExistsCall, RawFileReadCall, RawFileRemoveCall, + RawFileRemoveOldBackupsCall, RawFileWriteCall, ) @@ -141,3 +145,64 @@ def remove( file_not_found_exception=file_not_found_exception, ) self.__calls.place(name, call, before, instead) + + def backup( + self, + file_type_code, + path, + *, + exception_msg=None, + name="raw_file.backup", + before=None, + instead=None, + ): + """ + Create a call for backing up a file + + string file_type_code -- item from pcs.common.file_type_codes + string path -- expected file path + string exception_msg -- resulting error in case of unsuccessful backup + string name -- the key of the call + string before -- the key of a call before which this call is to be + placed + string instead -- the key of a call instead of which this new call is to + be placed + """ + call = RawFileBackupCall( + file_type_code, + path, + exception_msg=exception_msg, + ) + self.__calls.place(name, call, before, instead) + + def remove_old_backups( + self, + file_type_code, + path, + *, + backup_count=settings.pcs_cfgsync_file_backup_count_default, + exception_msg=None, + name="raw_file.remove_old_backups", + before=None, + instead=None, + ): + """ + Create a call for removing old backups of a file + + string file_type_code -- item from pcs.common.file_type_codes + string path -- expected file path + int backup_count -- expected number of backups to keep + string exception_msg -- resulting error in case of unsuccessful removal + string name -- the key of the call + string before -- the key of a call before which this call is to be + placed + string instead -- the key of a call instead of which this new call is to + be placed + """ + call = RawFileRemoveOldBackupsCall( + file_type_code, + path, + backup_count=backup_count, + exception_msg=exception_msg, + ) + self.__calls.place(name, call, before, instead) From 118ee40858d9bc33e56fb7c07e26c59fdd876d74 Mon Sep 17 00:00:00 2001 From: Peter Romancik Date: Thu, 30 Apr 2026 14:54:36 +0200 Subject: [PATCH 2/8] add Python handler for /remote/set_configs --- pcs/daemon/app/api_v0.py | 97 +++++++ pcs/daemon/app/sinatra_remote.py | 31 --- .../async_tasks/worker/command_mapping.py | 5 + pcs/daemon/run.py | 5 +- pcs_test/tier0/daemon/app/test_api_v0.py | 251 ++++++++++++++++++ pcs_test/tier0/daemon/app/test_app_remote.py | 77 ------ 6 files changed, 354 insertions(+), 112 deletions(-) diff --git a/pcs/daemon/app/api_v0.py b/pcs/daemon/app/api_v0.py index 86a399d63..e34c05f71 100644 --- a/pcs/daemon/app/api_v0.py +++ b/pcs/daemon/app/api_v0.py @@ -444,6 +444,98 @@ async def _handle_request(self) -> None: self.write("Sync thread options updated successfully") +class SetConfigsHandler(_BaseApiV0Handler): + _REPORT_CODE_TO_RESULT = { + reports.codes.PCS_CFGSYNC_CONFIG_ACCEPTED: "accepted", + reports.codes.PCS_CFGSYNC_CONFIG_REJECTED: "rejected", + reports.codes.PCS_CFGSYNC_CONFIG_SAVE_ERROR: "error", + reports.codes.PCS_CFGSYNC_CONFIG_UNSUPPORTED: "not_supported", + } + + _LEGACY_TO_FILE_TYPE_CODE = { + "pcs_settings.conf": file_type_codes.PCS_SETTINGS_CONF, + "known-hosts": file_type_codes.PCS_KNOWN_HOSTS, + } + _FILE_TYPE_CODE_TO_LEGACY = { + v: k for k, v in _LEGACY_TO_FILE_TYPE_CODE.items() + } + + _sync_config_lock: Lock + + def initialize( # type: ignore[override] + self, + api_auth_provider_factory: ApiAuthProviderFactoryInterface, + scheduler: Scheduler, + sync_config_lock: Lock, + ) -> None: + super().initialize(api_auth_provider_factory, scheduler) + self._sync_config_lock = sync_config_lock + + async def _handle_request(self) -> None: + try: + configs_json = json.loads(self.get_argument("configs", "")) + except json.JSONDecodeError: + # original handler returned 200 in this case + self.write({"status": "bad_json"}) + return + + try: + cluster_name = configs_json.get("cluster_name", "") + force = bool(configs_json.get("force", False)) + + configs_raw = configs_json.get("configs", {}) + configs = {} + non_file_configs = [] + for name, data in configs_raw.items(): + if data.get("type") == "file": + configs[self._LEGACY_TO_FILE_TYPE_CODE.get(name, name)] = ( + str(data.get("text", "")) + ) + else: + non_file_configs.append(name) + except AttributeError: + # original handler returned 200 in this case + self.write({"status": "bad_json"}) + return + + async with self._sync_config_lock: + result = await self._run_library_command( + "pcs_cfgsync.set_configs", + { + "cluster_name": cluster_name, + "configs": configs, + "force_flags": [reports.codes.FORCE] if force else [], + }, + ) + + if any( + rep.message.code + == reports.codes.NODE_REPORTS_UNEXPECTED_CLUSTER_NAME + for rep in result.reports + ): + self.write({"status": "wrong_cluster_name"}) + return + + not_file_results = dict.fromkeys(non_file_configs, "not_supported") + real_results = { + self._FILE_TYPE_CODE_TO_LEGACY.get( + rep.message.payload["file_type_code"], + rep.message.payload["file_type_code"], + ): self._REPORT_CODE_TO_RESULT[rep.message.code] + for rep in result.reports + if rep.message.code in self._REPORT_CODE_TO_RESULT + } + # default to "error" for any file that received no status report + for file_type in configs: + legacy_name = self._FILE_TYPE_CODE_TO_LEGACY.get( + file_type, file_type + ) + if legacy_name not in real_results: + real_results[legacy_name] = "error" + + self.write({"status": "ok", "result": not_file_results | real_results}) + + class SetPermissionsHandler(_BaseApiV0Handler): """ Input format: @@ -616,6 +708,11 @@ def r(url: str) -> str: (r("booth_get_config"), BoothGetConfigHandler, params), # cfgsync (r("get_configs"), GetConfigsHandler, params), + ( + r("set_configs"), + SetConfigsHandler, + {**params, "sync_config_lock": sync_config_lock}, + ), ( r("set_sync_options"), SetSyncOptionsHandler, diff --git a/pcs/daemon/app/sinatra_remote.py b/pcs/daemon/app/sinatra_remote.py index 4d9a5573c..2a46348f9 100644 --- a/pcs/daemon/app/sinatra_remote.py +++ b/pcs/daemon/app/sinatra_remote.py @@ -1,7 +1,5 @@ from typing import Optional -from tornado.locks import Lock - from pcs.daemon import log, ruby_pcsd from pcs.daemon.app.auth_provider import ( ApiAuthProviderFactoryInterface, @@ -64,29 +62,6 @@ async def _handle_request(self) -> None: self.send_sinatra_result(result) -class SyncConfigMutualExclusive(SinatraRemote): - """ - SyncConfigMutualExclusive handles urls which should be directed to the - Sinatra remote (non-GUI) functions that can not run at the same time as - config synchronization. The exclusivity is achieved by sync_config_lock. - """ - - __sync_config_lock: Lock - - def initialize( # type: ignore[override] - self, - api_auth_provider_factory: ApiAuthProviderFactoryInterface, - ruby_pcsd_wrapper: ruby_pcsd.Wrapper, - sync_config_lock: Lock, - ) -> None: - super().initialize(api_auth_provider_factory, ruby_pcsd_wrapper) - self.__sync_config_lock = sync_config_lock - - async def _handle_request(self) -> None: - async with self.__sync_config_lock: - await super()._handle_request() - - class SetCerts(SinatraRemote): """ SetCerts handles url for setting new certificate and key. It calls the @@ -117,7 +92,6 @@ async def _handle_request(self) -> None: def get_routes( api_auth_provider_factory: ApiAuthProviderFactoryInterface, ruby_pcsd_wrapper: ruby_pcsd.Wrapper, - sync_config_lock: Lock, https_server_manage: HttpsServerManage, ) -> RoutesType: sinatra_remote_options = dict( @@ -136,10 +110,5 @@ def get_routes( https_server_manage=https_server_manage, ), ), - ( - r"/remote/set_configs", - SyncConfigMutualExclusive, - dict(**sinatra_remote_options, sync_config_lock=sync_config_lock), - ), (r"/remote/.*", SinatraRemote, sinatra_remote_options), ] diff --git a/pcs/daemon/async_tasks/worker/command_mapping.py b/pcs/daemon/async_tasks/worker/command_mapping.py index 911dac0d7..236da44cb 100644 --- a/pcs/daemon/async_tasks/worker/command_mapping.py +++ b/pcs/daemon/async_tasks/worker/command_mapping.py @@ -309,6 +309,10 @@ class _Cmd: cmd=pcs_cfgsync.get_configs, required_permission=p.FULL, ), + "pcs_cfgsync.set_configs": _Cmd( + cmd=pcs_cfgsync.set_configs, + required_permission=p.FULL, + ), "pcs_cfgsync.update_sync_options": _Cmd( cmd=pcs_cfgsync.update_sync_options, required_permission=p.FULL, @@ -533,6 +537,7 @@ class _Cmd: "cluster.set_permissions", "manage_clusters.add_cluster", "manage_clusters.remove_clusters", + "pcs_cfgsync.set_configs", "pcs_cfgsync.update_sync_options", "qdevice.qdevice_net_get_ca_certificate", "resource_agent.describe_agent", diff --git a/pcs/daemon/run.py b/pcs/daemon/run.py index 6be6dfc86..0b9379bbd 100644 --- a/pcs/daemon/run.py +++ b/pcs/daemon/run.py @@ -180,10 +180,7 @@ def make_app(https_server_manage: HttpsServerManage): ) routes.extend( sinatra_remote.get_routes( - api_auth_factory, - ruby_pcsd_wrapper, - sync_config_lock, - https_server_manage, + api_auth_factory, ruby_pcsd_wrapper, https_server_manage ) ) diff --git a/pcs_test/tier0/daemon/app/test_api_v0.py b/pcs_test/tier0/daemon/app/test_api_v0.py index ea09dd867..131591a34 100644 --- a/pcs_test/tier0/daemon/app/test_api_v0.py +++ b/pcs_test/tier0/daemon/app/test_api_v0.py @@ -1211,6 +1211,257 @@ def test_failure(self): ) +class SetConfigs(ApiV0HandlerTest): + url = "/remote/set_configs" + command = "pcs_cfgsync.set_configs" + + def _request_body(self, data: dict) -> str: + return urlencode({"configs": json.dumps(data)}) + + def test_locked(self): + self.sync_config_lock.acquire() + + try: + self.fetch( + self.url, + body=self._request_body({"cluster_name": "", "configs": {}}), + ) + except TornadoTimeoutError: + self.sync_config_lock.release() + self.io_loop.run_sync(lambda: None) + else: + raise AssertionError("Timeout not raised") + + def test_bad_json(self): + bad_inputs = [ + "not a valid json", + [], + {"configs": "not a dict"}, + {"configs": []}, + {"configs": {"known-hosts": "not a dict"}}, + {"configs": {"known-hosts": []}}, + ] + + for data in bad_inputs: + with self.subTest(value=data): + self.mock_run_library_command.reset_mock() + response = self.fetch( + self.url, body=urlencode({"configs": data}) + ) + self.assert_body( + response.body, json.dumps({"status": "bad_json"}) + ) + self.assertEqual(response.code, 200) + self.mock_run_library_command.assert_not_called() + + def test_success_empty(self): + self.mock_run_library_command.return_value = self.result_success() + response = self.fetch( + self.url, + body=self._request_body({"cluster_name": "test", "configs": {}}), + ) + self.assertEqual(response.code, 200) + self.assert_body( + response.body, json.dumps({"status": "ok", "result": {}}) + ) + self.mock_run_library_command.assert_called_once_with( + self.command, + {"cluster_name": "test", "configs": {}, "force_flags": []}, + ) + + def test_success_multiple_files(self): + self.mock_run_library_command.return_value = self.result_success( + reports=[ + reports.ReportItem.info( + reports.messages.PcsCfgsyncConfigAccepted( + file_type_codes.PCS_KNOWN_HOSTS + ) + ).to_dto(), + reports.ReportItem.info( + reports.messages.PcsCfgsyncConfigAccepted( + file_type_codes.PCS_SETTINGS_CONF + ) + ).to_dto(), + reports.ReportItem.warning( + reports.messages.PcsCfgsyncConfigUnsupported( + file_type_codes.FileTypeCode("some-other-file") + ) + ).to_dto(), + ], + ) + response = self.fetch( + self.url, + body=self._request_body( + { + "cluster_name": "test", + "configs": { + "known-hosts": { + "type": "file", + "text": "known-hosts content", + }, + "pcs_settings.conf": { + "type": "file", + "text": "settings content", + }, + "some-other-file": { + "type": "file", + # missing text, default empty string used + }, + }, + } + ), + ) + self.assertEqual(response.code, 200) + self.assert_body( + response.body, + json.dumps( + { + "status": "ok", + "result": { + "known-hosts": "accepted", + "pcs_settings.conf": "accepted", + "some-other-file": "not_supported", + }, + } + ), + ) + self.mock_run_library_command.assert_called_once_with( + self.command, + { + "cluster_name": "test", + "configs": { + file_type_codes.PCS_KNOWN_HOSTS: "known-hosts content", + file_type_codes.PCS_SETTINGS_CONF: "settings content", + "some-other-file": "", + }, + "force_flags": [], + }, + ) + + def test_success_with_force(self): + self.mock_run_library_command.return_value = self.result_success() + response = self.fetch( + self.url, + body=self._request_body( + {"cluster_name": "test", "force": True, "configs": {}} + ), + ) + self.assertEqual(response.code, 200) + self.assert_body( + response.body, json.dumps({"status": "ok", "result": {}}) + ) + self.mock_run_library_command.assert_called_once_with( + self.command, + { + "cluster_name": "test", + "configs": {}, + "force_flags": [reports.codes.FORCE], + }, + ) + + def test_success_not_type_file(self): + self.mock_run_library_command.return_value = self.result_success() + response = self.fetch( + self.url, + body=self._request_body( + { + "cluster_name": "test", + "configs": { + "known-hosts": {"type": "other", "text": "content"}, + }, + } + ), + ) + self.assertEqual(response.code, 200) + self.assert_body( + response.body, + json.dumps( + {"status": "ok", "result": {"known-hosts": "not_supported"}} + ), + ) + self.mock_run_library_command.assert_called_once_with( + self.command, + {"cluster_name": "test", "configs": {}, "force_flags": []}, + ) + + def test_success_missing_report_for_file(self): + self.mock_run_library_command.return_value = self.result_success( + reports=[ + # success report for only one of the files + reports.ReportItem.info( + reports.messages.PcsCfgsyncConfigAccepted( + file_type_codes.PCS_KNOWN_HOSTS + ) + ).to_dto() + ], + ) + response = self.fetch( + self.url, + body=self._request_body( + { + "cluster_name": "test", + "configs": { + "known-hosts": { + "type": "file", + "text": "known-hosts content", + }, + "pcs_settings.conf": { + "type": "file", + "text": "settings content", + }, + }, + } + ), + ) + self.assertEqual(response.code, 200) + self.assert_body( + response.body, + json.dumps( + { + "status": "ok", + "result": { + "known-hosts": "accepted", + # should default to "error", since no report for this + # file was returned from the lib command + "pcs_settings.conf": "error", + }, + } + ), + ) + self.mock_run_library_command.assert_called_once_with( + self.command, + { + "cluster_name": "test", + "configs": { + file_type_codes.PCS_KNOWN_HOSTS: "known-hosts content", + file_type_codes.PCS_SETTINGS_CONF: "settings content", + }, + "force_flags": [], + }, + ) + + def test_wrong_cluster_name(self): + self.mock_run_library_command.return_value = self.result_failure( + report_items=[ + reports.ReportItem.error( + reports.messages.NodeReportsUnexpectedClusterName("test") + ).to_dto() + ], + ) + response = self.fetch( + self.url, + body=self._request_body({"cluster_name": "test", "configs": {}}), + ) + self.assertEqual(response.code, 200) + self.assert_body( + response.body, json.dumps({"status": "wrong_cluster_name"}) + ) + self.mock_run_library_command.assert_called_once_with( + self.command, + {"cluster_name": "test", "configs": {}, "force_flags": []}, + ) + + class SetPermissions(ApiV0HandlerTest): url = "/remote/set_permissions" command = "cluster.set_permissions" diff --git a/pcs_test/tier0/daemon/app/test_app_remote.py b/pcs_test/tier0/daemon/app/test_app_remote.py index 05d84cdf4..e5cfe3c03 100644 --- a/pcs_test/tier0/daemon/app/test_app_remote.py +++ b/pcs_test/tier0/daemon/app/test_app_remote.py @@ -1,10 +1,6 @@ import base64 import logging from unittest import mock -from urllib.parse import urlencode - -from tornado.locks import Lock -from tornado.util import TimeoutError as TornadoTimeoutError from pcs.daemon import http_server, ruby_pcsd from pcs.daemon.app import sinatra_remote @@ -22,7 +18,6 @@ def setUp(self): self.https_server_manage = mock.MagicMock( spec_set=http_server.HttpsServerManage ) - self.lock = Lock() self.api_auth_provider_factory = MockAuthProviderFactory() super().setUp() @@ -30,7 +25,6 @@ def get_routes(self): return sinatra_remote.get_routes( self.api_auth_provider_factory, self.wrapper, - self.lock, self.https_server_manage, ) @@ -104,74 +98,3 @@ def test_success_desired_user(self): self.wrapper.run_ruby_payload, {"username": "foo", "groups": ["haclient", "wheel", "square"]}, ) - - -class SyncConfigMutualExclusive(AppTest): - """ - This class contains tests that the request handler of url - `/remote/set_sync_options` waits until current synchronization is done (i.e. - respects lock). - - Every test simply calls url `/remote/set_sync_options`. If there is no lock - (it means that synchronization is not in progress) the handler gives - response. If there is a lock handler waits and the request fails because of - timeout and test detects an expected timeout error. - """ - - def fetch_set_sync_options(self, method): - def fetch_sync_options(): - return self.http_client.fetch( - self.get_url("/remote/set_configs"), **kwargs - ) - - kwargs = ( - dict(method=method, body=urlencode({})) - if method == "POST" - else dict(method=method) - ) - kwargs["headers"] = {"Cookie": "token=1234"} - - # Without lock the timeout should be enough to finish task. With the - # lock it should raise because of timeout. The same timeout is used for - # noticing differences between test with and test without lock. - # The timeout needs to be long enough for the test to fit into it even - # if running on a slower machine. And it should be short enough not to - # make the test run unnecessary long. - return self.io_loop.run_sync(fetch_sync_options, timeout=2.5) - - def check_call_wrapper_without_lock(self, method): - self.assert_wrappers_response(self.fetch_set_sync_options(method)) - - def check_locked(self, method): - self.lock.acquire() - try: - self.fetch_set_sync_options(method) - except TornadoTimeoutError: - # The http_client timeouted because of lock and this is how we test - # the locking function. However event loop on the server side should - # finish. So we release the lock and the request successfully - # finish. - self.lock.release() - # Now, there is an unfinished request. It was started by calling - # fetch("/remote/set_sync_options") (in self.fetch_set_sync_options) - # and it was waiting for the lock to be released. - # The lock was released and the request is able to be finished now. - # So, io_loop needs an opportunity to execute the rest of request. - # Next line runs io_loop to finish hanging request. Without this an - # error appears during calling - # `self.http_server.close_all_connections` in tearDown... - self.io_loop.run_sync(lambda: None) - else: - raise AssertionError("Timeout not raised") - - def test_get_not_locked(self): - self.check_call_wrapper_without_lock("GET") - - def test_get_locked(self): - self.check_locked("GET") - - def test_post_not_locked(self): - self.check_call_wrapper_without_lock("POST") - - def test_post_locked(self): - self.check_locked("POST") From d10d0afadfd007031b8742158b8d0754cc1093b0 Mon Sep 17 00:00:00 2001 From: Peter Romancik Date: Thu, 30 Apr 2026 14:54:58 +0200 Subject: [PATCH 3/8] remove unused Ruby code --- pcsd/cfgsync.rb | 41 ----------------------------------------- pcsd/remote.rb | 46 ---------------------------------------------- 2 files changed, 87 deletions(-) diff --git a/pcsd/cfgsync.rb b/pcsd/cfgsync.rb index b3c724187..69c75c800 100644 --- a/pcsd/cfgsync.rb +++ b/pcsd/cfgsync.rb @@ -277,47 +277,6 @@ def self.cluster_cfg_class() return CorosyncConf end - def self.get_cfg_classes() - return [PcsdSettings, PcsdKnownHosts] - # return [PcsdSettings, self.cluster_cfg_class] - end - - def self.get_cfg_classes_by_name() - classes = {} - self.get_cfg_classes.each { |cfgclass| - classes[cfgclass.name] = cfgclass - } - return classes - end - - def self.sync_msg_to_configs(sync_msg) - cfg_classes = self.get_cfg_classes_by_name - configs = {} - unknown_config_names = [] - sync_msg['configs'].each { |name, data| - if cfg_classes[name] - if 'file' == data['type'] and data['text'] - configs[name] = cfg_classes[name].from_text(data['text']) - end - else - unknown_config_names << name - end - } - return configs, unknown_config_names - end - - def self.get_configs_local(with_missing=false) - default = with_missing ? '' : nil - configs = {} - self.get_cfg_classes.each { |cfg_class| - begin - configs[cfg_class.name] = cfg_class.from_file(default) - rescue - end - } - return configs - end - def self.get_integer_value(value, default, minimum) return default if value.nil? if value.respond_to?(:match) diff --git a/pcsd/remote.rb b/pcsd/remote.rb index 689cfe4d5..cd937dc00 100644 --- a/pcsd/remote.rb +++ b/pcsd/remote.rb @@ -27,7 +27,6 @@ def remote(params, request, auth_user) :get_quorum_info => method(:get_quorum_info), :get_corosync_conf => method(:get_corosync_conf_remote), :set_corosync_conf => method(:set_corosync_conf), - :set_configs => method(:set_configs), :set_certs => method(:set_certs), :get_permissions => method(:get_permissions_remote), :cluster_start => method(:cluster_start), @@ -414,51 +413,6 @@ def set_corosync_conf(params, request, auth_user) end end -def set_configs(params, request, auth_user) - if not allowed_for_local_cluster(auth_user, Permissions::FULL) - return 403, 'Permission denied' - end - return JSON.generate({'status' => 'bad_json'}) if not params['configs'] - begin - configs_json = JSON.parse(params['configs']) - rescue JSON::ParserError - return JSON.generate({'status' => 'bad_json'}) - end - has_cluster = !($cluster_name == nil or $cluster_name.empty?) - if has_cluster and $cluster_name != configs_json['cluster_name'] - return JSON.generate({'status' => 'wrong_cluster_name'}) - end - - force = configs_json['force'] - remote_configs, unknown_cfg_names = Cfgsync::sync_msg_to_configs(configs_json) - local_configs = Cfgsync::get_configs_local - - result = {} - unknown_cfg_names.each { |name| result[name] = 'not_supported' } - remote_configs.each { |name, remote_cfg| - begin - # Save a remote config if it is a newer version than local. If the config - # is not present on a local node, the node is being added to a cluster, - # so we need to save the config as well. - if force or not local_configs.key?(name) or remote_cfg > local_configs[name] - local_configs[name].class.backup() if local_configs.key?(name) - remote_cfg.save() - result[name] = 'accepted' - elsif remote_cfg == local_configs[name] - # Someone wants this node to have a config that it already has. - # So the desired state is met and the result is a success then. - result[name] = 'accepted' - else - result[name] = 'rejected' - end - rescue => e - $logger.error("Error saving config '#{name}': #{e}") - result[name] = 'error' - end - } - return JSON.generate({'status' => 'ok', 'result' => result}) -end - def set_certs(params, request, auth_user) if not allowed_for_local_cluster(auth_user, Permissions::FULL) return 403, 'Permission denied' From 0b4e9407e1d04b6ba02c2b1f50381d7eb3f5ce2a Mon Sep 17 00:00:00 2001 From: Peter Romancik Date: Thu, 30 Apr 2026 18:12:15 +0200 Subject: [PATCH 4/8] update docstring --- pcs/lib/commands/pcs_cfgsync.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pcs/lib/commands/pcs_cfgsync.py b/pcs/lib/commands/pcs_cfgsync.py index 145b17b8d..2a2c3fdbb 100644 --- a/pcs/lib/commands/pcs_cfgsync.py +++ b/pcs/lib/commands/pcs_cfgsync.py @@ -30,8 +30,8 @@ def get_configs(env: LibraryEnvironment, cluster_name: str) -> SyncConfigsDto: """ Get contents of synced configuration files from node - cluster_name -- expected cluster name. End with an error if the requested - node is not in the cluster with the expected name. + cluster_name -- expected cluster name. End with an error if the local + cluster name does not match cluster_name. """ current_cluster_name = env.get_corosync_conf().get_cluster_name() if current_cluster_name != cluster_name: @@ -71,8 +71,8 @@ def set_configs( """ Save configuration files locally. - cluster_name -- expected cluster name. End with an error if the requested - node is not in the cluster with the expected name. + cluster_name -- expected cluster name. End with an error if the local + cluster name does not match cluster_name. configs -- contents of files to be saved force_flags -- list of flags codes """ From 28b3fa1b1cf42dafc89199427aab110685369988 Mon Sep 17 00:00:00 2001 From: Peter Romancik Date: Mon, 4 May 2026 09:31:39 +0200 Subject: [PATCH 5/8] do not report file path while parsing remote file --- pcs/lib/commands/pcs_cfgsync.py | 8 +++++++- pcs_test/tier0/lib/commands/test_pcs_cfgsync.py | 4 ++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/pcs/lib/commands/pcs_cfgsync.py b/pcs/lib/commands/pcs_cfgsync.py index 2a2c3fdbb..83e50818f 100644 --- a/pcs/lib/commands/pcs_cfgsync.py +++ b/pcs/lib/commands/pcs_cfgsync.py @@ -117,7 +117,13 @@ def set_configs( ) except ParserErrorException as e: env.report_processor.report_list( - file_instance.parser_exception_to_report_list(e) + file_instance.toolbox.parser.exception_to_report_list( + e, + file_type, + None, + force_code=None, + is_forced_or_warning=False, + ) + [ reports.ReportItem.error( reports.messages.PcsCfgsyncConfigSaveError(file_type) diff --git a/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py b/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py index 401c5ac6e..8775b5eb9 100644 --- a/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py +++ b/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py @@ -841,7 +841,7 @@ def test_remote_parse_error(self): "Expecting property name enclosed in double quotes: " "line 1 column 2 (char 1)" ), - file_path=settings.pcsd_known_hosts_location, + file_path=None, ), fixture.error( reports.codes.PCS_CFGSYNC_CONFIG_SAVE_ERROR, @@ -1118,7 +1118,7 @@ def test_multiple_files(self): "Expecting property name enclosed in double quotes: " "line 1 column 2 (char 1)" ), - file_path=settings.pcsd_known_hosts_location, + file_path=None, ), fixture.warn( reports.codes.PCS_CFGSYNC_CONFIG_REJECTED, From 55eb9db99f1014e74c429400849537434182a0be Mon Sep 17 00:00:00 2001 From: Peter Romancik Date: Wed, 6 May 2026 16:39:15 +0200 Subject: [PATCH 6/8] improve comments and docstring --- pcs/lib/commands/pcs_cfgsync.py | 10 +++++----- pcs_test/tier0/daemon/app/test_api_v0.py | 12 ++++++++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/pcs/lib/commands/pcs_cfgsync.py b/pcs/lib/commands/pcs_cfgsync.py index 83e50818f..79c355b22 100644 --- a/pcs/lib/commands/pcs_cfgsync.py +++ b/pcs/lib/commands/pcs_cfgsync.py @@ -74,12 +74,12 @@ def set_configs( cluster_name -- expected cluster name. End with an error if the local cluster name does not match cluster_name. configs -- contents of files to be saved - force_flags -- list of flags codes + force_flags -- list of force flag codes """ - # the node is either in cluster (has_corosync_conf) and the cluster names - # must match, or the node is not in cluster and there is no cluster name to - # check against - this happens when this command is used on a node that is - # being added to a cluster. + # the node is either in a cluster (has_corosync_conf) and the cluster names + # must match, or the node is not in a cluster and there is no cluster name + # to check against - this happens when this command is used on a node that + # is being added to a cluster. if env.has_corosync_conf: local_cluster_name = env.get_corosync_conf().get_cluster_name() if local_cluster_name != cluster_name: diff --git a/pcs_test/tier0/daemon/app/test_api_v0.py b/pcs_test/tier0/daemon/app/test_api_v0.py index 131591a34..9d1978d60 100644 --- a/pcs_test/tier0/daemon/app/test_api_v0.py +++ b/pcs_test/tier0/daemon/app/test_api_v0.py @@ -1227,7 +1227,19 @@ def test_locked(self): body=self._request_body({"cluster_name": "", "configs": {}}), ) except TornadoTimeoutError: + # The http_client timeouted because of lock and this is how we test + # the locking function. However event loop on the server side should + # finish. So we release the lock and the request successfully + # finish. self.sync_config_lock.release() + # Now, there is an unfinished request. It was started by calling + # fetch("/remote/set_configs") and it was waiting for the lock to be + # released. + # The lock was released and the request is able to be finished now. + # So, io_loop needs an opportunity to execute the rest of request. + # Next line runs io_loop to finish hanging request. Without this an + # error appears during calling + # `self.http_server.close_all_connections` in tearDown... self.io_loop.run_sync(lambda: None) else: raise AssertionError("Timeout not raised") From 6dd69d94f802b427a98e1ea08cedf3c372e1b3cc Mon Sep 17 00:00:00 2001 From: Peter Romancik Date: Wed, 6 May 2026 16:54:42 +0200 Subject: [PATCH 7/8] make backup removal errors non-fatal --- pcs/lib/commands/pcs_cfgsync.py | 14 ++++++++--- .../tier0/lib/commands/test_pcs_cfgsync.py | 23 +++++++++++-------- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/pcs/lib/commands/pcs_cfgsync.py b/pcs/lib/commands/pcs_cfgsync.py index 79c355b22..30a7aa59e 100644 --- a/pcs/lib/commands/pcs_cfgsync.py +++ b/pcs/lib/commands/pcs_cfgsync.py @@ -273,9 +273,17 @@ def __accept_and_write_file( try: if create_backup: file_instance.raw_file.backup() - file_instance.raw_file.remove_old_backups(backup_count) - # if any of the backup methods raise RawFileError, we do not - # want to try overwriting the file + # Remove old backups, but do not treat the errors as fatal so that + # the file is actually written if we were at least able to create + # the backup + try: + file_instance.raw_file.remove_old_backups(backup_count) + except RawFileError as e: + report_list.append( + raw_file_error_report(e, is_forced_or_warning=True) + ) + # if we failed to backup (raised RawFileError), we do not want to try + # overwriting the file file_instance.write_facade(file, can_overwrite=True) report_list.append( reports.ReportItem.info( diff --git a/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py b/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py index 8775b5eb9..e44c411b6 100644 --- a/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py +++ b/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py @@ -1056,24 +1056,29 @@ def test_old_backup_removal_error(self): exception_msg="Old backup removal error", name="pcs_settings.remove_old_backups", ) - self.env_assist.assert_raise_library_error( - lambda: lib.set_configs( - self.env_assist.get_env(), - self.CLUSTER_NAME, - {file_type_codes.PCS_SETTINGS_CONF: self.PCS_SETTINGS_V6}, - ) + self.config.raw_file.write( + file_type_codes.PCS_SETTINGS_CONF, + settings.pcsd_settings_conf_location, + self.PCS_SETTINGS_V6.encode(), + can_overwrite=True, + name="pcs_settings.write", + ) + lib.set_configs( + self.env_assist.get_env(), + self.CLUSTER_NAME, + {file_type_codes.PCS_SETTINGS_CONF: self.PCS_SETTINGS_V6}, ) self.env_assist.assert_reports( [ - fixture.error( + fixture.warn( reports.codes.FILE_IO_ERROR, file_type_code=file_type_codes.PCS_SETTINGS_CONF, operation="remove_backup", reason="Old backup removal error", file_path=settings.pcsd_settings_conf_location, ), - fixture.error( - reports.codes.PCS_CFGSYNC_CONFIG_SAVE_ERROR, + fixture.info( + reports.codes.PCS_CFGSYNC_CONFIG_ACCEPTED, file_type_code=file_type_codes.PCS_SETTINGS_CONF, ), ] From a236e2e25e87e2fcd738a004f73ba832291e55f1 Mon Sep 17 00:00:00 2001 From: Peter Romancik Date: Wed, 6 May 2026 16:57:22 +0200 Subject: [PATCH 8/8] test 'in-cluster' scenario more --- pcs_test/tier0/lib/commands/test_pcs_cfgsync.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py b/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py index e44c411b6..a46e87e68 100644 --- a/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py +++ b/pcs_test/tier0/lib/commands/test_pcs_cfgsync.py @@ -671,7 +671,7 @@ def test_unsupported_file_types(self): ) def test_local_file_not_exist_accepted(self): - self._fixture_not_in_cluster() + self._fixture_in_cluster() self._fixture_cfgsync_ctl_not_exists() self.config.raw_file.exists( file_type_codes.PCS_KNOWN_HOSTS, @@ -715,7 +715,7 @@ def test_same_version_and_hash_accepted_without_write(self): self.env_assist.assert_reports([self.REPORT_KNOWN_HOSTS_SAVE_SUCCESS]) def test_remote_newer_version_accepted_with_backup(self): - self._fixture_not_in_cluster() + self._fixture_in_cluster() self._fixture_cfgsync_ctl_exists() self.config.raw_file.exists( file_type_codes.PCS_SETTINGS_CONF,