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

Daily Libvirt CI #1595

Merged
merged 3 commits into from
Dec 6, 2023
Merged

Conversation

wainersm
Copy link
Member

This introduces a new daily workflow to test CAA for Libvirt. It is essentially the e2e tests on pull requests workflow running everyday at 4:15 UTC.

I'm sending a commit to disable the build of CentOS podvm because it's broken for a while now.

Tested in https://github.com/wainersm/cc-cloud-api-adaptor/actions/runs/6952867033/job/18917750444 . The job that effectively run the e2e tests didn't run because I don't have the GARM setup on my fork to serve the runner VM. I don't think it is a big deal though, we can put this workflow running to keep fixing until gets stable.

Does not depend directly on #1594 but certainly tests won't pass without that fix.

schedule:
# Runs "at 04:15(UTC time) every day" (see https://crontab.guru)
# will base on default branch `main`
- cron: '15 4 * * *'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if github supports H but with cronjobs it's usually adviced to use H rather than picking up random minute values so that cron can distribute the workflows throughout the hour (H 4 * * *)

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 learned something new today :)

But I don't know whether github supports H or not. It claims to use the POSIX cron syntax (https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule) but not sure if all expressions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Jenkins knows it ;-)

# cloud-api-adaptor: image release tag
E2E_IMG_RELEASE_TAG: latest
# cloud-api-adaptor image dev tag
E2E_IMG_DEV_TAG: latest-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that this is actually a copy of the e2e_on_pull with a different tags, without cloning the repo and without authorization. Wouldn't it be better to turn that to a workflow_call making some steps optional based on a variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

(I'm afraid things will digress as little issues are handled in one but not the other)

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. At first I tried to refactor to e2e_on_pull in order to make it callable (generic) but I faced some limitations on the github actions syntax. Let me have a 2nd look...

@@ -22,19 +22,23 @@ jobs:
strategy:
fail-fast: false
matrix:
os: [centos, ubuntu]
os:
# FIXME: temporarily disable CentOS builds while we don't find a
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you please add a few tabs to align to the lines? (to be consistent with the other files)

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 will do

Will run the libvirt e2e tests everyday at 04:15 UTC

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
@wainersm
Copy link
Member Author

wainersm commented Dec 1, 2023

@ldoktor what about this last version?

@@ -156,7 +156,9 @@ jobs:
fail-fast: false
matrix:
os:
- centos
# FIXME: temporarily disable CentOS tests as the CentOS podvm builds
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's a nit-pick, but there are still 2 indentation ways in this PR. The one used here fits me better but anything that is consistent would be appreciated.

with:
registry: ghcr.io/${{ github.repository_owner }}
image_tag: latest
authorized: true
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC this workflow didn't used the authorized in previous version, is it a mistake or intent?

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 didn't. Reading this comment I realized the "authorize" job is only required on the on_pull workflow; so I removed it from the "run_all" workflow.

runs-on: ubuntu-latest
if: |
inputs.authorized == true ||
contains(github.event.pull_request.labels.*.name, 'test_e2e_libvirt')
Copy link
Contributor

Choose a reason for hiding this comment

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

previously this condition was there and now you added the inputs.authorized, are you sure you want to use this to force-enable it rather than only check when authorized is set? I think && would fit better here (but I may be wrong)

# Build the podvm images.
#
podvm_builder:
needs: [authorize]
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'm interested whether this will work for tasks without authorize. Hopefully it will. (I mean the step will be skipped, but does that counts as completed? I hope it does)

Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

Thank you, Wainer, I like this version better. Unless I got it wrong the authorize handling is iffy and there are few little issues here and there, most of them probably negotiable :-)

The podvm build has failed on github actions since the update to Stream
9. For the sake of keeping the libvirt e2e tests running on CI for pull
pull requests and daily, let's disable its build.

CentOS builds for podvm_builder and podvm_binaries are still enabled as
a mean to keep testing to ensure regressions aren't introduced in
meanwhile.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Moved common code from e2e_on_pull to the new callable e2e_run_all
workflow. Adjusted daily-e2e-tests-libvirt and e2e_on_pull to pass the
right inputs.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
@wainersm
Copy link
Member Author

wainersm commented Dec 4, 2023

@ldoktor on this new version I moved the "authorize" job back to run only as part of "on_pull" workflow because the authorization is not required on other workflows (e.g. the daily). I also hope to have fixed all the indentation issues :)

Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

Sorry for being a bit late to this and I don't have anything to add other than that it looks good. Thanks @wainersm and also @ldoktor for doing the first round of reviewing.

This was referenced Dec 4, 2023
Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

Thank you, Wainer, it looks good although I'm not certain about the change from:

       inputs.authorized == true ||

to

      github.event_name == 'schedule' ||
      github.event_name == 'workflow_dispatch' ||

But I guess you know the right condition there better than me :-)

@wainersm wainersm merged commit 4d80e37 into confidential-containers:main Dec 6, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants