Add service_port_delta config for livehandler #5435

deborahbrouwer · 2024-01-23T23:30:51Z

Instead of hard-coding the service port delta for the livehandler, make the service port delta configurable while still defaulting to the expected value of 2. This allows cloud and/or containerized setups to access the web UI through a non-standard port while continuing to access the livehandler through a reverse proxy just by setting the service port delta to 0.

Issue: https://progress.opensuse.org/issues/153499

Martchus

The change looks generally good but I think having to specify the port in the worker config should be avoided (as explained in an inline comment).

Martchus · 2024-01-24T10:51:44Z

etc/openqa/openqa.ini

+## For the developer mode panel: if not accessing the web UI via Apache/NGINX reverse
+## proxy, then connect to the livehandler daemon at the <web UI port> + service_port_delta.
+## The livehandler daemon is supposed to run under the <web UI port> + 2.
+## If you want to keep using a reverse proxy while accessing the webUI through a custom port


For the sake of consistency:

Suggested change

## If you want to keep using a reverse proxy while accessing the webUI through a custom port

## If you want to keep using a reverse proxy while accessing the web UI through a custom port

Martchus · 2024-01-24T10:52:35Z

etc/openqa/openqa.ini

+## proxy, then connect to the livehandler daemon at the <web UI port> + service_port_delta.
+## The livehandler daemon is supposed to run under the <web UI port> + 2.
+## If you want to keep using a reverse proxy while accessing the webUI through a custom port
+## (e.g. 8080) then just set the service_port_delta = 0.


Suggested change

## (e.g. 8080) then just set the service_port_delta = 0.

## (e.g. 8080) then just set `service_port_delta` to 0.

Suggested change

## (e.g. 8080) then just set the service_port_delta = 0.

## (e.g. 8080) then just set `service_port_delta = 0`.

Martchus · 2024-01-24T10:55:46Z

lib/OpenQA/Worker/Job.pm

