Upgrade/Downgrade agent if Requested Version available #2479

larohra · 2022-01-20T12:41:51Z

Description

This PR contains the next set of changes for GA Versioning. Certain key points of this PR -

The agent wont accept downgrades for requested versions < current daemon version
Blacklist the Agent on downgrade
AutoUpdate as soon as a requested version is available rather than waiting for the wait times (4 hrs/24 hrs)
Remove dead code that is not relevant anymore
A bit of a re-write of the auto-update logic (with emphasis on making sure the current scenario is not broken)

Issue #

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
If applicable, the PR references the bug/issue that it fixes in the description.
New Unit tests were added for the changes made

Quality of Code and Contribution Guidelines

I have read the contribution guidelines.

# Conflicts: # azurelinuxagent/ga/update.py

larohra · 2022-01-20T12:42:19Z

azurelinuxagent/common/version.py

@@ -298,6 +298,3 @@ def set_goal_state_agent():

 GOAL_STATE_AGENT_VERSION = set_goal_state_agent()

-
-def is_current_agent_installed():


This code logic was not being used anymore, so removed it

larohra · 2022-01-20T12:44:16Z

azurelinuxagent/ga/update.py

@@ -815,17 +853,6 @@ def _is_orphaned(self):

        return fileutil.read_file(conf.get_agent_pid_file_path()) != ustr(parent_pid)

-    def _is_version_eligible(self, version):


This function was only being used in 1 place (during auto-update) and due to the condition below, it was always True (if version == CURRENT_VERSION and is_current_agent_installed(): return True). Since it was not adding much value to the code, removed it

codecov · 2022-01-20T12:45:41Z

Codecov Report

Merging #2479 (80c3575) into develop (2b6dc40) will increase coverage by 0.05%.
The diff coverage is 92.10%.

@@             Coverage Diff             @@
##           develop    #2479      +/-   ##
===========================================
+ Coverage    71.96%   72.01%   +0.05%     
===========================================
  Files          101      101              
  Lines        15064    15100      +36     
  Branches      2394     2399       +5     
===========================================
+ Hits         10841    10875      +34     
  Misses        3737     3737              
- Partials       486      488       +2

