Skip to content

Commit

Permalink
Dont create default status file for Single-Config extensions (#2318)
Browse files Browse the repository at this point in the history
  • Loading branch information
larohra authored Aug 3, 2021
1 parent 92afd7f commit 615d48a
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 28 deletions.
19 changes: 5 additions & 14 deletions azurelinuxagent/ga/exthandlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -735,7 +735,11 @@ def handle_enable(self, ext_handler_i, extension):
# failures back to CRP. If a placeholder for an extension already exists with Transitioning status, we would
# not override it, hence we only create a placeholder for enable/disable commands but the extensions have the
# data to create their own if needed.
if ext_handler_i.should_create_default_placeholder(extension):

# Note: Due to a bug in multiple extensions, we're only creating a default placeholder for Multi-Config extensions.
# A fix will follow soon where we will report transitioning status for extensions by default if no status file
# found instead of reporting an error.
if ext_handler_i.should_perform_multi_config_op(extension):
ext_handler_i.create_placeholder_status_file(extension)
self.__handle_extension(ext_handler_i, extension, uninstall_exit_code)

Expand Down Expand Up @@ -1399,19 +1403,6 @@ def initialize(self):
# Save HandlerEnvironment.json
self.create_handler_env()

def should_create_default_placeholder(self, extension=None):
"""
There's a bug in the AKS extension where they dont update the status file if it exists and another bug in
LinuxPatchExtension where they inherently have dependency on creating the status file first.
This violates the contract we have with extensions. Until they fix their extensions,
we're going to skip creating a placeholder for them to ensure they dont have any downtime.
For all other extensions, we should create a
"""

ignore_extensions_regex = [r"Microsoft.AKS.Compute.AKS\S*", r"Microsoft.CPlat.Core.LinuxPatchExtension\S*"]
return all(re.match(ext_regex, self.get_extension_full_name(extension)) is None
for ext_regex in ignore_extensions_regex)

def create_placeholder_status_file(self, extension=None, status=ValidHandlerStatus.transitioning, code=0,
operation="Enabling Extension", message="Install/Enable is in progress."):
_, status_path = self.get_status_file_path(extension)
Expand Down
13 changes: 5 additions & 8 deletions tests/ga/test_extension.py
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@ def _assert_handler_status(self, report_vm_status, expected_status,
self.assertNotEqual(0, len(vm_status.vmAgent.extensionHandlers))
handler_status = next(
status for status in vm_status.vmAgent.extensionHandlers if status.name == expected_handler_name)
self.assertEqual(expected_status, handler_status.status)
self.assertEqual(expected_status, handler_status.status, get_properties(handler_status))
self.assertEqual(expected_handler_name, handler_status.name)
self.assertEqual(version, handler_status.version)
self.assertEqual(expected_ext_count, len([ext_handler for ext_handler in vm_status.vmAgent.extensionHandlers if
Expand Down Expand Up @@ -1639,9 +1639,9 @@ def mock_popen(cmd, *args, **kwargs):
if "sample.py" in cmd:
status_path = os.path.join(kwargs['env'][ExtCommandEnvVariable.ExtensionPath], "status",
"{0}.status".format(kwargs['env'][ExtCommandEnvVariable.ExtensionSeqNumber]))
mock_popen.deleted_status_file = status_path
if os.path.exists(status_path):
os.remove(status_path)
mock_popen.deleted_status_file = status_path
return original_popen(["echo", "Yes"], *args, **kwargs)

with patch('azurelinuxagent.common.cgroupapi.subprocess.Popen', side_effect=mock_popen):
Expand All @@ -1664,7 +1664,7 @@ def mock_popen(cmd, *args, **kwargs):
expected_msg="Dependent Extension OSTCExtensions.OtherExampleHandlerLinux did not reach a terminal state within the allowed timeout. Last status was {0}".format(
ValidHandlerStatus.warning))

def test_it_should_not_create_placeholder_for_aks_extension(self, mock_http_get, mock_crypt_util, *args):
def test_it_should_not_create_placeholder_for_single_config_extensions(self, mock_http_get, mock_crypt_util, *args):
original_popen = subprocess.Popen

def mock_popen(cmd, *_, **kwargs):
Expand All @@ -1677,10 +1677,7 @@ def mock_popen(cmd, *_, **kwargs):
ext_path = kwargs['env'][ExtCommandEnvVariable.ExtensionPath]
status_file_name = "{0}.status".format(seq_no)
status_file = os.path.join(ext_path, "status", status_file_name)
if "AKS" in cmd:
self.assertFalse(os.path.exists(status_file), "Placeholder file should not be created for AKS")
else:
self.assertTrue(os.path.exists(status_file), "Placeholder file should be created for all extensions")
self.assertFalse(os.path.exists(status_file), "Placeholder file should not be created for single config extensions")

return original_popen(cmd, *_, **kwargs)

Expand Down Expand Up @@ -1732,7 +1729,7 @@ def mock_popen(cmd, *args, **kwargs):
"{0}.status".format(kwargs['env'][ExtCommandEnvVariable.ExtensionSeqNumber]))
invalid_json_path = os.path.join(data_dir, "ext", "sample-status-invalid-json-format.json")

if os.path.exists(status_path):
if 'enable' in cmd:
invalid_json = fileutil.read_file(invalid_json_path)
fileutil.write_file(status_path,invalid_json)

Expand Down
9 changes: 3 additions & 6 deletions tests/ga/test_multi_config_extension.py
Original file line number Diff line number Diff line change
Expand Up @@ -894,7 +894,7 @@ def test_it_should_ignore_disable_errors_for_multi_config_extensions(self):
fail_code in kwargs['message'] for args, kwargs in patch_report_event.call_args_list if
kwargs['name'] == third_ext.name), "Error not reported")

def test_it_should_always_create_placeholder_for_all_extensions(self):
def test_it_should_always_create_placeholder_for_multi_config_extensions(self):
original_popen = subprocess.Popen
handler_statuses = {}

Expand All @@ -903,9 +903,6 @@ def __assert_status_file_in_handlers():
file_name = "{0}.{1}.status".format(handler['runtimeSettingsStatus']['extensionName'],
handler['runtimeSettingsStatus']['sequenceNumber'])
__assert_status_file(handler['handlerName'], status_file=file_name)
for handler in sc_handler:
file_name = "{0}.status".format(handler['runtimeSettingsStatus']['sequenceNumber'])
__assert_status_file(handler['handlerName'], status_file=file_name)

def __assert_status_file(handler_name, status_file):
status = handler_statuses["{0}.{1}.enable".format(handler_name, status_file)]
Expand Down Expand Up @@ -938,7 +935,7 @@ def mock_popen(cmd, *_, **kwargs):
"ext_conf_multi_config_no_dependencies.xml")
with self._setup_test_env(mock_manifest=True) as (exthandlers_handler, protocol, no_of_extensions):
with patch('azurelinuxagent.common.cgroupapi.subprocess.Popen', side_effect=mock_popen):
mc_handlers, sc_handler = self.__run_and_assert_generic_case(exthandlers_handler, protocol,
mc_handlers, _ = self.__run_and_assert_generic_case(exthandlers_handler, protocol,
no_of_extensions)

# Ensure we dont create a placeholder for Install command
Expand All @@ -953,7 +950,7 @@ def mock_popen(cmd, *_, **kwargs):
__assert_status_file_in_handlers()

# Update GS, remove 2 extensions and add 3
mc_handlers, sc_handler = self.__setup_and_assert_disable_scenario(exthandlers_handler, protocol)
mc_handlers, _ = self.__setup_and_assert_disable_scenario(exthandlers_handler, protocol)
__assert_status_file_in_handlers()

def test_it_should_report_status_correctly_for_unsupported_goal_state(self):
Expand Down

0 comments on commit 615d48a

Please sign in to comment.