@@ -964,9 +964,11 @@ sub post_upload_progress_to_liveviewhandler {
    $self->{_progress_info} = \%new_progress_info;

    my $job_id = $self->id;
+    my $global_settings = OpenQA::Worker::Settings->new->global_settings;


When I remember correctly, this would parse the INI file again. It would be more efficient to re-use the existing settings object:

Suggested change

my $global_settings = OpenQA::Worker::Settings->new->global_settings;

my $global_settings = $self->worker->settings->global_settings;

Even better, we don't have to retrieve the worker settings at all once the web UI is telling the worker what the service port config is.

Depends on where the worker will store that port. Probably you'd add another attribute on worker level for this so the code accessing it would become $self->worker->service_port_delta.

I've add it as a attribute pretty much exactly the same as the worker_id that is also received from the web Ui when it registers a worker.

Patch is coming as soon as i think it has a chance of passing the CI tests

About CI failures: The javascript style issues should be fixed when you rebase your branch onto current master where a fix was applied. The other test failures are certainly related, e.g.

https://app.circleci.com/pipelines/github/os-autoinst/openQA/12812/workflows/ba5fce3d-e4ab-466f-a823-86ba92b79cd0/jobs/119431 stating " # message => "http://localhost:9526/asset/6229f1fe02/bootstrap.js 269:246 "jQuery.Deferred exception: Failed to construct 'WebSocket': The URL 'ws:/localhost:NaN/liveviewhandler/tests/1/developer/ws-proxy' is invalid." "Error: Failed to construct 'WebSocket': The URL 'ws:/localhost:NaN/liveviewhandler/tests/1/developer/ws-proxy' is invalid.\n at establishWebSocketConnection (http://localhost:9526/asset/285cd7a70d/ws_console.js:9:48)\\n at setupWebSocketConsole (http://localhost:9526/asset/285cd7a70d/ws_console.js:15:230)\\n at HTMLDocument.\u003Canonymous> (http://localhost:9526/tests/1/developer/ws-console?proxy=1:34:17)\\n at mightThrow (http://localhost:9526/asset/6229f1fe02/bootstrap.js:259:18)\\n at process (http://localhost:9526/asset/6229f1fe02/bootstrap.js:261:89)\" undefined",

https://app.circleci.com/pipelines/github/os-autoinst/openQA/12812/workflows/ba5fce3d-e4ab-466f-a823-86ba92b79cd0/jobs/119428 with

# Failed test '"test" configuration' # at t/config.t line 184. # Structures begin differing at: # $got->{global}{service_port_delta} = '2' # $expected->{global}{service_port_delta} = Does not exist

Martchus · 2024-01-24T10:58:45Z

etc/openqa/workers.ini

+# Config for livehandler port; see service_port_delta in openqa.ini.
+# SERVICE_PORT_DELTA = 2
+


It would be nice if that wasn't necessary. The web UI could tell the worker when the worker registers what the service port is. This way one would only have to configure this in one place and no mismatch was possible.

Could we put it in the JSON? Not sure if there's a better way to communicate state 🤔

That's what I meant. When the worker registers the JSON reply would contain the service port. (We still need a fallback to 2 so new workers still work when registering on an old web UI.)

okurz

Hi @deborahbrouwer, welcome to this project 💯 🎆 🥳 and thank you for your contribution. Same as Martchus I agree that this change can be helpful just needs "CI fixes" and the part about not needing the port delta stated explicitly in each and every worker config when the webUI can communicate what it needs to the worker.

Martchus · 2024-01-31T10:49:21Z

And please rebase against the latest master branch because otherwise the JavaScript test will keep failing. (It was only fixed recently.)
You probably also have to run make tidy locally for it to pass (but commit your changes before in case something goes wrong).

codecov · 2024-02-01T17:50:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (34cd7cf) 98.37% compared to head (07543d2) 98.37%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5435   +/-   ##
=======================================
  Coverage   98.37%   98.37%           
=======================================
  Files         389      389           
  Lines       37744    37751    +7     
=======================================
+ Hits        37132    37139    +7     
  Misses        612      612

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Martchus

Looks very good; I only have minor nitpicks (see inline comments).

lib/OpenQA/Worker/WebUIConnection.pm

assets/javascripts/running.js

Instead of hard-coding the service port delta for the livehandler, make the service port delta configurable while still defaulting to the expected value of 2. This allows cloud and/or containerized setups to access the web UI through a non-standard port while continuing to access the livehandler through a reverse proxy just by setting the service port delta to 0. Issue: https://progress.opensuse.org/issues/153499

okurz

Very nice. This looks great!

EDIT: I approved the CI run. As soon as all checks pass then mergify will automatically merge this PR.

Martchus requested changes Jan 24, 2024

View reviewed changes

okurz requested changes Jan 31, 2024

View reviewed changes

deborahbrouwer force-pushed the developer-mode-service-port branch 2 times, most recently from 94403b2 to 20842c7 Compare February 1, 2024 17:31

Martchus reviewed Feb 1, 2024

View reviewed changes

lib/OpenQA/Worker/WebUIConnection.pm Outdated Show resolved Hide resolved

assets/javascripts/running.js Outdated Show resolved Hide resolved

deborahbrouwer force-pushed the developer-mode-service-port branch from 20842c7 to 07543d2 Compare February 1, 2024 22:14

Martchus approved these changes Feb 2, 2024

View reviewed changes

okurz approved these changes Feb 2, 2024

View reviewed changes

mergify bot merged commit 0d50a81 into os-autoinst:master Feb 2, 2024
41 checks passed

okurz mentioned this pull request Feb 14, 2024

Make developer mode work in single-container setup #5471

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add service_port_delta config for livehandler #5435

Add service_port_delta config for livehandler #5435

deborahbrouwer commented Jan 23, 2024

Martchus left a comment

Martchus Jan 24, 2024

Martchus Jan 24, 2024

Martchus Jan 24, 2024

deborahbrouwer Jan 30, 2024

Martchus Jan 30, 2024

deborahbrouwer Jan 30, 2024

deborahbrouwer Jan 30, 2024

okurz Jan 31, 2024

Martchus Jan 24, 2024

kalikiana Jan 24, 2024

Martchus Jan 24, 2024

okurz left a comment

Martchus commented Jan 31, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

Martchus left a comment

okurz left a comment •

edited

Loading

	## If you want to keep using a reverse proxy while accessing the webUI through a custom port
	## If you want to keep using a reverse proxy while accessing the web UI through a custom port

	## (e.g. 8080) then just set the service_port_delta = 0.
	## (e.g. 8080) then just set `service_port_delta` to 0.

	my $global_settings = OpenQA::Worker::Settings->new->global_settings;
	my $global_settings = $self->worker->settings->global_settings;

		# Config for livehandler port; see service_port_delta in openqa.ini.
		# SERVICE_PORT_DELTA = 2

Add service_port_delta config for livehandler #5435

Add service_port_delta config for livehandler #5435

Conversation

deborahbrouwer commented Jan 23, 2024

Martchus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okurz left a comment

Choose a reason for hiding this comment

Martchus commented Jan 31, 2024 • edited Loading

codecov bot commented Feb 1, 2024 • edited Loading

Codecov Report

Martchus left a comment

Choose a reason for hiding this comment

okurz left a comment • edited Loading

Choose a reason for hiding this comment

Martchus commented Jan 31, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

okurz left a comment •

edited

Loading