-
Notifications
You must be signed in to change notification settings - Fork 366
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
Logging a disable error instead of Download error on extension update… #1616
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,7 +41,7 @@ | |
from azurelinuxagent.common.errorstate import ErrorState, ERROR_STATE_DELTA_INSTALL | ||
from azurelinuxagent.common.event import add_event, WALAEventOperation, elapsed_milliseconds, report_event | ||
from azurelinuxagent.common.exception import ExtensionError, ProtocolError, ProtocolNotFoundError, \ | ||
ExtensionDownloadError, ExtensionOperationError, ExtensionErrorCodes | ||
ExtensionDownloadError, ExtensionOperationError, ExtensionErrorCodes, ExtensionUpdateError | ||
from azurelinuxagent.common.future import ustr | ||
from azurelinuxagent.common.protocol import get_protocol_util | ||
from azurelinuxagent.common.protocol.restapi import ExtHandlerStatus, \ | ||
|
@@ -436,6 +436,9 @@ def handle_ext_handler(self, ext_handler, etag): | |
else: | ||
message = u"Unknown ext handler state:{0}".format(state) | ||
raise ExtensionError(message) | ||
except ExtensionUpdateError as e: | ||
# Only setting the handler status here as the error has already been reported from the old version | ||
ext_handler_i.set_handler_status(message=ustr(e), code=e.code) | ||
except ExtensionOperationError as e: | ||
self.handle_ext_handler_error(ext_handler_i, e, e.code) | ||
except ExtensionDownloadError as e: | ||
|
@@ -476,20 +479,45 @@ def handle_enable(self, ext_handler_i): | |
if old_ext_handler_i is None: | ||
ext_handler_i.install() | ||
elif ext_handler_i.version_ne(old_ext_handler_i): | ||
old_ext_handler_i.disable() | ||
ext_handler_i.copy_status_files(old_ext_handler_i) | ||
if ext_handler_i.version_gt(old_ext_handler_i): | ||
ext_handler_i.update() | ||
else: | ||
old_ext_handler_i.update(version=ext_handler_i.ext_handler.properties.version) | ||
old_ext_handler_i.uninstall() | ||
old_ext_handler_i.remove_ext_handler() | ||
ext_handler_i.update_with_install() | ||
ExtHandlersHandler._update_extension_handler(old_ext_handler_i, ext_handler_i) | ||
larohra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
else: | ||
ext_handler_i.update_settings() | ||
|
||
ext_handler_i.enable() | ||
|
||
@staticmethod | ||
def _update_extension_handler(old_ext_handler_i, ext_handler_i): | ||
|
||
def execute_old_handler_command(ext_handler_i, op): | ||
""" | ||
Created a common function to execute all commands that need to be executed from the old handler | ||
so that it can have a common exception handling mechanism | ||
:param ext_handler_i: The extension handler instance to run the operation on | ||
:param op: Which operation to run | ||
""" | ||
try: | ||
if op == WALAEventOperation.Disable: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: if you pass a lambda then you don't need the switch on operation code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! Didnt know about it, will implement it! |
||
ext_handler_i.disable() | ||
elif op == WALAEventOperation.UnInstall: | ||
# Currently uninstall failures swallows the exception so we wont catch anything here | ||
ext_handler_i.uninstall() | ||
except ExtensionError as e: | ||
# Reporting the event with the old handler and raising a new ExtensionUpdateError to set the | ||
# handler status on the new version | ||
msg = ustr(e) | ||
ext_handler_i.report_event(message=msg, is_success=False) | ||
raise ExtensionUpdateError(msg) | ||
|
||
execute_old_handler_command(old_ext_handler_i, WALAEventOperation.Disable) | ||
ext_handler_i.copy_status_files(old_ext_handler_i) | ||
if ext_handler_i.version_gt(old_ext_handler_i): | ||
ext_handler_i.update() | ||
else: | ||
old_ext_handler_i.update(version=ext_handler_i.ext_handler.properties.version) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wow! we support extension downgrade? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha yes, this is at par with Windows. Not sure how much its been tested though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would you open a work item to test it and double-check whether the extension publishers doc talks about this? i'm sure some extensions publishers will be surprised (let's also check whether the VERSION env variable is documented). Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opened a task. Will follow up later about this |
||
execute_old_handler_command(old_ext_handler_i, WALAEventOperation.UnInstall) | ||
old_ext_handler_i.remove_ext_handler() | ||
ext_handler_i.update_with_install() | ||
|
||
def handle_disable(self, ext_handler_i): | ||
self.log_process = True | ||
handler_state = ext_handler_i.get_handler_state() | ||
|
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 works fine now, but if we ever add/change logic in handle_ext_handler_error (which currently is the central place to handle errors) we may miss this one... consider adding a flag to handle_ext_handler_error to do/not do the logging and call it from here as well.
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.
Makes sense, I'll modify the code