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

Build boot.iso for webui e2e tests and use it #4834

Merged

Conversation

VladimirSlavik
Copy link
Contributor

@VladimirSlavik VladimirSlavik commented Jun 13, 2023

This is as far as I got, together with @velezd.

Depends on rhinstaller/permian#74 - this is why there's the "patch commit" to change the checkout.

The mechanism for passing the built boot.iso should be working on the workflow side.

Tested here: VladimirSlavik#96
Last run of this combination: https://github.com/VladimirSlavik/anaconda/actions/runs/5256905137/jobs/9498898869

...apparently, we don't get the VM.

@VladimirSlavik VladimirSlavik added infrastructure Changes affecting mostly infrastructure webui f39 labels Jun 13, 2023
@VladimirSlavik VladimirSlavik changed the title Master webui e2e tests Build boot.iso for webui e2e tests and use it Jun 13, 2023
@rvykydal
Copy link
Contributor

We seem to have the same issues with the periodic workflow since 2023-06-08 (https://github.com/rhinstaller/anaconda/actions/runs/5216574635/jobs/9415446812), the last known good is 2023-06-06. I'll look into it.

@VladimirSlavik
Copy link
Contributor Author

I believe that the workflow file itself is probably close to complete as such, the problem should be elsewhere.

@velezd
Copy link
Contributor

velezd commented Jun 14, 2023

@rvykydal It's not the same issue.
virt-install log from VladimirSlavik#96 test:

WARNING  /home/github/actions-runner/_work/anaconda/anaconda/anaconda/images/boot.iso may not be accessible by the hypervisor. You will need to grant the 'qemu' user search permissions for the following directories: ['/home/github']
ERROR    Couldn't find kernel for install tree.
Domain installation does not appear to have been successful.

virt-install log from periodic workflow:

ERROR    Failed to connect socket to '/var/run/libvirt/virtqemud-sock': No such file or directory

@VladimirSlavik
Copy link
Contributor Author

Ooooh. I need to learn how to read the permian logs, then...

Copy link
Contributor

@M4rtinK M4rtinK left a comment

Choose a reason for hiding this comment

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

Looks good to me so far. :)

@rvykydal
Copy link
Contributor

@velezd thank you for the hint, I haven't digged into the logs yet, my observation was very superfluous.

@KKoukiou KKoukiou self-requested a review June 15, 2023 09:17
@KKoukiou
Copy link
Contributor

@rvykydal It's not the same issue. virt-install log from VladimirSlavik#96 test:

WARNING  /home/github/actions-runner/_work/anaconda/anaconda/anaconda/images/boot.iso may not be accessible by the hypervisor. You will need to grant the 'qemu' user search permissions for the following directories: ['/home/github']
ERROR    Couldn't find kernel for install tree.
Domain installation does not appear to have been successful.

virt-install log from periodic workflow:

ERROR    Failed to connect socket to '/var/run/libvirt/virtqemud-sock': No such file or directory

This error often happens if you try to run session libvirt VMs inside a toolbox container. Are you doing this?

@rvykydal
Copy link
Contributor

ERROR    Failed to connect socket to '/var/run/libvirt/virtqemud-sock': No such file or directory

Should be fixed by this PR: https://gitlab.cee.redhat.com/rhinstaller/builders/-/merge_requests/130

@rvykydal
Copy link
Contributor

rvykydal commented Jun 15, 2023

@rvykydal It's not the same issue. virt-install log from VladimirSlavik#96 test:

WARNING  /home/github/actions-runner/_work/anaconda/anaconda/anaconda/images/boot.iso may not be accessible by the hypervisor. You will need to grant the 'qemu' user search permissions for the following directories: ['/home/github']
ERROR    Couldn't find kernel for install tree.
Domain installation does not appear to have been successful.

