Reorganize the history directory #2520
Conversation
narrieta
requested review from
larohra,
kevinclark19a,
ZhidongPeng,
dhivyaganesan and
nagworld9
as code owners
| @@ -38,7 +38,7 @@ | |||
|
|
|||
| ARCHIVE_DIRECTORY_NAME = 'history' | |||
|
|
|||
| _MAX_ARCHIVED_STATES = 100 | |||
| _MAX_ARCHIVED_STATES = 50 | |||
There was a problem hiding this comment.
Previously I increased this value from 50 to 100 because I was saving waagent_status.json to a separate directory. Since now I am saving it to the same directory as the goal state, I am reverting the value to the original 50
Codecov Report
@@ Coverage Diff @@
## develop #2520 +/- ##
===========================================
- Coverage 71.94% 71.91% -0.04%
===========================================
Files 101 102 +1
Lines 15147 15135 -12
Branches 2401 2400 -1
===========================================
- Hits 10898 10884 -14
+ Misses 3771 3769 -2
- Partials 478 482 +4
Continue to review full report at Codecov.
|
| timestamp = create_timestamp() | ||
| xml_text, xml_doc, incarnation = GoalState._fetch_goal_state(self._wire_client) | ||
| self._history = GoalStateHistory(timestamp, incarnation) | ||
|
|
There was a problem hiding this comment.
the main change here is intantiating the history during init; the rest of the changes are just moving the code around to ensure all properties are defined before invoking any method
| raise | ||
| except ResourceGoneError: | ||
| # retry after refreshing the HostGAPlugin | ||
| GoalState.update_host_plugin_headers(self._wire_client) | ||
| vm_settings, vm_settings_updated = self._wire_client.get_host_plugin().fetch_vm_settings(force_update=force_update) | ||
|
|
||
| if vm_settings_updated: |
There was a problem hiding this comment.
now the vmSettings are added to the history by the caller
| }} | ||
| '''.format(status_blob_text, debug_info) | ||
|
|
||
| fileutil.write_file(status_file, status_file_text) |
There was a problem hiding this comment.
now the file is created by the caller, and this code is exposed as get_ext_handlers_status_debug_info
| # else just ensure the extensions are using the latest vm_settings | ||
| if vm_settings is not None: | ||
| # else ensure the extensions are using the latest vm_settings | ||
| timestamp = create_timestamp() |
There was a problem hiding this comment.
why do we need different timestamp? can't we use L#135
There was a problem hiding this comment.
L#136 calls _fetch_goal_state, which does a network call, which can have delays. we need a new timestamp here
|
|
||
| vm_settings = self._fetch_vm_settings(force_update=force_update) | ||
| timestamp = create_timestamp() | ||
| xml_text, xml_doc, incarnation = GoalState._fetch_goal_state(self._wire_client) |
There was a problem hiding this comment.
where/when is this full goal state saved to history folder?
|
|
||
| def _fetch_extended_goal_state(self, xml_text, xml_doc, vm_settings): | ||
| def _fetch_extended_goal_state(self, xml_text, xml_doc, force_vm_settings_update=False): |
There was a problem hiding this comment.
NIT: maybe rename to fetch_complete_goal_state? Extended seemed a bit confusing to me personally
There was a problem hiding this comment.
in init, I split the member variables in 2 blocks, which I called "basic" and "extended", then initialize_basic_properties initializes the former and fetch_extended_goal_state the latter (I did not name fetch_extended_goal_state "initialize_extended_properties" because there is a fetch operation in it and I want that to be explicit)
| @@ -175,16 +170,17 @@ def _fetch_goal_state(wire_client): | |||
|
|
|||
| # In some environments a few goal state requests return a missing RoleInstance; these retries are used to work around that issue | |||
| # TODO: Consider retrying on 410 (ResourceGone) as well | |||
| incarnation = "unknown" | |||
There was a problem hiding this comment.
Will this ever be "unknown"? It should always get updated right?
There was a problem hiding this comment.
the linter believes it can be used before initialization at line #184. that would happen, i believe, only if _GET_GOAL_STATE_MAX_ATTEMPTS is 0, but I decided to go ahead an initialize it anyways
| except Exception as e: | ||
| if not self._errors: # report only 1 error per directory | ||
| self._errors = True | ||
| logger.warn("Failed to save goal state file {0}: {1} [no additional errors saving the goal state will be reported]".format(target_name, e)) | ||
| logger.warn("Failed to save goal state file {0}: {1} [no additional errors saving the goal state will be reported]".format(file_name, e)) |
There was a problem hiding this comment.
Maybe this error message needs updating? Failed to save {file_name} for incarnation {id} [no additional errors saving the goal state will be reported]: {error}
There was a problem hiding this comment.
i see... by "goal state file" i did not mean GoalState.xml, but a file related to the goal state... i'll update this to "Failed to save {0} to the goal state history: {1}" (the incarnation or etag are already logged 1 or 2 lines above this message)
|
|
||
| return json.dumps(debug_info) | ||
| return '''{{ | ||
| "__comment__": "The __status__ property is the actual status reported to CRP", |
There was a problem hiding this comment.
NIT: Indent missing for better visuals
There was a problem hiding this comment.
I did not want extra indentation in the actual file, so I added only 4 leading spaces
| self._report_extensions_summary(vm_status) | ||
| if self._goal_state is not None: | ||
| agent_status = exthandlers_handler.get_ext_handlers_status_debug_info(vm_status) | ||
| self._goal_state.save_to_history(agent_status, AGENT_STATUS_FILE) |
There was a problem hiding this comment.
maybe create an abstraction for this too? i.e. self._goal_state.save_reported_status() or something?
There was a problem hiding this comment.
currently i'm using save_to_history to save the manifests as well, i'm planning on getting rid of it in another round of refactoring
The changes I did to the history directory did not capture the goal state the way I intended to. I wanted to capture all the HTTP requests for needed to process a single goal state within the same directory. Some goal states were producing more than 1 directory, plus I was capturing the waagent_status.json in a separate directory. All of that made the history hard to read.
This PR addresses those issues. There is a single directory per goal state, be it Fabric or FastTrack, and the last status for a given goal state is captured in the directory for that goal state.
This is a sample directory, and the contents of 2 sample subdirectories (the "_*" suffix indicates the incarnation for a Fabric goal state, or the eTag for a FastTrack goal state.