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

Switch to new workflows #909

Merged
merged 5 commits into from
Nov 24, 2023

Conversation

uncleDecart
Copy link
Collaborator

@uncleDecart uncleDecart commented Oct 20, 2023

Following work done to split eden test workflows in #863 and integration
of this workflows in EVE repository, this commit switches to use new
workflows in eden itself

Also, new workflows are running on Buildjet runners, however, they
are not available in personal forks and it is useful to run test.yml
in fork, that is why this commit introduces determine-runner job,
which checks if we are in fork and changes runner to ubuntu-2204 which
is available everywhere

@uncleDecart
Copy link
Collaborator Author

I did not test it yet in my fork and would be great if @yash-zededa can check workflow files as well

steps:
- id: fork-check
run: |
if ["${{ github.event.repository.full_name}}" == "lf-edge/eve" || "${{ github.event.repository.full_name}}" == "lf-edge/eden"]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to extend this check down to action/setup-environment and enable eve.accel if workflow is running on buildjet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought we could actually change setup-environment and use some utilities to determine if hw acceleration is possible, that way we don't have coupling with runner, but rather with capability

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I also found option with lscpu :), sending it rn

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure that current logic works. I can see SandyBridge here, but it should be host in case of acceleration enabled.

@uncleDecart uncleDecart force-pushed the switch-to-new-workflows branch 2 times, most recently from 39f3909 to 75bd362 Compare October 20, 2023 11:22
${{ github.workspace }}/adam.log
test_suite_pr:
if: github.event.review.state == 'approved'
uses: ./eden/.github/workflows/test.yml@master
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we split the PR into two if you would like to modify the file you point onto here, but it doesn't exists in master branch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this file exists in master, only change here is runners for forks will be ubuntu :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I cannot understand, why we point onto branch here. I want to test my changes in eden with the workflow. But what we will check in that case? Main branch or my changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

syntax is GHA is tricky, my intention was to use local action, fixed to use just that. GHA seems to work, I'll wait for it to finish.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like another problem. It is really tricky

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's just my poor bash-ninja skills :D

Copy link
Collaborator

@giggsoff giggsoff Oct 20, 2023

Choose a reason for hiding this comment

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

Probably

[[ "${{ github.event.repository.full_name }}" == "lf-edge/eve" ]] || [[ "${{ github.event.repository.full_name }}" == "lf-edge/eden" ]]

Copy link
Collaborator

Choose a reason for hiding this comment

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

With spaces around braces ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems to be working

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it'll make sense to include similar thing to eden_setup to speed up process in eden repository

@uncleDecart
Copy link
Collaborator Author

Something weird is happening with GHA I see error which I shouldn't see here will retry in some time

@uncleDecart uncleDecart force-pushed the switch-to-new-workflows branch 4 times, most recently from d488b5a to 07c3f93 Compare October 20, 2023 12:36
@uncleDecart uncleDecart force-pushed the switch-to-new-workflows branch 7 times, most recently from 25f3184 to 258ca47 Compare October 20, 2023 14:08
@uncleDecart
Copy link
Collaborator Author

onborading seems to fail. Checked locally with master, works

@giggsoff
Copy link
Collaborator

giggsoff commented Oct 20, 2023

I can see level=fatal msg="Setup eden failed: cannot setup EVE: tag not present". So it looks like a problem with the check here. But it looks solid...

Update: Can we check something like:
if: ${{ inputs.eve_image != '' && contains(inputs.eve_image, ":" ) }}?

@uncleDecart
Copy link
Collaborator Author

I think you're right @giggsoff, let me check your changes

@uncleDecart
Copy link
Collaborator Author

Trying to make it work https://media.giphy.com/media/OVwfSuQXVUUI8/giphy.gif

@uncleDecart
Copy link
Collaborator Author

uncleDecart commented Oct 23, 2023

Now it's working, but we need to check if it works with EVE

@uncleDecart
Copy link
Collaborator Author

I think that we can merge it, try to run manually with eve_image on master branch and if it doesn't work, we do more fixes before, releasing eden and switching on EVE. Any objections @giggsoff @yash-zededa ?

@giggsoff giggsoff mentioned this pull request Oct 23, 2023
@uncleDecart
Copy link
Collaborator Author

So it gets curiouser and curiouser. I found this

actions/checkout#1418

Let's try to checkout to 3.5.3

uncleDecart added a commit to uncleDecart/eden that referenced this pull request Nov 7, 2023
Following on lf-edge#910 we saw that EVE checks out Eden master
code and not code from tag from yml file and if master fails
tests are failing on EVE, which is undesirable