In kickstart-test permian plugin this is seemingly handled here:
https://github.com/rhinstaller/permian/blob/43529c407d3ab7333376bd8b6b47e53d6f1eafc2/libpermian/plugins/kickstart_test/__init__.py#L309
I wonder if we should set the permissions in the GH or Permian workflow...

@rvykydal
Copy link
Contributor

rvykydal commented Jun 16, 2023

@rvykydal It's not the same issue. virt-install log from VladimirSlavik#96 test:

WARNING  /home/github/actions-runner/_work/anaconda/anaconda/anaconda/images/boot.iso may not be accessible by the hypervisor. You will need to grant the 'qemu' user search permissions for the following directories: ['/home/github']
ERROR    Couldn't find kernel for install tree.
Domain installation does not appear to have been successful.

In kickstart-test permian plugin this is seemingly handled here: https://github.com/rhinstaller/permian/blob/43529c407d3ab7333376bd8b6b47e53d6f1eafc2/libpermian/plugins/kickstart_test/__init__.py#L309 I wonder if we should set the permissions in the GH or Permian workflow...

The kernel location ERROR above is related to this issue rhinstaller/permian#74 (comment)
The WARNING about permissions warns about an error we hit later (after fixing the kernel location):

ERROR    internal error: qemu unexpectedly closed the monitor: 2023-06-16T13:02:50.172512Z qemu-system-x86_64: -blockdev {"driver":"file","filename":"/home/github/actions-runner/_work/anaconda/anaconda/anaconda/images/boot.iso","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}: Could not open '/home/github/actions-runner/_work/anaconda/anaconda/anaconda/images/boot.iso': Permission denied

Setting the permissions on the GH workflow didn't really worked for me (chmod -R 0755 images plus chown github:github -R images) to fix the issue.
On the other hand, copying the image to the Permian workflow temporary directory:
rvykydal/permian@9e4cff8
fixed the issue. It is the approach same to what we do in kickstart tests Permian plugin: https://github.com/rhinstaller/permian/blob/43529c407d3ab7333376bd8b6b47e53d6f1eafc2/libpermian/plugins/kickstart_test/__init__.py#L305

@rvykydal
Copy link
Contributor

rvykydal commented Jun 19, 2023

The issue with access to the boot.iso has been resolved in the Permian workflow (rhinstaller/permian#74 (comment)).

The kernel image not found issue would be fixed by using runners >= F37.

But alas, on Fedora 37 runner I am hitting a new issue, 2 of the 3 parallel virt-install runs show this in virt-install.log:

Starting install...
^MRetrieving 'vmlinuz'                                        |    0 B  00:00 ....
^MRetrieving 'initrd.img'                                     |    0 B  00:00 ....
ERROR    internal error: pool 'boot-scratch' has asynchronous jobs running.
Domain installation does not appear to have been successful.

I didn't hit the issue on F37 when I was providing the iso via http. Seems some timing issue of asynchronous jobs.

Logs (error) from run on F37 (local boot.iso): https://rvykydal.fedorapeople.org/webui/e2e_tests/asynchronous_issue/f37/
Logs (ok) from run on F34 (local boot.iso): https://rvykydal.fedorapeople.org/webui/e2e_tests/asynchronous_issue/f34/
Logs (ok) from run on F37 (boot.iso via http): https://rvykydal.fedorapeople.org/webui/e2e_tests/asynchronous_issue/f37_http/

@velezd any ideas ?

@velezd
Copy link
Contributor

velezd commented Jun 20, 2023

@rvykydal that seems to be an issue with the Permian workflow. I will fix it.

@rvykydal
Copy link
Contributor

@rvykydal that seems to be an issue with the Permian workflow. I will fix it.

Thank you for fast fix @velezd. The workflow now seems to be working as expected:
https://github.com/rvykydal/anaconda/actions/runs/5322140393
Logs: https://rvykydal.fedorapeople.org/webui/e2e_tests/asynchronous_issue/f37_fixed/

@rvykydal
Copy link
Contributor

rvykydal commented Jun 20, 2023

So now to merge this we seem to need:

As a follow-up, we should

Maybe:

  • It would be nice to add an option for providing url to tested boot.iso (something similar to what webui periodic test does now). This way it could be easier to devleop the e2e tests itself without rebuilding the iso which is not needed in that case.

@rvykydal
Copy link
Contributor

rvykydal commented Jun 21, 2023

@rvykydal that seems to be an issue with the Permian workflow. I will fix it.

Thank you for fast fix @velezd. The workflow now seems to be working as expected: https://github.com/rvykydal/anaconda/actions/runs/5322140393 Logs: https://rvykydal.fedorapeople.org/webui/e2e_tests/asynchronous_issue/f37_fixed/

Well, all tests are failing during storage creation, I wonder if it is a fail or an error. Need to look better.

@rvykydal
Copy link
Contributor

@rvykydal that seems to be an issue with the Permian workflow. I will fix it.

Thank you for fast fix @velezd. The workflow now seems to be working as expected: https://github.com/rvykydal/anaconda/actions/runs/5322140393 Logs: https://rvykydal.fedorapeople.org/webui/e2e_tests/asynchronous_issue/f37_fixed/

Well, all tests are failing during storage creation, I wonder if it is a fail or an error. Need to look better.

The tests are failing waiting for Reboot button. So to be fixed in tests, not a framework issue.

@rvykydal
Copy link
Contributor

I'm moving this to ready for review. Seems to work good and we can enhance it later.

@rvykydal rvykydal marked this pull request as ready for review June 21, 2023 14:12
@rvykydal rvykydal requested a review from jkonecny12 June 22, 2023 06:08
Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@rvykydal
Copy link
Contributor

So in my view we just need to update the PR to use the permian devel branch and the PR is ready to be merged.

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

It's still tested on vlada's fork, right?

@VladimirSlavik
Copy link
Contributor Author

VladimirSlavik commented Jun 22, 2023

Nice, thank you!

For the periodic tests, I'd say first fix one thing, such as this, then the other can follow?

Should I do something here, or not?

@rvykydal
Copy link
Contributor

rvykydal commented Jun 23, 2023

It was tested on my fork.
The only change needed here is the update to use Permian devel branch here: https://github.com/VladimirSlavik/anaconda/blob/735d9ef041561e39278e6a325df3c2b893020041/.github/workflows/webui-tests.yml#L131

@rvykydal
Copy link
Contributor

It was tested on my fork. The only change needed here is the update to use Permian devel branch here: https://github.com/VladimirSlavik/anaconda/blob/735d9ef041561e39278e6a325df3c2b893020041/.github/workflows/webui-tests.yml#L131

Ah, which actually means removing the second commit of this PR: 735d9ef

@jkonecny12
Copy link
Member

@rvykydal @VladimirSlavik could you please share current status of this when you are available?

@VladimirSlavik VladimirSlavik force-pushed the master-webui-e2e-tests branch 2 times, most recently from 670fe0b to 7181eae Compare July 4, 2023 13:54
@VladimirSlavik
Copy link
Contributor Author

Rebased and removed the temporary commit for Permian PR changes (now merged).

@VladimirSlavik
Copy link
Contributor Author

/kickstart-test --waive infra only

@VladimirSlavik
Copy link
Contributor Author

To describe the current state:

  • As agreed with Radek, this should be mergeable after fixing the branch.
  • The workflow itself should be final. I thought this would be "done" so many times already that I refuse to believe it anymore.
  • The actual tests are reportedly failing.
  • The periodic workflow counterpart to this should be updated to match.

@VladimirSlavik VladimirSlavik merged commit 025ae5b into rhinstaller:master Jul 4, 2023
14 checks passed
@VladimirSlavik VladimirSlavik deleted the master-webui-e2e-tests branch July 4, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f39 infrastructure Changes affecting mostly infrastructure webui
Development

Successfully merging this pull request may close these issues.

6 participants