From 6282aba926695c84b2037fbdaa7a33bc2f21c010 Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Mon, 16 Sep 2024 14:54:11 -0400 Subject: [PATCH 01/18] Marked PATCH session status endpoint to be ignored by the CLI --- CHANGELOG.md | 2 ++ api/openapi.yaml.in | 1 + 2 files changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 349a4275..defda346 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Changed +- Marked PATCH session status endpoint to be ignored by the CLI. ## [2.28.0] - 2024-09-11 ### Added diff --git a/api/openapi.yaml.in b/api/openapi.yaml.in index e8e989a2..5c608663 100644 --- a/api/openapi.yaml.in +++ b/api/openapi.yaml.in @@ -1567,6 +1567,7 @@ paths: tags: - v2 - sessions + - cli_ignore x-openapi-router-controller: bos.server.controllers.v2.sessions operationId: patch_v2_session requestBody: From c931cbfdf43e11be43f5b7408f90d1cb111466d4 Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Tue, 17 Sep 2024 11:25:19 -0400 Subject: [PATCH 02/18] Eliminate redundancies in API spec by defining V2SessionNameOrEmpty schema --- CHANGELOG.md | 1 + api/openapi.yaml.in | 16 +++++++--------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index defda346..ec6decfe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Changed - Marked PATCH session status endpoint to be ignored by the CLI. +- Eliminate redundancies in API spec by defining `V2SessionNameOrEmpty` schema. ## [2.28.0] - 2024-09-11 ### Added diff --git a/api/openapi.yaml.in b/api/openapi.yaml.in index 5c608663..0da76c3c 100644 --- a/api/openapi.yaml.in +++ b/api/openapi.yaml.in @@ -436,6 +436,10 @@ components: maxLength: 127 pattern: '^[a-zA-Z0-9](?:[-._a-zA-Z0-9]{0,125}[a-zA-Z0-9])?$' example: "session-20190728032600" + V2SessionNameOrEmpty: + oneOf: + - $ref: '#/components/schemas/V2SessionName' + - $ref: '#/components/schemas/EmptyString' V2SessionOperation: type: string enum: ['boot', 'reboot', 'shutdown'] @@ -762,9 +766,7 @@ components: configuration: $ref: '#/components/schemas/CfsConfiguration' session: - oneOf: - - $ref: '#/components/schemas/V2SessionName' - - $ref: '#/components/schemas/EmptyString' + $ref: '#/components/schemas/V2SessionNameOrEmpty' last_updated: $ref: '#/components/schemas/V2ComponentLastUpdated' additionalProperties: false @@ -851,9 +853,7 @@ components: description: A description of the most recent error to impact the Component. maxLength: 65536 session: - oneOf: - - $ref: '#/components/schemas/V2SessionName' - - $ref: '#/components/schemas/EmptyString' + $ref: '#/components/schemas/V2SessionNameOrEmpty' retry_policy: type: integer description: | @@ -892,9 +892,7 @@ components: description: A description of the most recent error to impact the Component. maxLength: 65536 session: - oneOf: - - $ref: '#/components/schemas/V2SessionName' - - $ref: '#/components/schemas/EmptyString' + $ref: '#/components/schemas/V2SessionNameOrEmpty' retry_policy: type: integer description: | From 89bd4ca873a3f5d68da060d61d8f6fefccd54cc2 Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Thu, 26 Sep 2024 13:07:05 -0400 Subject: [PATCH 03/18] Linting --- src/bos/common/utils.py | 6 ++--- src/bos/server/controllers/v2/boot_set.py | 30 +++++++++++------------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/bos/common/utils.py b/src/bos/common/utils.py index dfcff337..35fe4df5 100644 --- a/src/bos/common/utils.py +++ b/src/bos/common/utils.py @@ -150,14 +150,14 @@ def components_by_id(components: List[dict]) -> dict: """ return { component["id"]: component for component in components } -def reverse_components_by_id(components_by_id: dict) -> List[dict]: +def reverse_components_by_id(components_by_id_map: dict) -> List[dict]: """ Input: - components_by_id: a dictionary with the name of each component as the + components_by_id_map: a dictionary with the name of each component as the key and the value being the entire component itself. Return: A list with each component as an element Purpose: Reverse the effect of components_by_id. """ - return list(components_by_id.values()) + return list(components_by_id_map.values()) diff --git a/src/bos/server/controllers/v2/boot_set.py b/src/bos/server/controllers/v2/boot_set.py index 7854e134..2bcc2b69 100644 --- a/src/bos/server/controllers/v2/boot_set.py +++ b/src/bos/server/controllers/v2/boot_set.py @@ -22,6 +22,7 @@ # OTHER DEALINGS IN THE SOFTWARE. # +from functools import partial import logging from bos.common.utils import exc_type_msg from bos.operators.utils.boot_image_metadata.factory import BootImageMetaDataFactory @@ -39,6 +40,13 @@ HARDWARE_SPECIFIER_FIELDS = ( "node_list", "node_roles_groups", "node_groups" ) +def _bs_msg(msg: str, template_name: str, bs_name: str) -> str: + """ + Shortcut for creating validation error/warning messages for a specific bootset + """ + return f"Session template: '{template_name}' boot set: '{bs_name}': {msg}" + + def validate_boot_sets(session_template: dict, operation: str, template_name: str, @@ -74,21 +82,18 @@ def validate_boot_sets(session_template: dict, for bs_name, bs in session_template['boot_sets'].items(): warning_msgs = [] + bs_msg = partial(_bs_msg, template_name=template_name, bs_name=bs_name) # Verify that the hardware is specified specified = [bs.get(field, None) for field in HARDWARE_SPECIFIER_FIELDS] if not any(specified): - msg = f"Session template: '{template_name}' boot set: '{bs_name}' " \ - f"must have at least one non-empty" \ - f"hardware specifier field provided (%s); None were provided." \ - % (', '.join(sorted(HARDWARE_SPECIFIER_FIELDS))) + msg = bs_msg(f"No non-empty hardware specifier field {HARDWARE_SPECIFIER_FIELDS}") LOGGER.error(msg) return BOOT_SET_ERROR, msg try: if any(node[:3] == "nid" for node in bs["node_list"]): - msg = f"Session template: '{template_name}' boot set: '{bs_name}' "\ - "has NID in 'node_list'" + msg = bs_msg("Has NID in 'node_list'") if reject_nids: LOGGER.error(msg) return BOOT_SET_ERROR, msg @@ -105,8 +110,7 @@ def validate_boot_sets(session_template: dict, try: image_metadata = BootImageMetaDataFactory(bs)() except Exception as err: - msg = f"Session template: '{template_name}' boot set: '{bs_name}' " \ - f"could not locate its boot artifacts. Error: " + exc_type_msg(err) + msg = bs_msg(f"Can't locate boot artifacts. Error: {exc_type_msg(err)}") LOGGER.error(msg) return BOOT_SET_ERROR, msg @@ -119,8 +123,7 @@ def validate_boot_sets(session_template: dict, obj = S3Object(path, etag) _ = obj.object_header except Exception as err: - msg = f"Session template: '{template_name}' boot set: '{bs_name}' " \ - f"could not locate its {boot_artifact}. Error: " + exc_type_msg(err) + msg = bs_msg(f"Can't locate its {boot_artifact}. Error: {exc_type_msg(err)}") LOGGER.error(msg) return BOOT_SET_ERROR, msg @@ -128,16 +131,13 @@ def validate_boot_sets(session_template: dict, try: artifact = getattr(image_metadata.boot_artifacts, boot_artifact) if not artifact: - raise ArtifactNotFound(f"Session template: '{template_name}' " - f"boot set: '{bs_name}' " - f"does not contain a {boot_artifact}.") + raise ArtifactNotFound(f"Doesn't contain a {boot_artifact}") path = artifact ['link']['path'] etag = artifact['link']['etag'] obj = S3Object(path, etag) _ = obj.object_header except Exception as err: - msg = f"Session template: '{template_name}' boot set: '{bs_name}' " \ - f"could not locate its {boot_artifact}. Warning: " + exc_type_msg(err) + msg = bs_msg(f"Can't locate its {boot_artifact}. Warning: {exc_type_msg(err)}") LOGGER.warning(msg) warning_msgs.append(msg) if warning_msgs: From 5dabdedb80d58cb3af5417002e02c845d59be3f1 Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Thu, 26 Sep 2024 15:22:52 -0400 Subject: [PATCH 04/18] Run pylint during builds --- CHANGELOG.md | 3 +++ Dockerfile | 23 +++++++++++++++++++++-- Jenkinsfile.github | 7 ++++++- Makefile | 21 +++++++++++++++++++-- src/__init__.py => docker_pylint.sh | 5 ++++- 5 files changed, 53 insertions(+), 6 deletions(-) rename src/__init__.py => docker_pylint.sh (90%) mode change 100644 => 100755 diff --git a/CHANGELOG.md b/CHANGELOG.md index ec6decfe..c5896372 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added +- Run `pylint` during builds + ### Changed - Marked PATCH session status endpoint to be ignored by the CLI. - Eliminate redundancies in API spec by defining `V2SessionNameOrEmpty` schema. diff --git a/Dockerfile b/Dockerfile index 47bae2d8..afc281cd 100644 --- a/Dockerfile +++ b/Dockerfile @@ -79,7 +79,7 @@ RUN --mount=type=secret,id=netrc,target=/root/.netrc \ pip3 list --format freeze && \ python3 -m convert_oas30_schemas /app/openapi.json /app/lib/bos/server/openapi.jsonschema && \ cat /app/lib/bos/server/openapi.jsonschema - +RUN pylint /app/convert_oas/convert_oas.py || true # Base image FROM alpine-base AS base @@ -120,11 +120,30 @@ RUN --mount=type=secret,id=netrc,target=/root/.netrc \ FROM base AS testing WORKDIR /app COPY test-requirements.txt . -RUN --mount=type=secret,id=netrc,target=/root/.netrc cd /app && \ +RUN --mount=type=secret,id=netrc,target=/root/.netrc \ + cd /app && \ pip3 install --no-cache-dir -r test-requirements.txt && \ pip3 list --format freeze +# Pylint reporting +FROM base AS pylint-base +COPY srclist.txt docker_pylint.sh /app/venv/ +WORKDIR /app +RUN --mount=type=secret,id=netrc,target=/root/.netrc \ + pip3 install --no-cache-dir pylint -c constraints.txt && \ + pip3 list --format freeze + +# Pylint errors-only +FROM pylint-base AS pylint-errors-only +WORKDIR /app/venv +CMD [ "./docker_pylint.sh", "--errors-only" ] + +# Pylint full +FROM pylint-base AS pylint-full +WORKDIR /app/venv +CMD [ "./docker_pylint.sh", "--fail-under", "9" ] + # Codestyle reporting FROM testing AS codestyle WORKDIR /app diff --git a/Jenkinsfile.github b/Jenkinsfile.github index 8b216c4f..b5c4aa24 100644 --- a/Jenkinsfile.github +++ b/Jenkinsfile.github @@ -93,7 +93,12 @@ pipeline { } steps { echo "Docker args are ${env.DOCKER_ARGS}" - sh "make image" + sh "make image_setup" + sh "make image_build" + sh "make image_build_pylint_errors" + sh "make image_run_pylint_errors" + sh "make image_build_pylint_full" + sh "make image_run_pylint_full" } } stage('Chart') { diff --git a/Makefile b/Makefile index e6085777..35d6b8d3 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,7 @@ # # MIT License # -# (C) Copyright 2021-2023 Hewlett Packard Enterprise Development LP +# (C) Copyright 2021-2024 Hewlett Packard Enterprise Development LP # # Permission is hereby granted, free of charge, to any person obtaining a # copy of this software and associated documentation files (the "Software"), @@ -55,6 +55,7 @@ RPTR_SOURCE_PATH := ${RPTR_BUILD_DIR}/SOURCES/${RPTR_SOURCE_NAME}.tar.bz2 all : runbuildprep lint image chart rptr_rpm local: cms_meta_tools runbuildprep image chart_setup chart_package chart: chart_setup chart_package chart_test +image: image_setup image_build image_build_pylint_errors image_run_pylint_errors image_build_pylint_full image_run_pylint_full rptr_rpm: rptr_rpm_package_source rptr_rpm_build_source rptr_rpm_build clone_input_files: @@ -90,9 +91,25 @@ rptr_rpm_prepare: cp $(RPTR_SPEC_FILE) $(RPTR_BUILD_DIR)/SPECS/ cat $(RPTR_SPEC_FILE) $(RPTR_BUILD_DIR)/SPECS/bos-reporter.spec -image: +image_setup: + # Create list of BOS Python source files, to be checked later by pylint + find src/bos -type f -name \*.py -print | sed 's#^src/#/app/lib/#' | tr '\n' ' ' | tee srclist.txt + +image_build: docker build --pull ${DOCKER_ARGS} --tag '${NAME}:${DOCKER_VERSION}' . +image_build_pylint_errors: + docker build --pull ${DOCKER_ARGS} --target pylint-errors-only --tag 'pylint-errors-only:${DOCKER_VERSION}' . + +image_run_pylint_errors: + docker run --rm 'pylint-errors-only:${DOCKER_VERSION}' + +image_build_pylint_full: + docker build --pull ${DOCKER_ARGS} --target pylint-full --tag 'pylint-full:${DOCKER_VERSION}' . + +image_run_pylint_full: + docker run --rm 'pylint-full:${DOCKER_VERSION}' + chart_package: helm dep up ${CHART_PATH}/${NAME} helm package ${CHART_PATH}/${NAME} -d ${CHART_PATH}/.packaged --app-version ${DOCKER_VERSION} --version ${CHART_VERSION} diff --git a/src/__init__.py b/docker_pylint.sh old mode 100644 new mode 100755 similarity index 90% rename from src/__init__.py rename to docker_pylint.sh index f4345184..899f09a4 --- a/src/__init__.py +++ b/docker_pylint.sh @@ -1,7 +1,8 @@ +#!/usr/bin/env sh # # MIT License # -# (C) Copyright 2021-2022 Hewlett Packard Enterprise Development LP +# (C) Copyright 2024 Hewlett Packard Enterprise Development LP # # Permission is hereby granted, free of charge, to any person obtaining a # copy of this software and associated documentation files (the "Software"), @@ -21,3 +22,5 @@ # ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR # OTHER DEALINGS IN THE SOFTWARE. # + +pylint "$@" $(cat ./srclist.txt) From 3da11e681fa14a0d27876f4b086481fc508de6ee Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Thu, 26 Sep 2024 15:31:12 -0400 Subject: [PATCH 05/18] Linting --- src/bos/operators/status.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/bos/operators/status.py b/src/bos/operators/status.py index e60ffd7a..83980f07 100644 --- a/src/bos/operators/status.py +++ b/src/bos/operators/status.py @@ -123,6 +123,7 @@ def _check_status(self, component, power_state, cfs_component): Calculate the component's current status based upon its power state and CFS configuration state. If its status differs from the status in the database, return this information. """ + error = None if power_state and cfs_component: phase, override, disable, error, action_failed = self._calculate_status(component, power_state, From a1a9539b0a6d81e2c9ef6040e730bfd95ec40129 Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Thu, 26 Sep 2024 16:20:23 -0400 Subject: [PATCH 06/18] CASMCMS-9143: When validating boot sets, check all boot sets for severe errors before returning only warnings --- CHANGELOG.md | 3 +++ src/bos/server/controllers/v2/boot_set.py | 9 +++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5896372..de12c11c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Marked PATCH session status endpoint to be ignored by the CLI. - Eliminate redundancies in API spec by defining `V2SessionNameOrEmpty` schema. +### Fixed +- When validating boot sets, check all boot sets for severe errors before returning only warnings + ## [2.28.0] - 2024-09-11 ### Added - Added `reject_nids` BOS option, to reject Sessions and Session Template which appear to reference NIDs. diff --git a/src/bos/server/controllers/v2/boot_set.py b/src/bos/server/controllers/v2/boot_set.py index 2bcc2b69..aabf95bb 100644 --- a/src/bos/server/controllers/v2/boot_set.py +++ b/src/bos/server/controllers/v2/boot_set.py @@ -80,8 +80,8 @@ def validate_boot_sets(session_template: dict, if reject_nids is None: reject_nids = get_v2_options_data().get('reject_nids', False) - for bs_name, bs in session_template['boot_sets'].items(): - warning_msgs = [] + warning_msgs = [] + for bs_name, bs in session_template['boot_sets'].items(): bs_msg = partial(_bs_msg, template_name=template_name, bs_name=bs_name) # Verify that the hardware is specified @@ -140,8 +140,9 @@ def validate_boot_sets(session_template: dict, msg = bs_msg(f"Can't locate its {boot_artifact}. Warning: {exc_type_msg(err)}") LOGGER.warning(msg) warning_msgs.append(msg) - if warning_msgs: - return BOOT_SET_WARNING, "; ".join(warning_msgs) + + if warning_msgs: + return BOOT_SET_WARNING, "; ".join(warning_msgs) return BOOT_SET_SUCCESS, "Valid" From fed1aa07388ee26c00e1a69da1f9e41c037173b5 Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Fri, 27 Sep 2024 12:54:03 -0400 Subject: [PATCH 07/18] Refactor validate_boot_sets into multiple functions to make it easier to understand --- CHANGELOG.md | 1 + src/bos/server/controllers/v2/boot_set.py | 142 ++++++++++++---------- 2 files changed, 81 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de12c11c..24ed4071 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Marked PATCH session status endpoint to be ignored by the CLI. - Eliminate redundancies in API spec by defining `V2SessionNameOrEmpty` schema. +- Refactor `validate_boot_sets` function into multiple functions to make it easier to understand. ### Fixed - When validating boot sets, check all boot sets for severe errors before returning only warnings diff --git a/src/bos/server/controllers/v2/boot_set.py b/src/bos/server/controllers/v2/boot_set.py index aabf95bb..598cede4 100644 --- a/src/bos/server/controllers/v2/boot_set.py +++ b/src/bos/server/controllers/v2/boot_set.py @@ -39,12 +39,10 @@ # Valid boot sets are required to have at least one of these fields HARDWARE_SPECIFIER_FIELDS = ( "node_list", "node_roles_groups", "node_groups" ) - -def _bs_msg(msg: str, template_name: str, bs_name: str) -> str: +class BootSetError(Exception): """ - Shortcut for creating validation error/warning messages for a specific bootset + Generic error class for fatal problems found during boot set validation """ - return f"Session template: '{template_name}' boot set: '{bs_name}': {msg}" def validate_boot_sets(session_template: dict, @@ -69,8 +67,6 @@ def validate_boot_sets(session_template: dict, 0 -- Success 1 -- Warning, not fatal 2 -- Error, fatal - - """ # Verify boot sets exist. if not session_template.get('boot_sets', None): @@ -81,65 +77,22 @@ def validate_boot_sets(session_template: dict, reject_nids = get_v2_options_data().get('reject_nids', False) warning_msgs = [] - for bs_name, bs in session_template['boot_sets'].items(): + for bs_name, bs in session_template['boot_sets'].items(): bs_msg = partial(_bs_msg, template_name=template_name, bs_name=bs_name) - - # Verify that the hardware is specified - specified = [bs.get(field, None) - for field in HARDWARE_SPECIFIER_FIELDS] - if not any(specified): - msg = bs_msg(f"No non-empty hardware specifier field {HARDWARE_SPECIFIER_FIELDS}") + try: + bs_warning_msgs = _validate_boot_set(bs=bs, operation=operation, + reject_nids=reject_nids) + except BootSetError as err: + msg = bs_msg(str(err)) LOGGER.error(msg) return BOOT_SET_ERROR, msg - try: - if any(node[:3] == "nid" for node in bs["node_list"]): - msg = bs_msg("Has NID in 'node_list'") - if reject_nids: - LOGGER.error(msg) - return BOOT_SET_ERROR, msg - # Otherwise, log this as a warning -- even if reject_nids is not set, - # BOS still doesn't support NIDs, so this is still undesirable - LOGGER.warning(msg) - warning_msgs.append(msg) - except KeyError: - # If there is no node_list field, not a problem - pass - - if operation in ['boot', 'reboot']: - # Verify that the boot artifacts exist - try: - image_metadata = BootImageMetaDataFactory(bs)() - except Exception as err: - msg = bs_msg(f"Can't locate boot artifacts. Error: {exc_type_msg(err)}") - LOGGER.error(msg) - return BOOT_SET_ERROR, msg - - # Check boot artifacts' S3 headers - for boot_artifact in ["kernel"]: - try: - artifact = getattr(image_metadata.boot_artifacts, boot_artifact) - path = artifact ['link']['path'] - etag = artifact['link']['etag'] - obj = S3Object(path, etag) - _ = obj.object_header - except Exception as err: - msg = bs_msg(f"Can't locate its {boot_artifact}. Error: {exc_type_msg(err)}") - LOGGER.error(msg) - return BOOT_SET_ERROR, msg - - for boot_artifact in ["initrd", "boot_parameters"]: - try: - artifact = getattr(image_metadata.boot_artifacts, boot_artifact) - if not artifact: - raise ArtifactNotFound(f"Doesn't contain a {boot_artifact}") - path = artifact ['link']['path'] - etag = artifact['link']['etag'] - obj = S3Object(path, etag) - _ = obj.object_header - except Exception as err: - msg = bs_msg(f"Can't locate its {boot_artifact}. Warning: {exc_type_msg(err)}") - LOGGER.warning(msg) - warning_msgs.append(msg) + except Exception as err: + LOGGER.error( + bs_msg(f"Unexpected exception in _validate_boot_set: {exc_type_msg(err)}")) + raise + for msg in map(bs_msg, bs_warning_msgs): + LOGGER.warning(msg) + warning_msgs.append(msg) if warning_msgs: return BOOT_SET_WARNING, "; ".join(warning_msgs) @@ -147,6 +100,71 @@ def validate_boot_sets(session_template: dict, return BOOT_SET_SUCCESS, "Valid" +def _bs_msg(msg: str, template_name: str, bs_name: str) -> str: + """ + Shortcut for creating validation error/warning messages for a specific bootset + """ + return f"Session template: '{template_name}' boot set: '{bs_name}': {msg}" + + +def _validate_boot_set(bs: dict, operation: str, reject_nids: bool) -> list[str]: + """ + Helper function for validate_boot_sets that performs validation on a single boot set. + Raises BootSetError if fatal errors found. + Returns a list of warning messages (if any) + """ + warning_msgs = [] + + # Verify that the hardware is specified + specified = [bs.get(field, None) + for field in HARDWARE_SPECIFIER_FIELDS] + if not any(specified): + raise BootSetError(f"No non-empty hardware specifier field {HARDWARE_SPECIFIER_FIELDS}") + try: + if any(node[:3] == "nid" for node in bs["node_list"]): + msg = "Has NID in 'node_list'" + if reject_nids: + raise BootSetError(msg) + # Otherwise, log this as a warning -- even if reject_nids is not set, + # BOS still doesn't support NIDs, so this is still undesirable + warning_msgs.append(msg) + except KeyError: + # If there is no node_list field, not a problem + pass + + if operation in ['boot', 'reboot']: + # Verify that the boot artifacts exist + try: + image_metadata = BootImageMetaDataFactory(bs)() + except Exception as err: + raise BootSetError(f"Can't find boot artifacts. Error: {exc_type_msg(err)}") from err + + # Check boot artifacts' S3 headers + for boot_artifact in ["kernel"]: + try: + artifact = getattr(image_metadata.boot_artifacts, boot_artifact) + path = artifact ['link']['path'] + etag = artifact['link']['etag'] + obj = S3Object(path, etag) + _ = obj.object_header + except Exception as err: + raise BootSetError( + f"Can't find {boot_artifact}. Error: {exc_type_msg(err)}") from err + + for boot_artifact in ["initrd", "boot_parameters"]: + try: + artifact = getattr(image_metadata.boot_artifacts, boot_artifact) + if not artifact: + raise ArtifactNotFound(f"Doesn't contain a {boot_artifact}") + path = artifact ['link']['path'] + etag = artifact['link']['etag'] + obj = S3Object(path, etag) + _ = obj.object_header + except Exception as err: + warning_msgs.append(f"Can't find {boot_artifact}. Warning: {exc_type_msg(err)}") + + return warning_msgs + def validate_sanitize_boot_sets(template_data: dict) -> None: """ Calls validate_sanitize_boot_set on every boot set in the template. From 16cc2f6f05fbf3ce272cf8a0faa8a5e06f5dbc1a Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Fri, 27 Sep 2024 14:21:23 -0400 Subject: [PATCH 08/18] When reporting a missing boot artifact from a manifest, include the S3 URL in the message --- CHANGELOG.md | 2 ++ .../boot_image_metadata/s3_boot_image_metadata.py | 9 ++++++++- src/bos/server/controllers/v2/boot_set.py | 14 ++++++++++---- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24ed4071..50f80b24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added - Run `pylint` during builds +- When validating boot sets, if a boot artifact is missing from a manifest, include the manifest S3 URL + in the associated message. ### Changed - Marked PATCH session status endpoint to be ignored by the CLI. diff --git a/src/bos/operators/utils/boot_image_metadata/s3_boot_image_metadata.py b/src/bos/operators/utils/boot_image_metadata/s3_boot_image_metadata.py index a0ad8f28..490af3bf 100644 --- a/src/bos/operators/utils/boot_image_metadata/s3_boot_image_metadata.py +++ b/src/bos/operators/utils/boot_image_metadata/s3_boot_image_metadata.py @@ -27,7 +27,7 @@ from bos.common.utils import exc_type_msg from bos.operators.utils.boot_image_metadata import BootImageMetaData, BootImageMetaDataBadRead -from bos.operators.utils.clients.s3 import S3BootArtifacts, S3MissingConfiguration, \ +from bos.operators.utils.clients.s3 import S3BootArtifacts, S3MissingConfiguration, S3Url, \ ArtifactNotFound LOGGER = logging.getLogger('bos.operators.utils.boot_image_metadata.s3_boot_image_metadata') @@ -197,3 +197,10 @@ def boot_parameters_etag(self): if bp: return bp['link']['etag'] return None + + @property + def manifest_s3_url(self) -> S3Url: + """ + Returns the S3 URL to the boot manifest + """ + return self.boot_artifacts.s3url diff --git a/src/bos/server/controllers/v2/boot_set.py b/src/bos/server/controllers/v2/boot_set.py index 598cede4..d0c9aac9 100644 --- a/src/bos/server/controllers/v2/boot_set.py +++ b/src/bos/server/controllers/v2/boot_set.py @@ -148,20 +148,26 @@ def _validate_boot_set(bs: dict, operation: str, reject_nids: bool) -> list[str] obj = S3Object(path, etag) _ = obj.object_header except Exception as err: - raise BootSetError( - f"Can't find {boot_artifact}. Error: {exc_type_msg(err)}") from err + raise BootSetError(f"Can't find {boot_artifact} in " + f"{image_metadata.manifest_s3_url.url}. " + f"Error: {exc_type_msg(err)}") from err for boot_artifact in ["initrd", "boot_parameters"]: try: artifact = getattr(image_metadata.boot_artifacts, boot_artifact) if not artifact: - raise ArtifactNotFound(f"Doesn't contain a {boot_artifact}") + raise ArtifactNotFound() path = artifact ['link']['path'] etag = artifact['link']['etag'] obj = S3Object(path, etag) _ = obj.object_header + except ArtifactNotFound as err: + warning_msgs.append( + f"{image_metadata.manifest_s3_url.url} doesn't contain a {boot_artifact}") except Exception as err: - warning_msgs.append(f"Can't find {boot_artifact}. Warning: {exc_type_msg(err)}") + warning_msgs.append(f"Can't find {boot_artifact} in " + f"{image_metadata.manifest_s3_url.url}. " + f"Warning: {exc_type_msg(err)}") return warning_msgs From 4f85f1647e220f1f5567714ba251ea95ef577416 Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Fri, 27 Sep 2024 13:37:42 -0400 Subject: [PATCH 09/18] Add check that at least one hardware-specifier field is non-empty to validate_sanitize_boot_sets --- CHANGELOG.md | 1 + src/bos/server/controllers/v2/boot_set.py | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50f80b24..d4db098d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Run `pylint` during builds - When validating boot sets, if a boot artifact is missing from a manifest, include the manifest S3 URL in the associated message. +- Add check that at least one hardware-specifier field is non-empty to `validate_sanitize_boot_sets` ### Changed - Marked PATCH session status endpoint to be ignored by the CLI. diff --git a/src/bos/server/controllers/v2/boot_set.py b/src/bos/server/controllers/v2/boot_set.py index d0c9aac9..abf56ebb 100644 --- a/src/bos/server/controllers/v2/boot_set.py +++ b/src/bos/server/controllers/v2/boot_set.py @@ -218,6 +218,11 @@ def validate_sanitize_boot_set(bs_name: str, bs_data: dict, reject_nids: bool=Fa raise ParsingException(f"Boot set {bs_name} has none of the following " f"fields: {HARDWARE_SPECIFIER_FIELDS}") + # Validate that at least one of the HARDWARE_SPECIFIER_FIELDS is non-empty + if not any(field_name in bs_data and bs_data[field_name] for field_name in HARDWARE_SPECIFIER_FIELDS): + raise ParsingException(f"Boot set {bs_name} has no non-empty hardware-specifier fields: " + f"{HARDWARE_SPECIFIER_FIELDS}") + # Last thing to do is validate/sanitize the node_list field, if it is present try: node_list = bs_data["node_list"] From 497cf6432a96b57f60f48c0784fead1b1af477ae Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Fri, 27 Sep 2024 14:35:16 -0400 Subject: [PATCH 10/18] Minor improvements to exceptions/log messages in IMS client --- src/bos/operators/utils/clients/ims.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/bos/operators/utils/clients/ims.py b/src/bos/operators/utils/clients/ims.py index d192aec0..411db940 100644 --- a/src/bos/operators/utils/clients/ims.py +++ b/src/bos/operators/utils/clients/ims.py @@ -35,25 +35,39 @@ LOGGER = logging.getLogger('bos.operators.utils.clients.ims') IMS_TAG_OPERATIONS = ['set', 'remove'] + class TagFailure(Exception): pass + +class ImageNotFound(Exception): + """ + Raised if querying IMS for an image and it is not found + """ + def __init__(self, image_id: str): + super().__init__(self, f"IMS image id '{image_id}' does not exist in IMS") + + def patch_image(image_id, data, session=None): if not data: LOGGER.warning("patch_image called without data; returning without action.") return if not session: session = requests_retry_session() - LOGGER.debug("PATCH %s with body=%s", IMAGES_ENDPOINT, data) - response = session.patch(f"{IMAGES_ENDPOINT}/{image_id}", json=data) + url=f"{IMAGES_ENDPOINT}/{image_id}" + LOGGER.debug("PATCH %s with body=%s", url, data) + response = session.patch(url, json=data) LOGGER.debug("Response status code=%d, reason=%s, body=%s", response.status_code, response.reason, compact_response_text(response.text)) try: response.raise_for_status() except HTTPError as err: - LOGGER.error("Failed asking IMS to tag image: %s", exc_type_msg(err)) + LOGGER.error("Failed asking IMS to patch image %s: %s", image_id, exc_type_msg(err)) + if response.status_code == 404: + raise ImageNotFound(image_id) from err raise + def tag_image(image_id: str, operation: str, key: str, value: str = None, session=None) -> None: if operation not in IMS_TAG_OPERATIONS: msg = f"{operation} not valid. Expecting one of {IMS_TAG_OPERATIONS}" From 88da1837c3848801a513c6209bc8c454f23ded4b Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Fri, 27 Sep 2024 14:55:04 -0400 Subject: [PATCH 11/18] Linting --- src/bos/operators/utils/clients/ims.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bos/operators/utils/clients/ims.py b/src/bos/operators/utils/clients/ims.py index 411db940..fc20a471 100644 --- a/src/bos/operators/utils/clients/ims.py +++ b/src/bos/operators/utils/clients/ims.py @@ -45,7 +45,7 @@ class ImageNotFound(Exception): Raised if querying IMS for an image and it is not found """ def __init__(self, image_id: str): - super().__init__(self, f"IMS image id '{image_id}' does not exist in IMS") + super().__init__(f"IMS image id '{image_id}' does not exist in IMS") def patch_image(image_id, data, session=None): From 026d9423dd855645c0d368ebea8d72b2b3a07335 Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Fri, 27 Sep 2024 15:13:15 -0400 Subject: [PATCH 12/18] CASMCMS-8425: Include arch in boot set validation --- CHANGELOG.md | 3 + .../utils/boot_image_metadata/__init__.py | 6 ++ .../s3_boot_image_metadata.py | 39 ++++++++++ src/bos/operators/utils/clients/ims.py | 50 +++++++++++++ src/bos/server/controllers/v2/boot_set.py | 72 +++++++++++++++++++ 5 files changed, 170 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4db098d..f17374f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - When validating boot sets, if a boot artifact is missing from a manifest, include the manifest S3 URL in the associated message. - Add check that at least one hardware-specifier field is non-empty to `validate_sanitize_boot_sets` +- When validating boot sets (at session template patch/creation/validation or session creation), + attempt to validate that the specified architecture matches that of the corresponding IMS image. + Mismatches are fatal errors. ### Changed - Marked PATCH session status endpoint to be ignored by the CLI. diff --git a/src/bos/operators/utils/boot_image_metadata/__init__.py b/src/bos/operators/utils/boot_image_metadata/__init__.py index c27e84a4..6ec154f5 100644 --- a/src/bos/operators/utils/boot_image_metadata/__init__.py +++ b/src/bos/operators/utils/boot_image_metadata/__init__.py @@ -66,6 +66,12 @@ def rootfs(self): """ return None + @property + def arch(self): + """ + Get the arch + """ + return None class BootImageMetaDataBadRead(Exception): """ diff --git a/src/bos/operators/utils/boot_image_metadata/s3_boot_image_metadata.py b/src/bos/operators/utils/boot_image_metadata/s3_boot_image_metadata.py index 490af3bf..f7869078 100644 --- a/src/bos/operators/utils/boot_image_metadata/s3_boot_image_metadata.py +++ b/src/bos/operators/utils/boot_image_metadata/s3_boot_image_metadata.py @@ -27,6 +27,7 @@ from bos.common.utils import exc_type_msg from bos.operators.utils.boot_image_metadata import BootImageMetaData, BootImageMetaDataBadRead +from bos.operators.utils.clients.ims import get_image, get_ims_id_from_s3_url, ImageNotFound from bos.operators.utils.clients.s3 import S3BootArtifacts, S3MissingConfiguration, S3Url, \ ArtifactNotFound @@ -204,3 +205,41 @@ def manifest_s3_url(self) -> S3Url: Returns the S3 URL to the boot manifest """ return self.boot_artifacts.s3url + + @property + def arch(self): + """ + Extract the IMS image ID from the S3 manifest path. + Query IMS to get the architecture of the image. + Return the 'arch' field from the IMS image + If image is not in IMS, or 'arch' field not set, log warnings and return None. + (since technically BOS does not require the images to be in IMS) + """ + s3_url = self.manifest_s3_url + ims_id = get_ims_id_from_s3_url(s3_url) + if not ims_id: + LOGGER.warning( + "Boot artifact S3 URL '%s' does not follow the expected IMS image convention", + s3_url) + return None + try: + ims_image_data = get_image(ims_id) + except ImageNotFound: + LOGGER.warning( + "Can't determine architecture of '%s' because image '%s' does not exist in IMS", + s3_url.url, ims_id) + return None + except Exception as err: + LOGGER.error("Error getting IMS image data for '%s' (S3 path '%s'): %s", ims_id, + s3_url.url, exc_type_msg(err)) + return None + try: + return ims_image_data["arch"] + except KeyError: + LOGGER.warning("Can't determine architecture of '%s' because 'arch' field not set in " + "IMS image '%s': %s", s3_url.url, ims_id, ims_image_data) + except Exception as err: + LOGGER.error("IMS image '%s' (s3 path '%s'): %s", ims_id, s3_url.url, ims_image_data) + LOGGER.error("Error getting 'arch' field for IMS image '%s': %s", ims_id, + exc_type_msg(err)) + return None diff --git a/src/bos/operators/utils/clients/ims.py b/src/bos/operators/utils/clients/ims.py index fc20a471..484d9818 100644 --- a/src/bos/operators/utils/clients/ims.py +++ b/src/bos/operators/utils/clients/ims.py @@ -23,9 +23,11 @@ # import logging +import re from requests.exceptions import HTTPError from bos.common.utils import compact_response_text, exc_type_msg, requests_retry_session, PROTOCOL +from bos.operators.utils.clients.s3 import S3Url SERVICE_NAME = 'cray-ims' IMS_VERSION = 'v3' @@ -35,6 +37,11 @@ LOGGER = logging.getLogger('bos.operators.utils.clients.ims') IMS_TAG_OPERATIONS = ['set', 'remove'] +# Making minimal assumptions about the IMS ID itself, this pattern just makes sure that the +# S3 key is some string, then a /, then at least one more character. +IMS_S3_KEY_RE = r'^([^/]+)/.+' +IMS_S3_KEY_RE_PROG = re.compile(IMS_S3_KEY_RE) + class TagFailure(Exception): pass @@ -48,6 +55,38 @@ def __init__(self, image_id: str): super().__init__(f"IMS image id '{image_id}' does not exist in IMS") +def get_image(image_id, session=None) -> dict: + """ + Queries IMS to retrieve the specified image and return it. + If the image does not exist, raise ImageNotFound. + Other errors (like a failure to query IMS) will result in appropriate exceptions being raised. + """ + if not session: + session = requests_retry_session() + url=f"{IMAGES_ENDPOINT}/{image_id}" + LOGGER.debug("GET %s", url) + response = session.get(url) + LOGGER.debug("Response status code=%d, reason=%s, body=%s", response.status_code, + response.reason, compact_response_text(response.text)) + try: + response.raise_for_status() + except HTTPError as err: + msg = f"Failed asking IMS to get image {image_id}: {exc_type_msg(err)}" + if response.status_code == 404: + # If it's not found, we just log it as a warning, because we may be + # okay with that -- that will be for the caller to decide + LOGGER.warning(msg) + raise ImageNotFound(image_id) from err + LOGGER.error(msg) + raise + try: + return response.json() + except Exception as err: + LOGGER.error("Failed decoding JSON response from getting IMS image %s: %s", image_id, + exc_type_msg(err)) + raise + + def patch_image(image_id, data, session=None): if not data: LOGGER.warning("patch_image called without data; returning without action.") @@ -95,3 +134,14 @@ def tag_image(image_id: str, operation: str, key: str, value: str = None, sessio } } patch_image(image_id=image_id, data=data, session=session) + + +def get_ims_id_from_s3_url(s3_url: S3Url) -> str|None: + """ + If the s3_url matches the expected format of an IMS image path, then return the IMS image ID. + Otherwise return None. + """ + try: + return IMS_S3_KEY_RE_PROG.match(s3_url.key).group(1) + except (AttributeError, IndexError): + return None diff --git a/src/bos/server/controllers/v2/boot_set.py b/src/bos/server/controllers/v2/boot_set.py index abf56ebb..d1e41200 100644 --- a/src/bos/server/controllers/v2/boot_set.py +++ b/src/bos/server/controllers/v2/boot_set.py @@ -25,6 +25,7 @@ from functools import partial import logging from bos.common.utils import exc_type_msg +from bos.operators.utils.boot_image_metadata import BootImageMetaData from bos.operators.utils.boot_image_metadata.factory import BootImageMetaDataFactory from bos.operators.utils.clients.s3 import S3Object, ArtifactNotFound from bos.server.controllers.v2.options import get_v2_options_data @@ -39,12 +40,34 @@ # Valid boot sets are required to have at least one of these fields HARDWARE_SPECIFIER_FIELDS = ( "node_list", "node_roles_groups", "node_groups" ) +DEFAULT_ARCH = "X86" + +# Mapping from BOS boot set arch values to expected IMS image arch values +# Omits BOS Other value, since there is no corresponding IMS image arch value +EXPECTED_IMS_ARCH = { + "ARM": "aarch64", + "Unknown": "x86_64", + "X86": "x86_64" +} + + class BootSetError(Exception): """ Generic error class for fatal problems found during boot set validation """ +class BootSetArchMismatch(BootSetError): + def __init__(self, bs_arch: str, expected_ims_arch: str, actual_ims_arch: str): + super().__init__(f"Boot set arch '{bs_arch}' means IMS image arch should be " + f"'{expected_ims_arch}', but actual IMS image arch is '{actual_ims_arch}'") + + +class CannotValidateBootSetArch(BootSetError): + def __init__(self, msg: str): + super().__init__(f"Can't validate boot image arch: {msg}") + + def validate_boot_sets(session_template: dict, operation: str, template_name: str, @@ -139,6 +162,15 @@ def _validate_boot_set(bs: dict, operation: str, reject_nids: bool) -> list[str] except Exception as err: raise BootSetError(f"Can't find boot artifacts. Error: {exc_type_msg(err)}") from err + try: + validate_boot_set_arch(bs, image_metadata) + except CannotValidateBootSetArch as err: + warning_msgs.append(str(err)) + except BootSetError as err: + raise BootSetError(f"Arch validation error: {err}") from err + except Exception as err: + raise BootSetError(f"Arch validation error: {exc_type_msg(err)}") from err + # Check boot artifacts' S3 headers for boot_artifact in ["kernel"]: try: @@ -171,6 +203,36 @@ def _validate_boot_set(bs: dict, operation: str, reject_nids: bool) -> list[str] return warning_msgs + +def validate_boot_set_arch(bs: dict, image_metadata: BootImageMetaData|None=None) -> None: + """ + If the boot set architecture is not set to Other, check that the IMS image + architecture matches the boot set architecture (treating a boot set architecture + of Unknown as X86) + """ + arch = bs.get("arch", DEFAULT_ARCH) + if arch == 'Other': + raise CannotValidateBootSetArch("Boot set arch set to 'Other'") + + if image_metadata is None: + try: + image_metadata = BootImageMetaDataFactory(bs)() + except Exception as err: + raise CannotValidateBootSetArch( + f"Can't find boot artifacts: {exc_type_msg(err)}") from err + + try: + ims_image_arch = image_metadata.arch + except Exception as err: + raise CannotValidateBootSetArch(exc_type_msg(err)) from err + + if ims_image_arch is None: + raise CannotValidateBootSetArch("Can't determine architecture of boot artifacts") + if EXPECTED_IMS_ARCH[arch] != ims_image_arch: + raise BootSetArchMismatch(bs_arch=arch, expected_ims_arch=EXPECTED_IMS_ARCH[arch], + actual_ims_arch=ims_image_arch) + + def validate_sanitize_boot_sets(template_data: dict) -> None: """ Calls validate_sanitize_boot_set on every boot set in the template. @@ -213,6 +275,16 @@ def validate_sanitize_boot_set(bs_name: str, bs_data: dict, reject_nids: bool=Fa raise ParsingException(f"boot_sets key ({bs_name}) does not match 'name' " f"field of corresponding boot set ({bs_data['name']})") + # Validate the boot set architecture + try: + validate_boot_set_arch(bs_data) + except CannotValidateBootSetArch as err: + LOGGER.warning('%s', bs_data) + LOGGER.warning("Bboot set '%s': %s", bs_name, err) + except Exception as err: + raise ParsingException( + f"Error found validating arch of boot set '{bs_name}': {exc_type_msg(err)}") from err + # Validate that the boot set has at least one of the HARDWARE_SPECIFIER_FIELDS if not any(field_name in bs_data for field_name in HARDWARE_SPECIFIER_FIELDS): raise ParsingException(f"Boot set {bs_name} has none of the following " From f22de8ed213522d8ea5be6ee75c16e8be4eeca03 Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Tue, 1 Oct 2024 12:30:00 -0400 Subject: [PATCH 13/18] Assume IMS images are x86 if no arch field present --- .../utils/boot_image_metadata/s3_boot_image_metadata.py | 9 ++++++--- src/bos/operators/utils/clients/ims.py | 3 +++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/bos/operators/utils/boot_image_metadata/s3_boot_image_metadata.py b/src/bos/operators/utils/boot_image_metadata/s3_boot_image_metadata.py index f7869078..798edb29 100644 --- a/src/bos/operators/utils/boot_image_metadata/s3_boot_image_metadata.py +++ b/src/bos/operators/utils/boot_image_metadata/s3_boot_image_metadata.py @@ -27,7 +27,8 @@ from bos.common.utils import exc_type_msg from bos.operators.utils.boot_image_metadata import BootImageMetaData, BootImageMetaDataBadRead -from bos.operators.utils.clients.ims import get_image, get_ims_id_from_s3_url, ImageNotFound +from bos.operators.utils.clients.ims import get_image, get_ims_id_from_s3_url, ImageNotFound, \ + DEFAULT_IMS_IMAGE_ARCH from bos.operators.utils.clients.s3 import S3BootArtifacts, S3MissingConfiguration, S3Url, \ ArtifactNotFound @@ -236,8 +237,10 @@ def arch(self): try: return ims_image_data["arch"] except KeyError: - LOGGER.warning("Can't determine architecture of '%s' because 'arch' field not set in " - "IMS image '%s': %s", s3_url.url, ims_id, ims_image_data) + LOGGER.warning("Defaulting to '%s' because 'arch' field not set in IMS image '%s' " + "(s3 path '%s'): %s", DEFAULT_IMS_IMAGE_ARCH, ims_id, s3_url.url, + ims_image_data) + return DEFAULT_IMS_IMAGE_ARCH except Exception as err: LOGGER.error("IMS image '%s' (s3 path '%s'): %s", ims_id, s3_url.url, ims_image_data) LOGGER.error("Error getting 'arch' field for IMS image '%s': %s", ims_id, diff --git a/src/bos/operators/utils/clients/ims.py b/src/bos/operators/utils/clients/ims.py index 484d9818..f1c5f224 100644 --- a/src/bos/operators/utils/clients/ims.py +++ b/src/bos/operators/utils/clients/ims.py @@ -42,6 +42,9 @@ IMS_S3_KEY_RE = r'^([^/]+)/.+' IMS_S3_KEY_RE_PROG = re.compile(IMS_S3_KEY_RE) +# If an IMS image does not have the arch field, default to x86_64 for purposes of +# backward-compatibility +DEFAULT_IMS_IMAGE_ARCH = 'x86_64' class TagFailure(Exception): pass From b3c9f2b8641805721fda797a05e1caca3a716ad2 Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Tue, 1 Oct 2024 12:37:20 -0400 Subject: [PATCH 14/18] Add type hints to new functions in ims client --- src/bos/operators/utils/clients/ims.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bos/operators/utils/clients/ims.py b/src/bos/operators/utils/clients/ims.py index f1c5f224..dbd75c21 100644 --- a/src/bos/operators/utils/clients/ims.py +++ b/src/bos/operators/utils/clients/ims.py @@ -25,6 +25,7 @@ import logging import re from requests.exceptions import HTTPError +from requests.sessions import Session as RequestsSession from bos.common.utils import compact_response_text, exc_type_msg, requests_retry_session, PROTOCOL from bos.operators.utils.clients.s3 import S3Url @@ -58,7 +59,7 @@ def __init__(self, image_id: str): super().__init__(f"IMS image id '{image_id}' does not exist in IMS") -def get_image(image_id, session=None) -> dict: +def get_image(image_id: str, session: RequestsSession|None=None) -> dict: """ Queries IMS to retrieve the specified image and return it. If the image does not exist, raise ImageNotFound. From cd7e7be8a8feadc800b0a0cc7e71af1d45cd1552 Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Tue, 1 Oct 2024 12:39:45 -0400 Subject: [PATCH 15/18] Remove superfluous try/except clause when getting IMS arch --- src/bos/server/controllers/v2/boot_set.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/bos/server/controllers/v2/boot_set.py b/src/bos/server/controllers/v2/boot_set.py index d1e41200..a4dd52e8 100644 --- a/src/bos/server/controllers/v2/boot_set.py +++ b/src/bos/server/controllers/v2/boot_set.py @@ -221,10 +221,7 @@ def validate_boot_set_arch(bs: dict, image_metadata: BootImageMetaData|None=None raise CannotValidateBootSetArch( f"Can't find boot artifacts: {exc_type_msg(err)}") from err - try: - ims_image_arch = image_metadata.arch - except Exception as err: - raise CannotValidateBootSetArch(exc_type_msg(err)) from err + ims_image_arch = image_metadata.arch if ims_image_arch is None: raise CannotValidateBootSetArch("Can't determine architecture of boot artifacts") From 3242926b4ef27a0a5e761ca1f8d4be7d34c1e7ff Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Tue, 1 Oct 2024 12:50:35 -0400 Subject: [PATCH 16/18] Add type hints where missing in IMS client --- src/bos/operators/utils/clients/ims.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/bos/operators/utils/clients/ims.py b/src/bos/operators/utils/clients/ims.py index dbd75c21..1c940fbe 100644 --- a/src/bos/operators/utils/clients/ims.py +++ b/src/bos/operators/utils/clients/ims.py @@ -91,7 +91,7 @@ def get_image(image_id: str, session: RequestsSession|None=None) -> dict: raise -def patch_image(image_id, data, session=None): +def patch_image(image_id: str, data: dict, session: RequestsSession|None=None) -> None: if not data: LOGGER.warning("patch_image called without data; returning without action.") return @@ -111,7 +111,8 @@ def patch_image(image_id, data, session=None): raise -def tag_image(image_id: str, operation: str, key: str, value: str = None, session=None) -> None: +def tag_image(image_id: str, operation: str, key: str, value: str=None, + session: RequestsSession|None=None) -> None: if operation not in IMS_TAG_OPERATIONS: msg = f"{operation} not valid. Expecting one of {IMS_TAG_OPERATIONS}" LOGGER.error(msg) From f0556a8ce3fbd981b8657ddb9f8b18642b6111cc Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Tue, 1 Oct 2024 12:54:43 -0400 Subject: [PATCH 17/18] Migration: Set default arch values for boot sets with no arch field --- CHANGELOG.md | 2 ++ src/bos/server/migrations/sanitize.py | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f17374f9..fb185d8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Marked PATCH session status endpoint to be ignored by the CLI. - Eliminate redundancies in API spec by defining `V2SessionNameOrEmpty` schema. - Refactor `validate_boot_sets` function into multiple functions to make it easier to understand. +- During migration to this BOS version, for boot sets with no `arch` field, add the field with its + default value. ### Fixed - When validating boot sets, check all boot sets for severe errors before returning only warnings diff --git a/src/bos/server/migrations/sanitize.py b/src/bos/server/migrations/sanitize.py index 34443f04..d4171686 100644 --- a/src/bos/server/migrations/sanitize.py +++ b/src/bos/server/migrations/sanitize.py @@ -28,7 +28,7 @@ import string from bos.common.tenant_utils import get_tenant_aware_key -from bos.server.controllers.v2.boot_set import HARDWARE_SPECIFIER_FIELDS +from bos.server.controllers.v2.boot_set import DEFAULT_ARCH, HARDWARE_SPECIFIER_FIELDS from bos.server.schema import validator from .db import TEMP_DB, delete_component, delete_session, delete_template @@ -196,6 +196,10 @@ def sanitize_bootset(bsname: str, bsdata: dict) -> str|None: # be stored inside the boot set under the current API spec bsdata.pop("name", None) + # If the arch field is not present, set it to its default value + if "arch" not in bsdata: + bsdata["arch"] = DEFAULT_ARCH + # Remove any fields that are no longer in the spec bad_fields = [ field for field in bsdata if field not in validator.boot_set_fields ] for field in bad_fields: From 9c51d33e192d2bd76e30855a05182134c300d220 Mon Sep 17 00:00:00 2001 From: "Mitch Harding (the weird one)" Date: Tue, 1 Oct 2024 15:37:43 -0400 Subject: [PATCH 18/18] Release 2.29.0 for CSM 1.6 --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb185d8b..8eee05a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] + +## [2.29.0] - 2024-10-01 ### Added - Run `pylint` during builds - When validating boot sets, if a boot artifact is missing from a manifest, include the manifest S3 URL