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

Enable AITL for latest Azure images #20649

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Enable AITL for latest Azure images #20649

wants to merge 5 commits into from
+201 −0

Conversation

linguini-dev
Copy link
Contributor

@linguini-dev linguini-dev commented Nov 15, 2024 •
edited
Loading

This PR enables AITL (Azure Image Testing for Linux). This test module creates AITL Jobs in Azure.

@github-actions GitHub Actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files.

@linguini-dev linguini-dev changed the title WIP Enable AITL for latest Azure images Enable AITL for latest Azure images Nov 27, 2024
@linguini-dev linguini-dev marked this pull request as ready for review November 27, 2024 15:57
ricardobranco777
ricardobranco777 reviewed Nov 27, 2024
View reviewed changes
tests/publiccloud/azure_aitl.pm
@@ -0,0 +1,133 @@
# SUSE's openQA tests
#
# Copyright 2021-2024 SUSE LLC
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Copyright 2021-2024 SUSE LLC
# Copyright 2024 SUSE LLC

tests/publiccloud/azure_aitl.pm
assert_script_run("sed -i -e 's/<IMAGE_NAME>/$aitl_image_name/g' -e 's/<IMAGE_VERSION>/$aitl_image_version/g' -e 's/<IMAGE_GALLERY_NAME>/$aitl_image_gallery/g' /tmp/$aitl_manifest");

# Create AITL Job based on a manifest
script_output("cat /tmp/$aitl_manifest");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to use record_info here. Otherwise there's no reason to call script_output here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think I added this just as a debugging step and it can be removed altogether.

asmorodskyi
asmorodskyi reviewed Nov 27, 2024
View reviewed changes
data/publiccloud/aitl/shared_gallery.json
"selections": [
{
"caseName": [
"verify_connection_status",
Copy link
Member

Choose a reason for hiding this comment

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

this list means for me that if MS will add new test or remove existing we will need to add/remove it from here 🤔 maybe there is a way to say "Run whatever you have!" ? or maybe there is a reason why we don't want to do that ?

Copy link
Member

Choose a reason for hiding this comment

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

this should not block merge just question for future improvements ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is not option to run whatever (at the time of writing). A test manifest must be specified and it has some parameters. They have added 2 new tests since I wrote this, so your gut feeling is correct :)

As a side note - we want to keep track if the tests pass or not, and adding new tests without our knowledge (because MS updated something) can be annoying to debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might make sense to import the tests based on Tiers instead of by caseName. Looking into that.

asmorodskyi
asmorodskyi reviewed Nov 27, 2024
View reviewed changes
lib/main_publiccloud.pm
@@ -160,6 +162,7 @@ sub load_latest_publiccloud_tests {
}
elsif (get_var('PUBLIC_CLOUD_UPLOAD_IMG')) {
loadtest "publiccloud/upload_image", run_args => $args;
loadtest "publiccloud/azure_aitl", run_args => $args;
Copy link
Member

Choose a reason for hiding this comment

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

this line needs to be removed , I assume you simply forgot to tweak this part ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. The whole test needs to be moved elsewhere, I forgot to move it.

asmorodskyi
asmorodskyi reviewed Nov 27, 2024
View reviewed changes
tests/publiccloud/azure_aitl.pm
my $job_id = get_current_job_id();

# If 'az' is preinstalled, we test that version
if (script_run("which az") != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

we need to discuss this part , we should rely on the fact that tools image provide everything what you need

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied from the azure_cli test module, so let's discuss.

asmorodskyi
asmorodskyi requested changes Nov 27, 2024
View reviewed changes
Copy link
Member

@asmorodskyi asmorodskyi left a comment

Choose a reason for hiding this comment

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

generally all looks good but there are two major issues which we need to solve before it can be merged :

  1. triggering logic needs update (main_publiccloud)
  2. logic about az installation have several problems ( let's discuss it in person )

asmorodskyi
asmorodskyi reviewed Nov 27, 2024
View reviewed changes
tests/publiccloud/azure_aitl.pm
my $provider = $self->provider_factory();

my $image_url = get_required_var('PUBLIC_CLOUD_IMAGE_LOCATION');
my $region = get_var('PUBLIC_CLOUD_REGION', 'westeurope');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
my $region = get_var('PUBLIC_CLOUD_REGION', 'westeurope');
my $region = get_var('PUBLIC_CLOUD_REGION');

this is design decision done on purpose . It is responsibility of triggering logic / job group settings to define in which region test will run . And if region is not defined test MUST fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but in the specific instance for AITL, it kind of doesn't matter.

I couldn't get AITL jobs running anywhere but US (westus1,2,3 and etc.) so for the time being, the region is hardcoded.

@linguini-dev
Copy link
Contributor Author

I caught some issues with the While loop not triggering and not generatin the Failed xml correctly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asmorodskyi asmorodskyi asmorodskyi requested changes

@ricardobranco777 ricardobranco777 ricardobranco777 left review comments

@pdostal pdostal Awaiting requested review from pdostal pdostal is a code owner

@grisu48 grisu48 Awaiting requested review from grisu48 grisu48 is a code owner

@m-dati m-dati Awaiting requested review from m-dati m-dati is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@linguini-dev @ricardobranco777 @asmorodskyi