From 8505a5a7d07ccc7f7317f911c46109b67090c8b7 Mon Sep 17 00:00:00 2001 From: Matheus Tosta Date: Tue, 9 Apr 2024 15:44:02 -0300 Subject: [PATCH 1/2] hot fix a bug found in the self update task during QA where the upstream tasks weren't being executed after the package update --- jobbergate-agent/CHANGELOG.md | 2 + .../jobbergate_agent/internals/update.py | 72 ++++++++++++++++--- .../tests/internals/test_update.py | 26 ++++++- 3 files changed, 88 insertions(+), 12 deletions(-) diff --git a/jobbergate-agent/CHANGELOG.md b/jobbergate-agent/CHANGELOG.md index 730493773..e9ee2b619 100644 --- a/jobbergate-agent/CHANGELOG.md +++ b/jobbergate-agent/CHANGELOG.md @@ -4,6 +4,8 @@ This file keeps track of all notable changes to jobbergate-agent ## Unreleased +- Hot fix regarding the self update task where tasks weren't properly scheduled after the version update +- Added support for comparing pre-release versions in the self update task ## 5.0.0a1 -- 2024-04-04 - Added a task scheduler whose purpose is to self update the agent [PENG-2116] diff --git a/jobbergate-agent/jobbergate_agent/internals/update.py b/jobbergate-agent/jobbergate_agent/internals/update.py index 5765062d0..566d503f8 100644 --- a/jobbergate-agent/jobbergate_agent/internals/update.py +++ b/jobbergate-agent/jobbergate_agent/internals/update.py @@ -1,11 +1,16 @@ +import re import subprocess import sys +from apscheduler.job import Job +from apscheduler.schedulers.asyncio import AsyncIOScheduler from loguru import logger from pkg_resources import get_distribution from jobbergate_agent.clients.cluster_api import backend_client as jobbergate_api_client -from jobbergate_agent.utils.scheduler import schedule_tasks, scheduler + +# flake8 doesn't understand the scheduler is used in the self_update_agent function +from jobbergate_agent.utils.scheduler import schedule_tasks, scheduler # noqa: F401 package_name = "jobbergate_agent" @@ -25,18 +30,51 @@ def _need_update(current_version: str, upstream_version: str) -> bool: """Compare the current version with the upstream version. In case the current version is the same as the upstream version, return False. - As well as, in case the major versions are the same, return False, as we don't want - to update across major versions. Otherwise, return True. - - This behaviour allows the agent to update and rollback across all minor and patch versions. + If the major versions are different, return False, as updates across major versions are not desired. + Otherwise, return True, allowing updates and rollbacks across all minor and patch versions, + including handling for pre-release versions ('a' for alpha, 'b' for beta). """ - if current_version == upstream_version: - return False - current_major, _, _ = map(int, current_version.split(".")) - upstream_major, _, _ = map(int, upstream_version.split(".")) + current_major: int | str + current_minor: int | str + current_patch: int | str + current_pre_version: int | str + upstream_major: int | str + upstream_minor: int | str + upstream_patch: int | str + upstream_pre_version: int | str + + # regular expression to parse version strings: major.minor.patch[ab][pre-release] + version_pattern = r"^(\d+)\.(\d+)\.(\d+)([ab](\d+))?$" + + current_match = re.match(version_pattern, current_version) + upstream_match = re.match(version_pattern, upstream_version) + + if not current_match or not upstream_match: + raise ValueError( + f"One of the following versions are improperly formatted: {current_version}, {upstream_version}" + ) + + current_major, current_minor, current_patch, _, current_pre_version = current_match.groups(default="") + upstream_major, upstream_minor, upstream_patch, _, upstream_pre_version = upstream_match.groups(default="") + + current_major, current_minor, current_patch = map(int, [current_major, current_minor, current_patch]) + upstream_major, upstream_minor, upstream_patch = map(int, [upstream_major, upstream_minor, upstream_patch]) + + current_pre_version = int(current_pre_version) if current_pre_version.isdigit() else 0 + upstream_pre_version = int(upstream_pre_version) if upstream_pre_version.isdigit() else 0 + + # major version check if current_major != upstream_major: return False - return True + # minor version check + elif (current_minor != upstream_minor and upstream_pre_version == 0) or ( + current_minor == upstream_minor and current_pre_version != 0 and upstream_pre_version == 0 + ): + return True + # patch version check + elif current_patch != upstream_patch and upstream_pre_version == 0: + return True + return False def _update_package(version: str) -> None: @@ -48,6 +86,8 @@ async def self_update_agent(): In case the agent is updated, the scheduler is shutdown and restarted with the new version. """ + global scheduler + current_version = get_distribution(package_name).version upstream_version = await _fetch_upstream_version_info() logger.debug( @@ -60,12 +100,22 @@ async def self_update_agent(): logger.debug("Shutting down the scheduler...") scheduler.shutdown(wait=False) + logger.debug("Clearing the scheduler jobs...") + scheduler_jobs: list[Job] = scheduler.get_jobs() + for job in scheduler_jobs: + job.remove() + logger.debug(f"Updating {package_name} from version {current_version} to {upstream_version}...") _update_package(upstream_version) logger.debug("Update completed successfully.") logger.debug(f"Loading plugins from version {upstream_version}...") - schedule_tasks(scheduler) + new_scheduler = AsyncIOScheduler() + schedule_tasks(new_scheduler) + new_scheduler.start() logger.debug("Plugins loaded successfully.") + + logger.info("Replacing the scheduler with the new version...") + scheduler = new_scheduler else: logger.debug("No update is required or update crosses a major version divide.") diff --git a/jobbergate-agent/tests/internals/test_update.py b/jobbergate-agent/tests/internals/test_update.py index 27638b848..442d20ea8 100644 --- a/jobbergate-agent/tests/internals/test_update.py +++ b/jobbergate-agent/tests/internals/test_update.py @@ -46,6 +46,12 @@ async def test__fetch_upstream_version_info__check_http_error(http_code: int): ("2.0.0", "1.0.0", False), # Major version rollback ("1.2.0", "1.1.0", True), # Minor version rollback ("1.0.1", "1.0.0", True), # Patch version rollback + ("1.1.0a1", "1.1.0", True), # Minor alpha version rollback + ("1.6.0a1", "1.6.1", True), # Patch alpha version update available + ("1.0.0a1", "1.0.0", True), # Alpha version rollback + ("1.0.9a1", "1.1.0", True), # Alpha version update available + ("1.0.9a1", "1.0.9a2", False), # Alpha version update available + ("1.4.7", "1.5.0a1", False), # Alpha minor version update ], ) def test_need_update(current_version: str, upstream_version: str, expected_result: bool): @@ -59,6 +65,8 @@ def test_need_update(current_version: str, upstream_version: str, expected_resul ("1.0", "1.0.1"), # Improperly formatted current version ("1.0.0", "1.0"), # Improperly formatted upstream version ("1", "2"), # Major version with no minor/patch + ("1.0.1a", "1.1.0"), # Pre-release improperly formatted + ("1.0.1", "1.0.10b"), # Pre-release improperly formatted ], ) def test_need_update__check_improperly_formatted_versions( @@ -112,7 +120,9 @@ def test_update_package(mocked_sys: mock.MagicMock, mocked_subprocess: mock.Magi @mock.patch("jobbergate_agent.internals.update._need_update") @mock.patch("jobbergate_agent.internals.update.scheduler") @mock.patch("jobbergate_agent.internals.update.schedule_tasks") +@mock.patch("jobbergate_agent.internals.update.AsyncIOScheduler") async def test_self_update_agent( + mocked_asyncio_scheduler: mock.MagicMock, mocked_schedule_tasks: mock.MagicMock, mocked_scheduler: mock.MagicMock, mocked_need_update: mock.MagicMock, @@ -133,6 +143,11 @@ async def test_self_update_agent( mocked_need_update.return_value = is_update_available mocked_scheduler.shutdown = mock.Mock() + mocked_new_scheduler = mock.Mock() + mocked_asyncio_scheduler.return_value = mocked_new_scheduler + mocked_new_scheduler.shutdown = mock.Mock() + mocked_new_scheduler.start = mock.Mock() + await self_update_agent() mocked_get_distribution.assert_called_once_with("jobbergate_agent") @@ -141,8 +156,17 @@ async def test_self_update_agent( if is_update_available: mocked_scheduler.shutdown.assert_called_once_with(wait=False) mocked_update_package.assert_called_once_with(upstream_version) - mocked_schedule_tasks.assert_called_once_with(mocked_scheduler) + mocked_schedule_tasks.assert_called_once_with(mocked_new_scheduler) + mocked_asyncio_scheduler.assert_called_once_with() + mocked_new_scheduler.start.assert_called_once_with() + + # this asserts that the scheduler is updated *in memory* with the new version + from jobbergate_agent.internals.update import scheduler + + assert scheduler is mocked_new_scheduler else: mocked_scheduler.shutdown.assert_not_called() mocked_update_package.assert_not_called() mocked_schedule_tasks.assert_not_called() + mocked_asyncio_scheduler.assert_not_called() + mocked_new_scheduler.start.assert_not_called() From 12c27dfab7bec4da2e7d172821f5523f6e5ca175 Mon Sep 17 00:00:00 2001 From: Matheus Tosta Date: Tue, 9 Apr 2024 17:10:43 -0300 Subject: [PATCH 2/2] remove support for pre release self update --- jobbergate-agent/CHANGELOG.md | 1 - .../jobbergate_agent/internals/update.py | 22 +++++-------------- .../tests/internals/test_update.py | 7 +----- 3 files changed, 7 insertions(+), 23 deletions(-) diff --git a/jobbergate-agent/CHANGELOG.md b/jobbergate-agent/CHANGELOG.md index e9ee2b619..e62caa9c8 100644 --- a/jobbergate-agent/CHANGELOG.md +++ b/jobbergate-agent/CHANGELOG.md @@ -5,7 +5,6 @@ This file keeps track of all notable changes to jobbergate-agent ## Unreleased - Hot fix regarding the self update task where tasks weren't properly scheduled after the version update -- Added support for comparing pre-release versions in the self update task ## 5.0.0a1 -- 2024-04-04 - Added a task scheduler whose purpose is to self update the agent [PENG-2116] diff --git a/jobbergate-agent/jobbergate_agent/internals/update.py b/jobbergate-agent/jobbergate_agent/internals/update.py index 566d503f8..c1408e161 100644 --- a/jobbergate-agent/jobbergate_agent/internals/update.py +++ b/jobbergate-agent/jobbergate_agent/internals/update.py @@ -37,14 +37,12 @@ def _need_update(current_version: str, upstream_version: str) -> bool: current_major: int | str current_minor: int | str current_patch: int | str - current_pre_version: int | str upstream_major: int | str upstream_minor: int | str upstream_patch: int | str - upstream_pre_version: int | str - # regular expression to parse version strings: major.minor.patch[ab][pre-release] - version_pattern = r"^(\d+)\.(\d+)\.(\d+)([ab](\d+))?$" + # regular expression to parse version strings: major.minor.patch + version_pattern = r"^(\d+)\.(\d+)\.(\d+)$" current_match = re.match(version_pattern, current_version) upstream_match = re.match(version_pattern, upstream_version) @@ -54,25 +52,17 @@ def _need_update(current_version: str, upstream_version: str) -> bool: f"One of the following versions are improperly formatted: {current_version}, {upstream_version}" ) - current_major, current_minor, current_patch, _, current_pre_version = current_match.groups(default="") - upstream_major, upstream_minor, upstream_patch, _, upstream_pre_version = upstream_match.groups(default="") + current_major, current_minor, current_patch = current_match.groups() + upstream_major, upstream_minor, upstream_patch = upstream_match.groups() current_major, current_minor, current_patch = map(int, [current_major, current_minor, current_patch]) upstream_major, upstream_minor, upstream_patch = map(int, [upstream_major, upstream_minor, upstream_patch]) - current_pre_version = int(current_pre_version) if current_pre_version.isdigit() else 0 - upstream_pre_version = int(upstream_pre_version) if upstream_pre_version.isdigit() else 0 - # major version check if current_major != upstream_major: return False - # minor version check - elif (current_minor != upstream_minor and upstream_pre_version == 0) or ( - current_minor == upstream_minor and current_pre_version != 0 and upstream_pre_version == 0 - ): - return True - # patch version check - elif current_patch != upstream_patch and upstream_pre_version == 0: + # minor and patch version check + elif current_minor != upstream_minor or current_patch != upstream_patch: return True return False diff --git a/jobbergate-agent/tests/internals/test_update.py b/jobbergate-agent/tests/internals/test_update.py index 442d20ea8..5163f8658 100644 --- a/jobbergate-agent/tests/internals/test_update.py +++ b/jobbergate-agent/tests/internals/test_update.py @@ -46,12 +46,6 @@ async def test__fetch_upstream_version_info__check_http_error(http_code: int): ("2.0.0", "1.0.0", False), # Major version rollback ("1.2.0", "1.1.0", True), # Minor version rollback ("1.0.1", "1.0.0", True), # Patch version rollback - ("1.1.0a1", "1.1.0", True), # Minor alpha version rollback - ("1.6.0a1", "1.6.1", True), # Patch alpha version update available - ("1.0.0a1", "1.0.0", True), # Alpha version rollback - ("1.0.9a1", "1.1.0", True), # Alpha version update available - ("1.0.9a1", "1.0.9a2", False), # Alpha version update available - ("1.4.7", "1.5.0a1", False), # Alpha minor version update ], ) def test_need_update(current_version: str, upstream_version: str, expected_result: bool): @@ -67,6 +61,7 @@ def test_need_update(current_version: str, upstream_version: str, expected_resul ("1", "2"), # Major version with no minor/patch ("1.0.1a", "1.1.0"), # Pre-release improperly formatted ("1.0.1", "1.0.10b"), # Pre-release improperly formatted + ("1.0.9a1", "1.0.9a2"), # Alpha version is not an allowed format ], ) def test_need_update__check_improperly_formatted_versions(