Re: [PATCH] automation: test.yaml: Introduce templates to reduce the overhead

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Stefano,

On 21/10/2022 23:42, Stefano Stabellini wrote:
> 
> 
> On Wed, 19 Oct 2022, Michal Orzel wrote:
>> At the moment, we define lots of test jobs in test.yaml, that make use
>> of the same configuration sections like variables, tags, artifacts.
>> Introduce templates (hidden jobs whose names start with a dot) to
>> reduce the overhead and simplify the file (more than 100 lines saved).
>> This way, the actual jobs can only specify sections that are unique
>> to them.
>>
>> Most of the test jobs specify the same set of prerequisite jobs under needs
>> property with just one additional being unique to the job itself. Introduce
>> YAML anchors for that purpose, because when using extends, the needs property
>> is not being merged (the parent property overwrites the child one).
> 
> I like the patch. Replying here on top because the diff below is not
> very helpful.
> 
> When you say that "extends" overwrites the properties, do you mean that
> "needs" in qemu-smoke-dom0-arm64-gcc overwrites "needs" in .qemu-arm64,
> when qemu-smoke-dom0-arm64-gcc includes .qemu-arm64?
Yes, exactly. The behavior depends on the property. For example, the variables
section is merged but needs end up being overwritten. This is because the 
extends
does not merge the keys (variables uses key: value, whereas needs does not).

> 
> 
> If there is no way to solve the overwrite problem then it is OK to use
> YAML achors but is it possible to define the anchors outside of
> .qemu-arm64/.qemu-arm32 ? It would make things a lot clearer in the
> code. Maybe under a top level "definitions" key? The point is that
> .qemu-arm64 and .qemu-arm32 should use the anchor rather than define the
> anchor.
It is possible to define anchors outside qemu-arm64/arm32. I decided to
define them in these jobs because for me it looked cleaner (less lines of code).
But I'm ok to carve them out if that is what you prefer. This would
require dropping the needs property from the extend jobs, as they cannot make 
use
of the anchors (overwrite issue), and using the anchors from real jobs (just 
like I did in this patch).
So we would have:

.arm64-test-needs: &arm64-test-needs
  - alpine-3.12-arm64-rootfs-export
  - kernel-5.19-arm64-export
  - qemu-system-aarch64-6.0.0-arm64-export

.qemu-arm64:
  extends: .test-jobs-common
  variables:
    CONTAINER: debian:unstable-arm64v8
    LOGFILE: qemu-smoke-arm64.log
  artifacts:
    paths:
      - smoke.serial
      - '*.log'
    when: always
  tags:
    - arm64

qemu-smoke-dom0-arm64-gcc:
  extends: .qemu-arm64
  script:
    - ./automation/scripts/qemu-smoke-dom0-arm64.sh 2>&1 | tee ${LOGFILE}
  needs:
    - *arm64-test-needs
    - alpine-3.12-gcc-arm64


> 
> I wouldn't call it qemu-arm64-needs because it has things
> like alpine-3.12-arm64-rootfs-export and kernel-5.19-arm64-export that
> are not required by qemu-system-aarch64-6.0.0-arm64-export. If anything
> qemu-system-aarch64-6.0.0-arm64-export needs CONTAINER:
> debian:unstable-arm64v8.
> 
> So I would call the anchor something like "arm64-test-needs". Same
> comment for the arm32 anchor.
Ok, this naming sounds good to me.

> 
> 
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>> This patch is based on the CI next branch where we already have several
>> patches (already acked) to be merged into staging after the release:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.com%2Fxen-project%2Fpeople%2Fsstabellini%2Fxen%2F-%2Ftree%2Fnext&amp;data=05%7C01%7Cmichal.orzel%40amd.com%7Ca83af11b062b431b4f0908dab3ad2162%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019853419768862%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=TZxie442G%2Bm6SP%2FemyPuv8dwCDXAv1Wxwe22yGQZaB4%3D&amp;reserved=0
>>
>> Tested pipeline:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.com%2Fxen-project%2Fpeople%2Fmorzel%2Fxen-orzelmichal%2F-%2Fpipelines%2F671114820&amp;data=05%7C01%7Cmichal.orzel%40amd.com%7Ca83af11b062b431b4f0908dab3ad2162%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019853419768862%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=tMwGAZUKyvDp%2BxmVdxUD1kg3uMagWdO2P1DjF5O3b2M%3D&amp;reserved=0
>> ---
>>  automation/gitlab-ci/test.yaml | 266 ++++++++++-----------------------
>>  1 file changed, 80 insertions(+), 186 deletions(-)
>>
>> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
>> index 92e0a1f7c510..fc0884b12082 100644
>> --- a/automation/gitlab-ci/test.yaml
>> +++ b/automation/gitlab-ci/test.yaml
>> @@ -7,32 +7,12 @@
>>      - /^coverity-tested\/.*/
>>      - /^stable-.*/
>>
>> -# Test jobs
>> -build-each-commit-gcc:
>> -  extends: .test-jobs-common
>> -  variables:
>> -    CONTAINER: debian:stretch
>> -    XEN_TARGET_ARCH: x86_64
>> -    CC: gcc
>> -  script:
>> -    - BASE=${BASE_SHA:-${CI_COMMIT_BEFORE_SHA}} 
>> TIP=${TIP_SHA:-${CI_COMMIT_SHA}} ./automation/gitlab-ci/build-each-commit.sh 
>> 2>&1 | tee ../build-each-commit-gcc.log
>> -    - mv ../build-each-commit-gcc.log .
>> -  artifacts:
>> -    paths:
>> -      - '*.log'
>> -    when: always
>> -  needs: []
>> -  tags:
>> -    - x86_64
>> -
>> -qemu-smoke-dom0-arm64-gcc:
>> +.qemu-arm64:
>>    extends: .test-jobs-common
>>    variables:
>>      CONTAINER: debian:unstable-arm64v8
>> -  script:
>> -    - ./automation/scripts/qemu-smoke-dom0-arm64.sh 2>&1 | tee 
>> qemu-smoke-arm64.log
>> -  needs:
>> -    - alpine-3.12-gcc-arm64
>> +    LOGFILE: qemu-smoke-arm64.log
>> +  needs: &qemu-arm64-needs
>>      - alpine-3.12-arm64-rootfs-export
>>      - kernel-5.19-arm64-export
>>      - qemu-system-aarch64-6.0.0-arm64-export
> 
> LOGFILE should be listed among the artifacts (and maybe we can remove
> *.log if it has become redundant?)
*.log is not redundant because we have 4 logs to be stored, e.g.:
- smoke.serial
- config.log
- build.log
- qemu-smoke-arm64.log aka LOGFILE

So we can either have this:
artifacts:
  paths:
    - smoke.serial
    - '*.log'

or this:
artifacts:
  paths:
    - smoke.serial
    - ${LOGFILE}
    - '*.log'

I would prefer the former (just as it is now) but if you prefer the latter, it 
is ok.

~Michal