https://github.com/lf-edge/eve/actions/runs/6613253197/job/17960730583?pr=3516#step:2:474

Investigating it more turns out in version 3.5.3 of github checkout
action behaviour is what we need. Check lf-edge#909 for more info

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
.github/workflows/test.yml Outdated Show resolved Hide resolved
uncleDecart added a commit to uncleDecart/eden that referenced this pull request Nov 7, 2023
Following on lf-edge#910 we saw that EVE checks out Eden master
code and not code from tag from yml file and if master fails
tests are failing on EVE, which is undesirable

https://github.com/lf-edge/eve/actions/runs/6613253197/job/17960730583?pr=3516#step:2:474

Investigating it more turns out in version 3.5.3 of github checkout
action behaviour is what we need. Check lf-edge#909 for more info

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
@@ -34,7 +34,7 @@ runs:
- name: Install Packages
run: |
sudo add-apt-repository ppa:stefanberger/swtpm-jammy
sudo apt install -y qemu-utils qemu-system-x86 jq swtpm
sudo apt install -y qemu-utils qemu-system-x86 jq swtpm lscpu
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems lscpu comes from util-linux package.

uncleDecart added a commit to uncleDecart/eden that referenced this pull request Nov 8, 2023
Following on lf-edge#910 we saw that EVE checks out Eden master
code and not code from tag from yml file and if master fails
tests are failing on EVE, which is undesirable

https://github.com/lf-edge/eve/actions/runs/6613253197/job/17960730583?pr=3516#step:2:474

Investigating it more turns out in version 3.5.3 of github checkout
action behaviour is what we need. Check lf-edge#909 for more info

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
giggsoff pushed a commit that referenced this pull request Nov 8, 2023
Following on #910 we saw that EVE checks out Eden master
code and not code from tag from yml file and if master fails
tests are failing on EVE, which is undesirable

https://github.com/lf-edge/eve/actions/runs/6613253197/job/17960730583?pr=3516#step:2:474

Investigating it more turns out in version 3.5.3 of github checkout
action behaviour is what we need. Check #909 for more info

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
@giggsoff
Copy link
Collaborator

giggsoff commented Nov 8, 2023

Please rebase on top of master with #924 merged

@giggsoff
Copy link
Collaborator

giggsoff commented Nov 8, 2023

Well, I can see host cpu in EVE-OS start line. So we use acceleration. Let's wait for the rest of the test. But seems we should workaround cloud-init test (revert changes or stabilize and adjust EVE-OS hash).

Following work done to split eden test workflows in lf-edge#863 and integration
of this workflows in EVE repository, this commit switches to use new
workflows in eden itself

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
New workflows are running on Buildjet runners, however, they
are not available in personal forks and it is useful to run test.yml
in fork, that is why this commit introduces determine-runner job,
which checks if we are in fork and changes runner to ubuntu-2204 which
is available everywhere

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
This closes lf-edge#902

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
@giggsoff
Copy link
Collaborator

giggsoff commented Nov 9, 2023

New day, new problems: I can see The self-hosted runner lost communication with the server again and again https://github.com/lf-edge/eden/actions/runs/6811998752?pr=909

@uncleDecart
Copy link
Collaborator Author

I'll ask BuildJet about it

@uncleDecart
Copy link
Collaborator Author

@giggsoff buildjet did not respond, but I see that workflows are running, any objections to merge it?

@uncleDecart uncleDecart merged commit 87f7b8a into lf-edge:master Nov 24, 2023
16 of 20 checks passed
europaul pushed a commit to europaul/eden that referenced this pull request Feb 7, 2024
Following on lf-edge#910 we saw that EVE checks out Eden master
code and not code from tag from yml file and if master fails
tests are failing on EVE, which is undesirable

https://github.com/lf-edge/eve/actions/runs/6613253197/job/17960730583?pr=3516#step:2:474

Investigating it more turns out in version 3.5.3 of github checkout
action behaviour is what we need. Check lf-edge#909 for more info

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
uncleDecart added a commit that referenced this pull request Feb 7, 2024
Following on #910 we saw that EVE checks out Eden master
code and not code from tag from yml file and if master fails
tests are failing on EVE, which is undesirable

https://github.com/lf-edge/eve/actions/runs/6613253197/job/17960730583?pr=3516#step:2:474

Investigating it more turns out in version 3.5.3 of github checkout
action behaviour is what we need. Check #909 for more info

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
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