Extend install doc to cover nginx and improve nginx config #5232

asdil12 · 2023-06-30T12:45:44Z

Ticket: https://progress.opensuse.org/issues/131024

github-actions · 2023-06-30T12:46:07Z

Great PR! Please pay attention to the following items before merging:

Files matching docs/*.asciidoc:

Consider generating documentation locally to verify it is rendered correctly using tools/generate-htmldoc

This is an automatically generated QA checklist based on modified files.

docs/Installing.asciidoc

asdil12 · 2023-07-04T13:03:53Z

@okurz you encountering the 403 error on o3 was actually an issue with the name of the virtualhost not being set properly.
Therefore the default vhost in nginx.conf was taking over.
The config on o3 can be improved in this regard by commenting out the default vhost.
This will also get rid of this error in the journal:

ariel nginx[11223]: nginx: [warn] conflicting server name "openqa.opensuse.org" on 0.0.0.0:80, ignored

dist/rpm/openQA.spec

docs/Installing.asciidoc

etc/nginx/vhosts.d/openqa-locations.inc

Martchus

Moving the config did not break the Docker test. Looks like it is using its own config under container/webui/nginx.conf. That means we don't have a test covering the config file you're editing/moving here. Maybe we can make the Docker test use the config under etc/nginx instead?

docs/Installing.asciidoc

Packaging is done via %config(noreplace) as this should make the life easier for the operator. See https://www.cl.cam.ac.uk/~jw35/docs/rpm_config.html

asdil12 · 2023-07-05T11:59:04Z

Moving the config did not break the Docker test. Looks like it is using its own config under container/webui/nginx.conf. That means we don't have a test covering the config file you're editing/moving here. Maybe we can make the Docker test use the config under etc/nginx instead?

I had a look at the docker nginx file but this container seems to just a load balancer within a docker-compose setup.
The nginx config for the container is quite different. It uses individual host (container) names in the proxy target urls to reach the different perl webapps in their corresponding containers.
Also it has some basic TLS setup.
So I would argue that it actually could make sense how the container has its own nginx config file right now.

asdil12 · 2023-07-06T09:34:49Z

PR RPMs tested locally

okurz

See one online comment and also:

Did you test on o3?
I suggest to split the PR with first the extension to bootstrap and then in a follow-up PR the changes to nginx

script/configure-web-proxy

asdil12 · 2023-07-06T14:20:54Z

Did you test on o3?

yes - works fine.

I suggest to split the PR with first the extension to bootstrap and then in a follow-up PR the changes to nginx

I don't see much use in that. Why should the bootstrap script support things that will fail due to missing config files.
We can simply merge this PR.

perlpunk · 2023-07-06T14:41:18Z

script/openqa-bootstrap

+    pgrep -f openqa-websockets-daemon >/dev/null || su geekotest -c /usr/share/openqa/script/openqa-websockets-daemon &
+    pgrep -f openqa-gru >/dev/null || su geekotest -c /usr/share/openqa/script/openqa-gru &
+    pgrep -f openqa-livehandler-daemon >/dev/null || su geekotest -c /usr/share/openqa/script/openqa-livehandler-daemon &
+    if [ "$setup_web_proxy" == "nginx" ] ; then


Suggested change

if [ "$setup_web_proxy" == "nginx" ] ; then

if [[ $setup_web_proxy == nginx ]] ; then

script/openqa-bootstrap

perlpunk · 2023-07-06T14:46:13Z

https://app.circleci.com/pipelines/github/os-autoinst/openQA/11886/workflows/0f8caf6c-6f51-4666-940f-ed2f19b14e33/jobs/111144

[13:52:43] t/ui/18-tests-details.t .................... 1/? 
    #   Failed test 'correct tags displayed'
    #   at t/ui/18-tests-details.t line 154.
    #     Structures begin differing at:
    #          $got->{inst-bootmenu} = Does not exist
    #     $expected->{inst-bootmenu} = ARRAY(0x5645b375db90)
    # Looks like you failed 1 test of 4.
[13:52:43] t/ui/18-tests-details.t .................... 4/? 
#   Failed test 'displaying image result with candidates'
#   at t/ui/18-tests-details.t line 155.
[13:52:43] t/ui/18-tests-details.t .................... 5/?
 [13:52:43] t/ui/18-tests-details.t .................... 9/?
 [13:52:43] t/ui/18-tests-details.t .................... 10/?
 [13:52:43] t/ui/18-tests-details.t .................... 12/?
 [13:52:43] t/ui/18-tests-details.t .................... 13/?
 [13:52:43] t/ui/18-tests-details.t .................... 14/?
 [13:52:43] t/ui/18-tests-details.t .................... 15/?
 [13:52:43] t/ui/18-tests-details.t .................... 18/?
 [13:52:43] t/ui/18-tests-details.t .................... 28/?
 [13:52:43] t/ui/18-tests-details.t .................... 32/?
 [13:52:43] t/ui/18-tests-details.t .................... 35/? # Looks like you failed 1 test of 35.
[13:52:43] t/ui/18-tests-details.t .................... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/35 subtests

I can't remember I have seen this before.
If it's not related to this PR, I'll create a ticket.

perlpunk

I would suggest using [[ .. ]] instead of [ ... ] everywhere.
https://www.baeldung.com/linux/bash-single-vs-double-brackets

Martchus · 2023-07-06T14:50:57Z

I second what @perlpunk said regarding [. This also allows you to avoid quotes on the left hand side. However, this could be done as a follow up.

script/configure-web-proxy

asdil12 · 2023-07-06T14:59:20Z

I second what @perlpunk said regarding [. This also allows you to avoid quotes on the left hand side. However, this could be done as a follow up.

I updated the lines that I'm adding in this file. script/configure-web-proxy is currently written in POSIX compliant sh, so shellcheck will complain about [[ there.

But I guess it makes sense to do the refactoring of the shell scripts to use bash internals in a separate PR.

script/configure-web-proxy

perlpunk · 2023-07-06T15:21:04Z

I created a ticket for the unrelated test failure: https://progress.opensuse.org/issues/132410

commit 8224eee Merge: efc15ea 4fd97ce Author: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> AuthorDate: Thu Jul 6 15:19:39 2023 +0000 Commit: GitHub <[email protected]> CommitDate: Thu Jul 6 15:19:39 2023 +0000 Merge pull request os-autoinst#5232 from asdil12/nginx Extend install doc to cover nginx and improve nginx config

perlpunk approved these changes Jun 30, 2023

View reviewed changes

okurz requested changes Jul 2, 2023

View reviewed changes

docs/Installing.asciidoc Outdated Show resolved Hide resolved

docs/Installing.asciidoc Outdated Show resolved Hide resolved

docs/Installing.asciidoc Show resolved Hide resolved

docs/Installing.asciidoc Outdated Show resolved Hide resolved

asdil12 force-pushed the nginx branch from a151d99 to 4580135 Compare July 4, 2023 11:05

Martchus reviewed Jul 4, 2023

View reviewed changes

docs/Installing.asciidoc Outdated Show resolved Hide resolved

docs/Installing.asciidoc Show resolved Hide resolved

asdil12 force-pushed the nginx branch from 4580135 to 6e48504 Compare July 4, 2023 12:59

asdil12 changed the title ~~Extend install doc to cover nginx~~ Extend install doc to cover nginx and improve nginx config Jul 4, 2023

asdil12 requested a review from okurz July 4, 2023 13:04

okurz reviewed Jul 4, 2023

View reviewed changes

asdil12 force-pushed the nginx branch 2 times, most recently from ea5a12b to 68eef2e Compare July 4, 2023 14:11

Martchus requested changes Jul 4, 2023

View reviewed changes

docs/Installing.asciidoc Outdated Show resolved Hide resolved

docs/Installing.asciidoc Outdated Show resolved Hide resolved

docs/Installing.asciidoc Outdated Show resolved Hide resolved

docs/Installing.asciidoc Outdated Show resolved Hide resolved

asdil12 marked this pull request as draft July 4, 2023 15:31

asdil12 force-pushed the nginx branch from fae2f22 to fe82394 Compare July 4, 2023 15:33

asdil12 added 2 commits July 4, 2023 17:40

Extend install doc to cover nginx

1866e19

Improved nginx config: Provide template and includes

816add8

Packaging is done via %config(noreplace) as this should make the life easier for the operator. See https://www.cl.cam.ac.uk/~jw35/docs/rpm_config.html

asdil12 force-pushed the nginx branch 3 times, most recently from 3b83430 to b6ad0bf Compare July 5, 2023 10:19

asdil12 marked this pull request as ready for review July 6, 2023 09:00

asdil12 requested review from okurz and Martchus July 6, 2023 09:34

This was referenced Jul 6, 2023

Add option for testing nginx proxy instead of apache in openqa_bootstrap os-autoinst/os-autoinst-distri-opensuse#17373

Merged

Schedule openqa_bootstrap test using nginx os-autoinst/opensuse-jobgroups#347

Merged

okurz requested changes Jul 6, 2023

View reviewed changes

script/configure-web-proxy Outdated Show resolved Hide resolved

asdil12 force-pushed the nginx branch from b6ad0bf to 943c39a Compare July 6, 2023 13:41

asdil12 mentioned this pull request Jul 6, 2023

Revert "Revert "Run openQA-in-openQA test using nginx web proxy as well"" os-autoinst/opensuse-jobgroups#349

Merged

asdil12 requested a review from okurz July 6, 2023 14:25