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

Add Nginx magic for Dashboard access #3518

Merged
merged 2 commits into from
Aug 17, 2023

Conversation

webbnh
Copy link
Member

@webbnh webbnh commented Aug 9, 2023

Currently, attempts to load the Pbench Dashboard (e.g., from the Staging server) via requests like

GET /Dashboard/

or

GET /dashboard

will each fail (with a 404 and a blank page, respectively).

To improve the user experience, this PR adds a location directive which rewrites the URI, down-casing the "dashboard" and adding the trailing slash if it is missing. This returns a 301/Moved Permanently response to the browser which causes it to retry at the correct location (which then gets routed properly).

This change also changes the existing Dashboard location to require the trailing slash and makes the match preferred over the regex matches, to avoid recursive rewrites.

(BTW, this change has already been deployed on the current Staging Pbench Server instance, if you want to kick the tires.)

Add a location/rewrite directive to rewrite URIs which begin with /dashboard
which aren't lower case or which lack the trailing slash, mapping them to
lower case and adding the slash.
@webbnh webbnh added this to the v0.73 milestone Aug 9, 2023
@webbnh webbnh self-assigned this Aug 9, 2023
rewrite ^ /dashboard/ permanent;
}

location ^~ /dashboard/ {
Copy link
Member

Choose a reason for hiding this comment

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

So what exactly does the addition of the ^~ accomplish vs the old form? I assume this means "begin-anchored pattern match", but as the "pattern" is a fixed text string, what does this add? And when we just load /dashboard/, are we loading $uri/index.html (line 143's try_files), which is /dashboard//index.html, or are we loading the /dashboard/index.html literal fallback?

Copy link
Member Author

Choose a reason for hiding this comment

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

So what exactly does the addition of the ^~ accomplish vs the old form?

Per the docs,

If the longest matching prefix location has the ^~ modifier then regular expressions are not checked.

So, this prevents recursive rewrites: after the rewrite, the location at line 139 matches and disables further match-searching, so the regex match is not reevaluated. (This was critical in the previous iteration of this code; however, now that I've got the no-trailing-slash location anchored at both ends, we could get by without this, but having it makes the search more efficient.)

when we just load /dashboard/, are we loading $uri/index.html (line 143's try_files), which is /dashboard//index.html, or are we loading the /dashboard/index.html literal fallback?

I expect that we try to load /dashboard/ which fails because it's a directory; then we probably try to load /dashboard//index.html which probably succeeds (Nginx may squash out the extra slash, and, if not, then open() call probably ignores it); and, if that fails, we load the fallback, /dashboard/index.html.

@dbutenhof
Copy link
Member

pquisby 0.0.13 is now up on PyPi, and it's broken:

File "/var/tmp/jenkins/tox/py39/lib/python3.9/site-packages/pbench/server/api/resources/datasets_compare.py", line 6, in
from pquisby.lib.post_processing import BenchmarkName, InputType, QuisbyProcessing
File "/var/tmp/jenkins/tox/py39/lib/python3.9/site-packages/pquisby/lib/post_processing.py", line 4, in
from src.pquisby.lib.benchmarks.fio.fio import extract_fio_run_data
ModuleNotFoundError: No module named 'src'

@sourabhtk37 @siddardh-ra @vishalvvr @MVarshini

@sourabhtk37
Copy link
Collaborator

@dbutenhof Hi,

Could you share the details of the project for me to debug? I haven't been aware of the development so far. This looks like a simple import issue. Let me know.
Thanks

@webbnh
Copy link
Member Author

webbnh commented Aug 10, 2023

Hi @sourabhtk37,

We install the pquisby package from PyPI via a requirements.txt file, and then, as the diagnostics indicate above, we import from the pquisby.lib.post_processing sub-package, which seems to contain a bad import spec.

I wasn't able to find the details of the pquisby package (it points to a GitHub repo which doesn't seem to contain all the files)...hopefully, you can connect the dots better than I could.

Thanks!

@webbnh
Copy link
Member Author

webbnh commented Aug 11, 2023

@sourabhtk37 @siddardh-ra @vishalvvr @MVarshini

Is there any update on a fix for pquisby 0.0.13? Our CI is currently broken by this, and I need to know whether I should be putting in a rev-lock on 0.0.12.

Thanks!

@webbnh
Copy link
Member Author

webbnh commented Aug 16, 2023

@sourabhtk37 @siddardh-ra @vishalvvr @MVarshini

I see that a pquisby 0.0.14 is now available, but it doesn't seem to have changed anything in terms of breaking our CI.

Is there any update on fixing this problem?

@webbnh webbnh merged commit ebb2c19 into distributed-system-analysis:main Aug 17, 2023
3 checks passed
@webbnh
Copy link
Member Author

webbnh commented Aug 17, 2023

Worked around the build problem for the moment via #3520.

@webbnh webbnh deleted the nginx_dashboard_magic branch August 17, 2023 20:06
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.

4 participants