-
Notifications
You must be signed in to change notification settings - Fork 372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removing infinite download of extension manifest without a new GS #1874
Conversation
DCR ok (except for the known errors) - https://tuxgold.corp.microsoft.com/job/AzLinux/job/DungeonCrawler/job/DungeonCrawler.larohra2/461/artifact/results/ |
…ete scenario rather than specific functions
@@ -249,29 +249,6 @@ def test_extension_sequence_number(self): | |||
disk_sequence_number=3, | |||
expected_sequence_number=-1) | |||
|
|||
@patch("azurelinuxagent.ga.exthandlers.add_event") | |||
@patch("azurelinuxagent.common.errorstate.ErrorState.is_triggered") | |||
def test_it_should_report_an_error_if_the_wireserver_cannot_be_reached(self, patch_is_triggered, patch_add_event): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a redundant test, this scenario is being tested by the tests for try_update_goal_state
functionality
Codecov Report
@@ Coverage Diff @@
## develop #1874 +/- ##
===========================================
- Coverage 69.38% 69.24% -0.15%
===========================================
Files 83 83
Lines 11499 11483 -16
Branches 1598 1597 -1
===========================================
- Hits 7979 7951 -28
- Misses 3183 3193 +10
- Partials 337 339 +2
Continue to review full report at Codecov.
|
@@ -1388,6 +1361,8 @@ def set_handler_status(self, status="NotReady", message="", code=0): | |||
try: | |||
handler_status_json = json.dumps(get_properties(handler_status)) | |||
if handler_status_json is not None: | |||
if not os.path.exists(state_dir): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently if there is any exceptions in extension processing and the config directory doesnt exist, the error is never reported back to CRP and the customer has to wait for 90 mins before the operation timesout. This is a bad Customer Experience. To mitigate it, I'm creating the config directory incase it doesnt exist while reporting status.
exthandlers_handler.run() | ||
_assert_mock_add_event_call(expected_call_count=1) | ||
self._assert_handler_status(protocol.report_vm_status, "NotReady", 0, "1.0.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line will fail this test currently because the cleanup logic cleans up the state and we dont report it again. Once the PR #1889 is merged, this test should pass.
tests/ga/test_extension.py
Outdated
protocol.update_goal_state() | ||
|
||
exthandlers_handler.run() | ||
_assert_mock_add_event_call(expected_call_count=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checking for a fixed number of calls to add_event is fragile. If we happen to add an unrelated event on this path the test will break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments
Description
This PR contains the fix where we dont download the extension manifest without a new GoalSatate
Issue #
PR information
Quality of Code and Contribution Guidelines