Skip to content
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

Do not raise on missing status blob; reduce amount of logging for vms… #2492

Merged
merged 2 commits into from
Feb 8, 2022

Conversation

narrieta
Copy link
Member

@narrieta narrieta commented Feb 7, 2022

The status blob url should not be a required property in vmSettings. Instead of raising an error, not it is initialized to None (to preserve the behavior established by extensionsConfig).

Also, since some errors are more common than expected, reduced the max number of errors from 5/hour to 3/hour (telemetry) and 1/hour (local log), and reduced the amount of details in parsing errors.

@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #2492 (21960ae) into release-2.7.0.0 (a21d9dd) will increase coverage by 0.01%.
The diff coverage is 80.95%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           release-2.7.0.0    #2492      +/-   ##
===================================================
+ Coverage            71.92%   71.94%   +0.01%     
===================================================
  Files                  101      101              
  Lines                15028    15030       +2     
  Branches              2386     2387       +1     
===================================================
+ Hits                 10809    10813       +4     
+ Misses                3735     3734       -1     
+ Partials               484      483       -1     
Impacted Files Coverage Δ
...protocol/extensions_goal_state_from_vm_settings.py 81.67% <69.23%> (+0.87%) ⬆️
...inuxagent/common/protocol/extensions_goal_state.py 88.46% <100.00%> (-0.08%) ⬇️
azurelinuxagent/common/protocol/wire.py 81.61% <100.00%> (+0.04%) ⬆️
azurelinuxagent/common/version.py 70.58% <100.00%> (ø)
azurelinuxagent/ga/exthandlers.py 85.75% <100.00%> (ø)

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 b4fa41b...21960ae. Read the comment docs.

@@ -0,0 +1,24 @@
<Extensions version="1.0.0.0" goalStateIncarnation="9">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this file used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

if self._status_upload_blob is None:
raise Exception("Missing statusUploadBlob.value")
self._status_upload_blob_type = status_upload_blob.get("statusBlobType")
if self._status_upload_blob is None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate check, I think it should be self._status_upload_blob_type

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add failure scenario if you could?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks; fixed typo. the test already exists: test_create_from_vm_settings_should_assume_block_when_blob_type_is_not_valid

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this typo carried from develop and mentioned test case not covering None case that's why we didn't catch it before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving it unless you want to add blob_type is None case, thanks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking for invalid is good enough. it is unrealistic to check for every possible invalid value: missing, empty string, mixed case, garbage, etc.

the existing test captures the desired behavior: an invalid value defaults to Block

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not agreed 100%, At least we should have a test case that was handled it in the code. My intention is we would have caught this typo if we have test case where status_upload_blob_type is None (not some invalid value) and assert BlockBlob type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we would have captured this typo, but we did not know we were going to make this typo. As another example: should we add a test for mixed case that could catch a possibly defective piece of code that doesn't handle that case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree but my only point in general, we were expected certain behavior when it is None. That behavior should have been tested.

Beyond this, we fixed the typo I'm good with that :)

@narrieta narrieta merged commit 7cce03b into Azure:release-2.7.0.0 Feb 8, 2022
narrieta added a commit to narrieta/WALinuxAgent that referenced this pull request Feb 8, 2022
Azure#2492)

* Do not raise on missing status blob; reduce amount of logging for vmsettings

* remove extra file; fix typo

Co-authored-by: narrieta <narrieta>
(cherry picked from commit 7cce03b)
narrieta added a commit that referenced this pull request Feb 8, 2022
#2492) (#2496)

* Do not raise on missing status blob; reduce amount of logging for vmsettings

* remove extra file; fix typo