Impacted Files	Coverage Δ
azurelinuxagent/common/version.py	`70.23% <ø> (-0.36%)`	⬇️
azurelinuxagent/ga/update.py	`89.03% <92.10%> (+0.23%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b6dc40...80c3575. Read the comment docs.

larohra · 2022-01-20T12:46:39Z

tests/ga/test_update.py

@@ -1100,34 +1101,6 @@ def test_is_orphaned_returns_true_if_parent_does_not_exist(self):
        with patch('os.getppid', return_value=42):
            self.assertTrue(self.update_handler._is_orphaned)

-    def test_is_version_available(self):


Removed the test cases for the functionality that was removed

narrieta

submitting a few comments for now... still reviewing the code

azurelinuxagent/ga/update.py

narrieta · 2022-01-21T16:35:09Z

azurelinuxagent/ga/update.py

+                    # (unless the CURRENT_VERSION == daemon version, but since we don't support downgrading
+                    # below daemon version, we will never reach this code path if that's the scenario)
+                    current_agent = next(agent for agent in self.agents if agent.version == CURRENT_VERSION)
+                    current_agent.mark_failure(is_fatal=True)


can we add a log statement explaining why this is being blacklisted? (and not mentioning "blacklisting", which an implementation thing, but rather that that specific version will not be executed because blah blah)

Done (though I did mention the word blacklisting because I feel its pretty self explanatory (and makes it easier to explain) even to folks outside our dev group who read the logs for debugging.

narrieta · 2022-01-21T16:37:49Z

azurelinuxagent/ga/update.py

+                    current_agent = next(agent for agent in self.agents if agent.version == CURRENT_VERSION)
+                    current_agent.mark_failure(is_fatal=True)
+                except StopIteration:
+                    logger.warn(


do we need this warning? this is the current version not finding itself, right? i think that would happen only if the current version is running as the daemon

i think that would happen only if the current version is running as the daemon

This scenario is already handled separately. If that is the case, we would not hit this code path. That is the primary reason for this warning, as that situation should not happen ever

azurelinuxagent/ga/update.py

narrieta · 2022-01-21T17:11:41Z

azurelinuxagent/ga/update.py

@@ -923,59 +967,78 @@ def _check_and_download_agent_if_upgrade_available(self, protocol, base_version=
        if not conf.get_autoupdate_enabled():
            return False

-        def report_error(msg_, version=CURRENT_VERSION):
+        def report_error(msg_, version_=CURRENT_VERSION, op=WALAEventOperation.Download):


Let's not default this WALAEventOperation.Download, but make it an explicit parameter. There are some usages where the default is being used to report issues not related do download

Which usage exactly did you see where we're reporting it as a download failure but its not one? This helper function is mainly to log those errors inside the _download_agent_if_upgrade_available function and instead of one call to it, all other calls for this function are Download errors, that's why I defaulted to that operation to make the code cleaner

larohra added 10 commits January 12, 2022 17:57

Removed dead code

42925b3

Rename functions

a62a8f8

Merge branch 'develop' into requested-ver-upgrade

e423926

# Conflicts: # azurelinuxagent/ga/update.py

Added support for GA versioning upgrade/downgrade

f598fcf

Added support for GA versioning upgrade/downgrade

37054a4

Fixed minor bugs and failing test cases

c6e19ed

Fix failing test

bd194e9

Merge branch 'develop' into requested-ver-upgrade

2280709

Added more tests

7e5fc02

Removed dead code

fec32f6

larohra assigned narrieta Jan 20, 2022

larohra requested review from dhivyaganesan and kevinclark19a as code owners January 20, 2022 12:41

larohra assigned kevinclark19a Jan 20, 2022

larohra requested review from nagworld9, narrieta and ZhidongPeng as code owners January 20, 2022 12:41

larohra commented Jan 20, 2022

View reviewed changes

narrieta reviewed Jan 21, 2022

View reviewed changes

larohra added 2 commits January 24, 2022 15:05

Code refactor and address PR comments

31441dc

Moved flag out of helper functions

f6dbed2

narrieta approved these changes Jan 26, 2022

View reviewed changes

kevinclark19a approved these changes Jan 27, 2022

View reviewed changes

Merge branch 'develop' into requested-ver-upgrade

80c3575

larohra merged commit b27e475 into Azure:develop Jan 28, 2022

larohra deleted the requested-ver-upgrade branch January 28, 2022 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade/Downgrade agent if Requested Version available #2479

Upgrade/Downgrade agent if Requested Version available #2479

larohra commented Jan 20, 2022

larohra Jan 20, 2022

larohra Jan 20, 2022

codecov bot commented Jan 20, 2022 •

edited

Loading

larohra Jan 20, 2022

narrieta left a comment

narrieta Jan 21, 2022

larohra Jan 24, 2022

narrieta Jan 21, 2022

larohra Jan 24, 2022

narrieta Jan 21, 2022

larohra Jan 24, 2022

		@@ -298,6 +298,3 @@ def set_goal_state_agent():

		GOAL_STATE_AGENT_VERSION = set_goal_state_agent()


		def is_current_agent_installed():

		@@ -815,17 +853,6 @@ def _is_orphaned(self):

		return fileutil.read_file(conf.get_agent_pid_file_path()) != ustr(parent_pid)

		def _is_version_eligible(self, version):

Upgrade/Downgrade agent if Requested Version available #2479

Upgrade/Downgrade agent if Requested Version available #2479

Conversation

larohra commented Jan 20, 2022

Description

PR information

Quality of Code and Contribution Guidelines

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 20, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

narrieta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 20, 2022 •

edited

Loading