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.

Sign up for GitHub

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

Jump to bottom

Bug fixes for end-to-end tests #2820

Merged
merged 1 commit into from
May 16, 2023
Merged

Bug fixes for end-to-end tests #2820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions tests_e2e/orchestrator/lib/agent_test_suite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ def _initialize(self, node: Node, variables: Dict[str, Any], lisa_working_path:

self.__context = self._Context(
vm=VmIdentifier(
cloud=self._get_required_parameter(variables, "c_cloud"),
Copy link
Member Author

Choose a reason for hiding this comment

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

The 'c_cloud' variable was not being passed to runbooks when running on an existing VM.

While fixing this I realized there is no need for this variable, since the AgentTestSuite and AgentTestSuiteCombinator can just use 'cloud' directly from the runbook.

cloud=self._get_required_parameter(variables, "cloud"),
location=self._get_required_parameter(variables, "c_location"),
subscription=node.features._platform.subscription_id,
resource_group=node_context.resource_group_name,
Expand Down Expand Up @@ -454,8 +454,6 @@ def _execute_test_suite(self, suite: TestSuiteInfo) -> bool:
suite_full_name = f"{suite_name}-{self.context.environment_name}"
suite_start_time: datetime.datetime = datetime.datetime.now()

success: bool = True # True if all the tests succeed
Copy link
Member Author

Choose a reason for hiding this comment

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

this variable was a leftover of a previous iteration of the code and _execute_test_suite was always returning True (i.e. Success, so VM logs were not being collected on test failures)


with _set_thread_name(suite_full_name): # The thread name is added to the LISA log
log_path: Path = self.context.log_path/f"{suite_full_name}.log"
with set_current_thread_log(log_path):
Expand Down Expand Up @@ -550,7 +548,7 @@ def _execute_test_suite(self, suite: TestSuiteInfo) -> bool:
if not suite_success:
self._mark_log_as_failed()

return success
return suite_success

def _check_agent_log(self) -> bool:
"""
Expand Down
1 change: 0 additions & 1 deletion tests_e2e/orchestrator/lib/agent_test_suite_combinator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ def create_environment(c_env_name: str) -> Dict[str, Any]:
c_vm_tags["templates"] = suite_info.template
return {
"c_marketplace_image": c_marketplace_image,
"c_cloud": self.runbook.cloud,
"c_location": c_location,
"c_vm_size": c_vm_size,
"c_vhd": c_vhd,
Expand Down
4 changes: 1 addition & 3 deletions tests_e2e/orchestrator/runbook.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ variable:
value: "agent_bvt, no_outbound_connections"
- name: cloud
value: "AzureCloud"
is_case_visible: true
- name: image
value: ""
- name: location
Expand Down Expand Up @@ -82,9 +83,6 @@ variable:
value: ""
- name: c_vm_size
value: ""
- name: c_cloud
value: ""
is_case_visible: true
- name: c_location
value: ""
is_case_visible: true
Expand Down
4 changes: 1 addition & 3 deletions tests_e2e/orchestrator/sample_runbooks/existing_vm.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ variable:
#
- name: cloud
value: "AzureCloud"
is_case_visible: true
- name: subscription_id
value: ""
- name: resource_group_name
Expand Down Expand Up @@ -88,9 +89,6 @@ variable:
value: ""
- name: c_shared_resource_group_location
value: ""
- name: c_cloud
value: ""
is_case_visible: true
- name: c_location
value: ""
is_case_visible: true
Expand Down
4 changes: 1 addition & 3 deletions tests_e2e/tests/lib/vm_extension.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,7 @@ def enable(
extension_parameters
).result(timeout=_TIMEOUT))

if result.provisioning_state not in ('Succeeded', 'Updating'):
Copy link
Member Author

Choose a reason for hiding this comment

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

I copied this check from the original prototype, but it has been producing intermittent failures. Depending on the timing, it can be Succeeded, Updating or just None. I double-checked that on extension failure the call to result() in the previous line will raise an exception anyway, so this check is actually not needed and now we just log an INFO.

raise Exception(f"Enable {self._identifier} failed. Provisioning state: {result.provisioning_state}")
log.info("Enable completed (provisioning state: %s).", result.provisioning_state)
log.info("Enable completed. Provisioning state: %s", result.provisioning_state)

def get_instance_view(self) -> VirtualMachineExtensionInstanceView: # TODO: Check type for scale sets
"""
Expand Down