Parse statusUploadBlob in vmSettings #2382
Conversation
narrieta
requested review from
dhivyaganesan,
kevinclark19a,
larohra,
nagworld9 and
ZhidongPeng
as code owners
Codecov Report
@@ Coverage Diff @@
## develop #2382 +/- ##
===========================================
+ Coverage 71.09% 71.14% +0.05%
===========================================
Files 97 97
Lines 14302 14384 +82
Branches 2064 2076 +12
===========================================
+ Hits 10168 10234 +66
- Misses 3677 3689 +12
- Partials 457 461 +4
Continue to review full report at Codecov.
|
| Does validations common to vmSettings and ExtensionsConfig | ||
| """ | ||
| if self._status_upload_blob_type not in ["BlockBlob", "PageBlob"]: | ||
| logger.info("Status Blob type '{0}' is ot valid, assuming BlockBlob", self._status_upload_blob) |
narrieta
dismissed stale reviews from dhivyaganesan and kevinclark19a
via
cbd45d2
| try: | ||
| self._do_common_validations() | ||
| except Exception as e: | ||
| raise ExtensionsConfigError("Error parsing ExtensionsConfig (incarnation: {0}): {1}\n{2}".format(incarnation, format_exception(e), xml_text)) |
There was a problem hiding this comment.
Should we trim the xml text? Incase its very big? Is there a chance xml_text can contain secrets? We should redact it in that case too
There was a problem hiding this comment.
These errors should be rare (don't remember ever seeing one in the wild) and the files are usually a few KB long. I would not worry about disk space on the log or event files; telemetry will truncate the message before sending it so no concern there,
Since they are rare, it is best not to truncate in order to have as much debugging info as possible.
The text should be redacted, though, thanks for pointing that out.
There was a problem hiding this comment.
while at it, i also changed get_redacted_text to return an empty string instead of None for empty extensions config.
|
|
||
| self.in_vm_gs_metadata.parse_node(find(xml_doc, "InVMGoalStateMetaData")) | ||
|
|
||
| self._do_common_validations() |
There was a problem hiding this comment.
Any reason for validating in 2 different places? (Line 133 and 169)?
There was a problem hiding this comment.
I want to be really, really sure the text is valid...
No, seriously, the second call is a leftover, will remove it. Thanks
| status_upload_blob = vm_settings.get("statusUploadBlob") | ||
| if status_upload_blob is None: | ||
| raise Exception("Missing statusUploadBlob") | ||
| self._status_upload_blob = status_upload_blob["value"] |
There was a problem hiding this comment.
We should also check if value is present in the status_upload_blob and raise appropriate Exception if not (else a KeyError might be raised)
There was a problem hiding this comment.
Thanks, should be a call to get(), not to the [] operator. Will fix it.
| self._status_upload_blob = status_upload_blob["value"] | ||
| if self._status_upload_blob is None: | ||
| raise Exception("Missing statusUploadBlob.value") | ||
| self._status_upload_blob_type = status_upload_blob["statusBlobType"] |
| # | ||
| # TODO: The current implementation of the vmSettings API uses inconsistent cases on the names of the json items it returns. | ||
| # To work around that, we use _CaseFoldedDict to query those json items in a case-insensitive matter, Do not use | ||
| # _CaseFoldedDict for other purposes. Remove it once the vmSettings API is updated. |
There was a problem hiding this comment.
We might never be able to remove this given the long tail nature of the HostGA deployments and also the fact that some hosts are never updated/not updated frequently. Just a thought to keep in mind when considering removing this.
There was a problem hiding this comment.
A future version of the API will include a version number and we'll send it over telemetry. That'll help us drive the removal/update of many of those TODO comments I've been adding.
narrieta
dismissed stale reviews from dhivyaganesan and kevinclark19a
via
4e3f55b
No description provided.