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.

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

Enable 12sp5 ltss and ltss_es virt tests #20403

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tbaev
Copy link
Contributor

@tbaev tbaev commented Oct 15, 2024

Variable EXTENDED_SECURITY is set automatically if INCIDENT_REPO conatins ltts or ltss_se repo. No need for new test suites or job groups to run tests.

LTSS:
XEN Migration => http://openqa.qam.suse.cz/tests/81030#dependencies
KVM Migration => http://openqa.qam.suse.cz/tests/81032#dependencies
XEN => http://openqa.qam.suse.cz/tests/81045
KVM => http://openqa.qam.suse.cz/tests/81021
HyperV => https://openqa.suse.de/tests/15732313
VMware => https://openqa.suse.de/tests/15732314

LTSS_ES:
XEN Migration => http://openqa.qam.suse.cz/tests/81028#dependencies
KVM Migration => http://openqa.qam.suse.cz/tests/81025#dependencies
XEN => http://openqa.qam.suse.cz/tests/81033
KVM => http://openqa.qam.suse.cz/tests/81022
HyperV => https://openqa.suse.de/tests/15732311
VMware => https://openqa.suse.de/tests/15732312

@tbaev tbaev added the WIP Work in progress label Oct 15, 2024
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.

@@ -51,7 +51,7 @@
<arch>{{ARCH}}</arch>
<release_type/>
</addon>
% if ($ltss_code) {
% if ($ltss_code && $vm_name !~ /ltsses/) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A minor suggestion, it maybe more name telling with ltss-es? Given that not all know the details of 12sp5 ltss es, suggest to add a comment here to explain a little bit and add the progress ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, ltss-es is better name.

@@ -69,6 +69,9 @@
% if ($key eq 'ltss') {
<reg_code><%= $get_var->('SCC_REGCODE_LTSS') %></reg_code>
% }
% if ($key eq 'ltss_es') {
<reg_code><%= $get_var->('SCC_REGCODE_LTSS_ES') %></reg_code>
Copy link
Contributor

Choose a reason for hiding this comment

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

This will add handling requirement in this task -- adopt MU CI tool A5, https://progress.opensuse.org/issues/160868. I will extend it later.

@@ -131,12 +145,12 @@ if (get_var("REGRESSION", '') =~ /xen/) {
delete $guests{$guest} unless grep { $_ eq $guest } @allowed_guests;
}
} elsif (is_sle('=15-SP5')) {
my @allowed_guests = qw(sles12sp5HVM sles12sp5PV sles15sp5HVM sles15sp5PV sles15sp6HVM sles15sp6PV);
my @allowed_guests = qw(sles12sp5HVM sles12sp5PV sles12sp5ltssesHVM sles12sp5ltssesPV sles15sp5HVM sles15sp5PV sles15sp6HVM sles15sp6PV);
Copy link
Contributor

Choose a reason for hiding this comment

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

As stated in the ticket, only 12sp5 ltss-es host has test for 12sp5 ltss-es guest, any other host does not need to test 12sp5 ltss-es guest.

foreach my $guest (keys %guests) {
delete $guests{$guest} unless grep { $_ eq $guest } @allowed_guests;
}
} elsif (is_sle('=15-SP6')) {
my @allowed_guests = qw(sles12sp5HVM sles12sp5PV sles15sp6HVM sles15sp6PV);
my @allowed_guests = qw(sles12sp5HVM sles12sp5PV sles12sp5ltssesHVM sles12sp5ltssesPV sles15sp6HVM sles15sp6PV);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, no 12sp5 ltss-es guest on 15sp6 host

@@ -111,7 +125,7 @@ if (get_var("REGRESSION", '') =~ /xen/) {
);
# Filter out guests not allowed for the detected SLE version
if (is_sle('=12-SP5')) {
my @allowed_guests = qw(sles12sp5HVM sles12sp5PV sles15sp5HVM sles15sp5PV sles15sp6HVM sles15sp6PV);
my @allowed_guests = qw(sles12sp5HVM sles12sp5PV sles12sp5ltssesHVM sles12sp5ltssesPV sles15sp5HVM sles15sp5PV sles15sp6HVM sles15sp6PV);
Copy link
Contributor

@alice-suse alice-suse Oct 16, 2024

Choose a reason for hiding this comment

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

I am afraid, here differentiation is needed, as given in ticket:

if  (get_var('EXTENDED_SECURITY'))  {
  @allowed_guests = qw(sles12sp5ltssesHVM sles12sp5ltssesPV sles15sp5HVM sles15sp5PV sles15sp6HVM sles15sp6PV);
} else
  @allowed_guests = qw(sles12sp5HVM sles12sp5PV sles15sp5HVM sles15sp5PV sles15sp6HVM sles15sp6PV);
}

@@ -201,7 +222,7 @@ if (get_var("REGRESSION", '') =~ /xen/) {
delete $guests{$guest} unless grep { $_ eq $guest } @allowed_guests;
}
} elsif (is_sle('=12-SP5')) {
my @allowed_guests = qw(sles12sp5 sles15sp5 sles15sp6);
my @allowed_guests = qw(sles12sp5 sles12sp5ltsses sles15sp5 sles15sp6);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here,

  • 12sp5 ltss host has 12sp5 ltss
  • 12sp5 ltss es host has 12sp5 ltss-es guest.

Not both.

@@ -221,12 +242,12 @@ if (get_var("REGRESSION", '') =~ /xen/) {
delete $guests{$guest} unless grep { $_ eq $guest } @allowed_guests;
}
} elsif (is_sle('=15-SP5')) {
my @allowed_guests = qw(sles12sp5 sles15sp5 sles15sp6);
my @allowed_guests = qw(sles12sp5 sles12sp5ltsses sles15sp5 sles15sp6);
Copy link
Contributor

Choose a reason for hiding this comment

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

no ltss-es

foreach my $guest (keys %guests) {
delete $guests{$guest} unless grep { $_ eq $guest } @allowed_guests;
}
} elsif (is_sle('=15-SP6')) {
my @allowed_guests = qw(sles12sp5 sles15sp6);
my @allowed_guests = qw(sles12sp5 sles12sp5ltsses sles15sp6);
Copy link
Contributor

Choose a reason for hiding this comment

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

no ltss-es

@@ -243,6 +264,9 @@ if (get_var("REGRESSION", '') =~ /xen/) {
sles12sp5 => {
name => 'sles12sp5',
},
sles12sp5ES => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If all the vm names of 12sp5 ltss-es can align with this one, it will be be more aligned in naming style ;). This is optional ofc.

@@ -29,6 +29,7 @@ sub create_profile {
my $path = $version >= 15 ? "virtualization/autoyast/guest_15.xml.ep" : "virtualization/autoyast/guest_12.xml.ep";
my $scc_code = get_required_var("SCC_REGCODE");
my %ltss_products = @{get_var_array("LTSS_REGCODES_SECRET")};
my %ltss_es_products = @{get_var_array("SCC_REGCODE_LTSS_ES")};
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need corresponding handling in adopt CI tools A5 task too. I will do.

Copy link
Contributor

@alice-suse alice-suse left a comment

Choose a reason for hiding this comment

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

@tbaev Thanks for providing this. I gave some comments. Some may need necessary changes, while some are optional for coding style. Please have a look. Feel free to reply if anything unclear.

Besides, I guess this will also need adjustment in schedule_issue.py? Because 12sp5 ltss es updates will trigger 12sp5 ES host jobs, while 12sp5 ltss will trigger regular 12sp5 ltss host tests?

Also in your overall solution, would you please explain how you differentiate the ES and NON-ES test/testsuite? Additional testsuite introduced? Otherwise if sharing the same testsuite, some logic in openqa code shall be here to dynamically set EXTENDED_SECURITY? To be honest, I would suggest more the latter way, otherwise the adopt MU CI tools relevant tasks would need many adjustments for additional testsuite which is avoidable.

@tbaev
Copy link
Contributor Author

tbaev commented Oct 16, 2024

I have created a new flavor "flavor: Server-DVD-Virt-XEN-Incidents-Extended_Security" and job group "SLE 12 SP5 ES Virt XEN Incidents", but it is easier for MU CI tools task to use the old ones I can change the logic.

If I understand correctly it will be best to change it like this: run 12SP5ltss if EXTENDED_SECURITY="" and will run ltts_es if EXTENDED_SECURITY="12.5"

@alice-suse
Copy link
Contributor

I have created a new flavor "flavor: Server-DVD-Virt-XEN-Incidents-Extended_Security" and job group "SLE 12 SP5 ES Virt XEN Incidents", but it is easier for MU CI tools task to use the old ones I can change the logic.

Yep, that will be so considerate. Thanks!

If I understand correctly it will be best to change it like this: run 12SP5ltss if EXTENDED_SECURITY="" and will run ltts_es if EXTENDED_SECURITY="12.5"

Yes, EXTENDED_SECURITY can be a flag var -- 1 or 0, the version can be told from scheduled product's VERSION. It can scale well if in future there are other ES :) .

@tbaev
Copy link
Contributor Author

tbaev commented Oct 16, 2024

Okay, I have a suspicion that maybe more ES version are coming and wanted to made it easy to add future ones.
Thank you for the corrections/advises.

@alice-suse
Copy link
Contributor

alice-suse commented Oct 16, 2024

Okay, I have a suspicion that maybe more ES version are coming and wanted to made it easy to add future ones. Thank you for the corrections/advises.

@tbaev Sorry, Teodor, I replied too fast about flavor for ES flavor, please let me think about it more overall. Will reply ASAP.

@alice-suse
Copy link
Contributor

alice-suse commented Oct 16, 2024

Okay, I have a suspicion that maybe more ES version are coming and wanted to made it easy to add future ones. Thank you for the corrections/advises.

@tbaev Sorry, Teodor, I replied too fast about flavor for ES flavor, please let me think about it more overall. Will reply ASAP.

Hi @tbaev , after more careful consideration, I think

  • it is fine that in your current solution, you create new flavor and job group, although IMHO, it is optional, reusing existing ones would be fine. In adopt MU CI tool task, I will need new flavors for LTSS-ES anyway, which comes from requirement of bot, so similar case with TERADATA. And actually I would need 5 new flavors, which will be independent with your flavor/job group for this task.
  • it will be good that NO NEW TESTSUITE is created, so same testsuites can be shared between other products and 12sp5 LTSS-ES, then
    • EXTENDED_SECURITY(a flag var) can be set within openqa test code here by judging if the incident repo contains key word 'LTSS-Extended-Security' based on this

@tbaev
Copy link
Contributor Author

tbaev commented Oct 16, 2024

@alice-suse EXTENDED_SECURITY variable is set by checking the incident repo urls. Using only EXTENDED_SECURITY variable test is set to run ltss or ltss_es version. With this change I think we don't need new flavors/test suites.

@tbaev
Copy link
Contributor Author

tbaev commented Oct 16, 2024

@alice-suse one more thing , becouse only 12sp5ltss-se is tested on sles12sp5ltss-es host, I can remove sles12sp5ltss-es names and just use sles12sp5. Will make that change too.

@tbaev tbaev force-pushed the 12sp5ltss_virt branch 3 times, most recently from 0fd7464 to fa8132c Compare October 16, 2024 20:38
@alice-suse
Copy link
Contributor

alice-suse commented Oct 17, 2024

@alice-suse one more thing , becouse only 12sp5ltss-se is tested on sles12sp5ltss-es host, I can remove sles12sp5ltss-es names and just use sles12sp5. Will make that change too.

@tbaev 12sp5ltss-se is tested on sles12sp5ltss-es host, vmware host, and hyperv host, similar with TERADATA. I guess your above statement is for sles host case, right? So basically similar with TERADATA, for kvm/xen hosts, only one 12sp5 guest, while for vmware/hyperv, two separate names, one with ES, one without ES. Right? I think this is good and more aligned.

@@ -51,7 +51,7 @@
<arch>{{ARCH}}</arch>
<release_type/>
</addon>
% if ($ltss_code) {
% if ($ltss_code && $check_var->('EXTENDED_SECURITY', '0') || $ltss_code && !$ltss_es_code && $check_var->('EXTENDED_SECURITY', '1')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What to be checked here ? A && B || C && !D && E. This checking looks not scientific enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A && B = sle12 guest with ltss keys will use ltss key to register, when we are not testing ltss_es.
C && !D && E = When testing ltss_es we will register all sle12 ltss guests that have ltss key, except the guest with ltss_se key

But on second look I think C && !D && E is redundant. Only sles12 guest we test when EXTENDED_SECURITY=1 is sles12sp5. I will remove C && !D && E .

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you implement when you want. Also add parentheses to make it better if necessary.

@tbaev
Copy link
Contributor Author

tbaev commented Oct 17, 2024

@alice-suse Yes, statement is for sles host case. Names for sles hosts are sles12sp5HVM and names for hyperV/VMware guests are with ES suffix.

@tbaev tbaev force-pushed the 12sp5ltss_virt branch 3 times, most recently from ed72ce9 to 182ba40 Compare October 18, 2024 20:34
@tbaev tbaev removed the WIP Work in progress label Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants