-
Notifications
You must be signed in to change notification settings - Fork 371
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
Add distros and location to the test suites definition files #2756
Conversation
@@ -4,7 +4,7 @@ | |||
# Runs every 3 hours and deletes any resource groups that are more than a day old and contain string "lisa-WALinuxAgent-" | |||
# | |||
schedules: | |||
- cron: "0 */3 * * *" # Run every 3 hours | |||
- cron: "0 */12 * * *" # Run twice a day (every 12 hours) |
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.
I decreased the frequency of cleanup. Twice a day should be enough, we can adjust later if needed.
Codecov Report
@@ Coverage Diff @@
## develop #2756 +/- ##
========================================
Coverage 71.99% 71.99%
========================================
Files 104 104
Lines 15831 15831
Branches 2264 2264
========================================
Hits 11397 11397
Misses 3913 3913
Partials 521 521 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
"marketplace_image": image.urn, | ||
"location": location, | ||
"vm_size": vm_size, | ||
"vhd": "", |
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.
support for VHDs will come in a separate PR
# | ||
# ubuntu_2004: "Canonical 0001-com-ubuntu-server-focal 20_04-lts latest" | ||
# | ||
# or by an object with 3 properties: urn, locations and vm_sizes, as in |
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.
Can we provide location or vm_size? Looks like we'll need to provide both or neither
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.
Mariner 2 ARM64 provides both, but they are independent. Most images will omit both, a few images will set a location, and even fewer will set both a location and vm_size (or just a vm_size).
DCR doesn't allow to specify the VM size for an image. This is a new requirement needed to support Mariner 2 ARM64, currently that image has very limited availability.
# now load the image-sets, mapping them to the images that we just computed | ||
for image_set_name, image_list in image_descriptions["image-sets"].items(): | ||
# the same name cannot denote an image and an image-set | ||
if image_set_name in images: |
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.
We currently only have one image_set which is 'endorsed', is that correct?
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.
for now, yes. the bvts use 'endorsed'. dcr has many more images and image sets, but i don't want to migrate them in bulk. we'll migrate them as we migrate the tests that actually used them.
) | ||
|
||
|
||
class AgentTestSuitesCombinator(Combinator): |
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.
when is this invoked?
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.
It is invoked by LISA.
LISA will invoke the combinator, whose output feeds the the values needed to create the test VM
platform:
- type: azure
....
azure:
marketplace: $(marketplace_image)
vhd: $(vhd)
location: $(location)
vm_size: $(vm_size)
e['marketplace_image'], e['location'], e['vm_size'], e['vhd'], [s.name for s in e['test_suites_info']]) | ||
log.info("***************************") | ||
|
||
return environment_list |
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.
who gets this return list?
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.
based on this image list, lisa creates that many vms?
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.
The runbook uses the values returned by the combinator to feed the values used to create the VMs (see my previous comment above)
for suite in test_suites: | ||
# pylint seems to think self.context.test_suites is not iterable. Suppressing warning, since its type is List[AgentTestSuite] | ||
# E1133: Non-iterable value self.context.test_suites is used in an iterating context (not-an-iterable) | ||
for suite in self.context.test_suites: # pylint: disable=E1133 | ||
test_suite_success = self._execute_test_suite(suite) and test_suite_success |
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.
how this suite type as TestSuiteInfo. I see self.context.test_suites
get assigned from the variable we pass to Lisa run. That would be list of commas separated or dictionary type of single test.
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.
The string we pass to the runbook is actually an argument for the combinator:
#
# These variables parameters for the AgentTestSuitesCombinator combinator
#
# The test suites to execute
- name: test_suites
value: "agent_bvt"
is_case_visible: true
Then the combinator produces an array of TestSuiteInfos that is passed to the AgentTestSuite in the "test_suites_info" variable:
#
# These variables are set by the AgentTestSuitesCombinator combinator
#
...
- name: test_suites_info
value: []
is_case_visible: true
My next PR already includes extra comments and renames the variables produced by the combinator so that they are prefixed by "c_", which should help.
mariner_2: "microsoftcblmariner cbl-mariner cbl-mariner-2 latest" | ||
mariner_2_arm64: | ||
urn: "microsoftcblmariner cbl-mariner cbl-mariner-2-arm64 latest" | ||
locations: |
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.
why do we need list here? I don't see a use case? and also, your implementation is only reading first value? same goes for vm_sizes?
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.
An image may be available only in a few regions.
Say an image I is available only on regions R1 and R2. Say we have test suites T1, which needs to run on R1, T2, which needs to run on R2, and T3, which can run on any region.
In that case, we would use R1 for T1, R2 for T2, and for T3 we don't care about the region, so we just use whatever value is item 0 of the list.
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.
That is not implemented right where you are not taking region based on test_suite. The example you provided could be achieved by setting region on test_suite.yml itself. I still don't see multiple in images list.
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.
it's done by the combinator:
for image in images_info:
# If the suite specifies a location, use it. Else, if the image specifies a list of locations, use
# any of them. Otherwise, use the default location.
if suite_info.location != '':
location = suite_info.location
elif len(image.locations) > 0:
location = image.locations[0]
else:
location = AgentTestSuitesCombinator._DEFAULT_LOCATION
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.
The example you provided could be achieved by setting region on test_suite.yml itself.
No, setting the region on the test suite has a different meaning. That means the suite must run on that location. When set on the image it means the image is available only on a subset of regions. They are different (and could have different values)
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.
it's done by the combinator. see the code snippet i posted above and run it over the example i mentioned
Say an image I is available only on regions R1 and R2. Say we have test suites T1, which needs to run on R1, T2, which needs to run on R2, and T3, which can run on any region.
In that case, we would use R1 for T1, R2 for T2, and for T3 we don't care about the region, so we just use whatever value is item 0 of the list.
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.
According to the schema I could have R3, R4, R5 in the image location. Lets say I didn't define any region on T3. So T3 would pick R3 and say new T4 also don't have region defined. It will pick R3 only, where is the case any test case would pick R4 and R5.
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.
None.
That the image is available on R3, R4, and R5 doesn't mean we need to use all of them. If T4 doesn't specify a location then using item 0 (R3) is as good as using item 1 (R4), or item 2 (R5).
If later on, another suite T9 needs to run on R5, then R5 will be used.
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.
Maybe I didn't understand completely. I still don't see list is useful here. I'm approving it and we can discuss when you provide another session on this pipeline.
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.
The list is needed if there are test suites that need to run on different locations.
If T1 needs to run on R1 and T2 needs to run on R2, and the image is available only on R1 and R2, if we don't use a list then you need to chose between R1 and R2, and then you can run only 1 test: either T1 or T2.
Yes, we can go over this in the presentation.
tests: List[Type[AgentTest]] | ||
# An image or image set (as defined in images.yml) specifying the images the suite must run on. | ||
images: str |
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.
what if I want to run on multiple images, this is just handling either one image-set or single image. I feel like this is very common case where we want to run particular tets_suite on few distros and need this feature too.
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.
the solution is the same as in dcr: create a new image set that includes all the images that the suite needs to run on and use that set
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.
In DCR, we create the new image set in specific test_suite, now we need to define all those in image.yml. If we end of doing it in one file, it becomes too much in single file. May be worth considering allowing new image set at test_suite level. For now, ok and I guess when we port more tests, we get the idea whether we need to implement this or not.
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.
Yes, start simple and add functionality only if actually needed. Using an array instead of a single item is pretty simple, actually, but for the moment keep things simple.
mariner_2: "microsoftcblmariner cbl-mariner cbl-mariner-2 latest" | ||
mariner_2_arm64: | ||
urn: "microsoftcblmariner cbl-mariner cbl-mariner-2-arm64 latest" | ||
locations: |
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.
That is not implemented right where you are not taking region based on test_suite. The example you provided could be achieved by setting region on test_suite.yml itself. I still don't see multiple in images list.
Now the distros and location where test should run are defined in the configuration files for the test suites, similarly to the approach followed in DCR.