Co-authored-by: narrieta <narrieta>
(cherry picked from commit 7cce03b)
@narrieta narrieta deleted the vmsettings-errors branch February 9, 2022 23:49
nagworld9 added a commit that referenced this pull request Mar 7, 2023
* release prepare-2.4.0.0 (#2280)

* Fix bug with dependent extensions with no settings (#2285)

* update test-requirements to pin pylint. (#2288) (#2299)

Co-authored-by: Kevin Clark <kevin.clark@microsoft.com>

* Do not create placeholder status file for AKS extensions (#2298)

* Exception for Linux Patch Extension for creating placeholder status file (#2307)

* update release version (#2308)

* Dont create default status file for Single-Config extensions (#2318)

* version update (#2319)

* Update Version (#2348)

* Fix bug with dependent extensions with no settings (#2285) (#2349)

Co-authored-by: Laveesh Rohra <larohra@microsoft.com>

* Use handler status if extension status is None when computing the Ext… (#2358)

* Use handler status if extension status is None when computing the ExtensionsSummary

* Add NotReady to convergence statuses

Co-authored-by: narrieta <narrieta>

* Release preparation 2.5.0.1 (#2360)

* Define ExtensionsSummary.__eq__ (#2371)

* define ExtensionsSummary.__eq__

* Fix test name

* Fix test name

* Fix test name

Co-authored-by: narrieta <narrieta>

* Release preparation 2.5.0.2 (#2372)

* update cgroups monitoring expiry date (#2392)

* Updated Agent Version to 2.6.0.0 (#2393)

* Do not parse status blob (#2394)

Co-authored-by: narrieta <narrieta>

* Exclude dcr from setup (#2396)

* Improve error message for vmSettings errors (#2397)

* Improve error message for vmSettings errors

* Add etag and correlation id

* pylint warning

Co-authored-by: narrieta <narrieta>

* Updated agent version to 2.6.0.1 (#2400)

* Verify that extensions goal state from vmsettings has been initialized (#2404)

Co-authored-by: narrieta <narrieta>

* Updated agent version to 2.6.0.2 (#2405)

* Set Agent version to 2.7.0.0 (#2457)

* Set Agent version to 2.7.0.0

* Remove duplicate import

Co-authored-by: narrieta <narrieta>

* Simplify the logic to update the extensions goal state (#2466)

* Simplify the logic to update the extensions goal state

* Added telemetry for NotSupported

* Added comments

* Do not support old hostgaplugin

Co-authored-by: narrieta <narrieta>

* Retry update_goal_state on GoalStateMismatchError (#2470)

* Retry update_goal_state on GoalStateMismatchError

* Add sleep before retry

* Enable test

* Enable test

* Update test

* Add unit test

* Add data file

* pylint warning

* Add comment; fix typos

* fix typo

Co-authored-by: narrieta <narrieta>

* Set agent version to 2.7.0.1 (#2473)

* Redact settings from mismatch message (#2477)

Co-authored-by: narrieta <narrieta>

* Set Agent Version to 2.7.0.2 (#2478)

Co-authored-by: narrieta <narrieta>

* Improvements in telemetry for vmSettings (#2482)

Co-authored-by: narrieta <narrieta>

* Set Agent version to 2.7.0.3 (#2483)

* Do not raise on missing status blob; reduce amount of logging for vms… (#2492)

* Do not raise on missing status blob; reduce amount of logging for vmsettings

* remove extra file; fix typo

Co-authored-by: narrieta <narrieta>

* Set agent version to 2.7.0.4 (#2494)

Co-authored-by: narrieta <narrieta>

* Save vmSettings on parse errors; improve messages in parse errors (#2503)

* Save vmSettings on parse errors; improve messages in parse errors

* pylint warnings

* pylint warnings

Co-authored-by: narrieta <narrieta>

* Set agent version to 2.7.0.5 (#2504)

Co-authored-by: narrieta <narrieta>

* Revert "Set agent version to 2.7.0.5 (#2504)" (#2505)

This reverts commit ae5a222.

Co-authored-by: narrieta <narrieta>

* ignore firewall packets reset error, check enable firewall config flag and extend cgroup extension monitoring expiry time (#2502)

* Set agent version to 2.7.0.5 (#2506)

Co-authored-by: narrieta <narrieta>

* Handle OOM errors by stopping the periodic log collector (#2510)

* Set agent version to 2.7.0.6 (#2511)

* Add keep_alive property to collect_logs (#2533)

* Set agent version to 2.7.1.0. (#2534)

* Set agent version to 2.8.0.0 (#2545)

Co-authored-by: narrieta <narrieta>

* Update HGAP minimum required version for FastTrack (#2549)

Co-authored-by: narrieta <narrieta>

* Update agent version to 2.8.0.1 (#2550)

Co-authored-by: narrieta <narrieta>

* Improvements in waagent.log (#2558)

* Improvements in waagent.log

* fix function call

* update comment

* typo

Co-authored-by: narrieta <narrieta>

* Change format of history items (#2560)

* Change format of history directory

* Update message; fix typo

* py2 compat

* py2 compat

Co-authored-by: narrieta <narrieta>

* Update agent version to 2.8.0.2 (#2561)

Co-authored-by: narrieta <narrieta>

* Refresh goal state when certificates are missing (#2562)

* Refresh goal state when certificates are missing

* Improve error reporting

* Fix assert message

Co-authored-by: narrieta <narrieta>

* Update agent version to 2.8.0.3 (#2563)

Co-authored-by: narrieta <narrieta>

* Do not mark goal state as processed when goal state fails to update (#2569)

Co-authored-by: narrieta <narrieta>

* Update agent version to 2.8.0.4 (#2570)

Co-authored-by: narrieta <narrieta>

* Bug fix for fetching a goal state with empty certificates property (#2575)

* Move error counter reset down to end of block. (#2576)

* Bug Fix: Change fast track timestamp default from None to datetime.min (#2577)

* Update agent version to 2.8.0.5. (#2580)

* Create placeholder GoalState.*.xml file (#2594)

Co-authored-by: narrieta <narrieta>

* Update version to 2.8.0.6 (#2595)

Co-authored-by: narrieta <narrieta>

* fix network interface restart in RHEL9 (#2592) (#2612)

(cherry picked from commit b8ca432)

* set agent version to 2.7.2.0 (#2613)

* Parse missing agent manifests as empty

* Set agent version to 2.8.0.7

* Retry HGAP's extensionsArtifact requests on BAD_REQUEST status (#2621)

* Retry HGAP's extensionsArtifact requests on BAD_REQUEST status

* python 2.6 compat

Co-authored-by: narrieta <narrieta>

* Retry HGAP's extensionsArtifact requests on BAD_REQUEST status (#2621) (#2622)

* Retry HGAP's extensionsArtifact requests on BAD_REQUEST status

* python 2.6 compat

Co-authored-by: narrieta <narrieta>
(cherry picked from commit dbc82d3)

* fix if command in rhel v8.6+ (#2624)

* Set agent version to 2.7.3.0 (#2625)

Co-authored-by: narrieta <narrieta>

* Set Agent version to 2.8.0.8 (#2627)

Co-authored-by: narrieta <narrieta>

* fix network interface restart in RHEL9 (#2592) (#2629)

(cherry picked from commit b8ca432)

Co-authored-by: Nageswara Nandigam <84482346+nagworld9@users.noreply.github.com>

* fix if command in rhel v8.6+ (#2624) (#2630)

(cherry picked from commit e540728)

Co-authored-by: Nageswara Nandigam <84482346+nagworld9@users.noreply.github.com>

* Set Agent version to 2.8.0.9 (#2631)

Co-authored-by: narrieta <narrieta>

* Cleanup history directory when creating new subdirectories (#2633)

* Cleanup history directory when creating new subdirectories

* Review feedback

Co-authored-by: narrieta <narrieta>

* Set agent version to 2.8.0.10

* Save sharedconfig to disk (#2649)

* Save sharedconfig to disk

* Update tests

* pylint warnings

Co-authored-by: narrieta <narrieta>

* Set Agent version to 2.8.0.11 (#2650)

Co-authored-by: narrieta <narrieta>

* test fixes

* Microsoft mandatory file (#2737)

Co-authored-by: microsoft-github-policy-service[bot] <77245923+microsoft-github-policy-service[bot]@users.noreply.github.com>

---------

Co-authored-by: Laveesh Rohra <larohra@microsoft.com>
Co-authored-by: Kevin Clark <kevin.clark@microsoft.com>
Co-authored-by: Norberto Arrieta <narrieta@users.noreply.github.com>
Co-authored-by: Dhivya Ganesan <dhivyaganesan@users.noreply.github.com>
Co-authored-by: narrieta <narrieta>
Co-authored-by: Kevin Clark <keclar@microsoft.com>
Co-authored-by: Norberto Arrieta <narrieta@microsoft.com>
Co-authored-by: microsoft-github-policy-service[bot] <77245923+microsoft-github-policy-service[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants