Skip to content

Healthcheck gives correct result even behind proxy #1964

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

Merged
merged 4 commits into from
Aug 13, 2023
Merged

Healthcheck gives correct result even behind proxy #1964

merged 4 commits into from
Aug 13, 2023

Conversation

MatthiasJobst
Copy link
Contributor

@MatthiasJobst MatthiasJobst commented Aug 12, 2023

Changes

For environments that use proxies, mostly corporate environments, the health check did not work. Proxies are not necessary for the health check so the proxies are disabled.
The health check runs in the docker container and makes a request to a specific address. If that request is routed through a proxy it leaves the docker container. This can cause two types of problems:

  • The proxy does not know the ip address and thus the request never terminates
  • Outside of the container the ports can change and the request will not reach the proper port.

Issue ticket

Closed 1962

Checklist (especially for first-time contributors)

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests
  • I will try not to use force-push to make the review process easier for reviewers
  • [-] I have updated the documentation for significant changes

Copy link
Member

@mathbunnyru mathbunnyru left a comment

Choose a reason for hiding this comment

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

Overall, LGTM and thank you for adding a test 👍

Use empty string instead of None

Co-authored-by: Ayaz Salikhov <mathbunnyru@users.noreply.github.com>
@MatthiasJobst
Copy link
Contributor Author

The tests that fail do not have anything to do with the test I added. Interestingly they seem to fail without the changes too?
Maybe some deprecation issue with notebook?

@mathbunnyru
Copy link
Member

The tests that fail do not have anything to do with the test I added. Interestingly they seem to fail without the changes too?
Maybe some deprecation issue with notebook?

Let's hope it was a spurious fail. I restarted failed GitHub jobs.

@mathbunnyru
Copy link
Member

mathbunnyru commented Aug 12, 2023

I guess the reason is that for some reason mamba decides to downgrade jupyterlab and notebook:

#6 394.0   Change:
#6 394.0 ────────────────────────────────────────────────────────────────────────────────────────
#6 394.0 
#6 394.0   - _openmp_mutex                  4.5  2_gnu                    conda-forge      24kB
#6 394.0   + _openmp_mutex                  4.5  2_kmp_llvm               conda-forge       6kB
#6 394.0 
#6 394.0   Downgrade:
#6 394.0 ────────────────────────────────────────────────────────────────────────────────────────
#6 394.0 
#6 394.0   - notebook                     7.0.2  pyhd8ed1ab_0             conda-forge       3MB
#6 394.0   + notebook                     6.5.4  pyha770c72_0             conda-forge     306kB
#6 394.0   - jupyterlab                   4.0.4  pyhd8ed1ab_0             conda-forge       6MB
#6 394.0   + jupyterlab                   3.6.5  pyhd8ed1ab_0             conda-forge       5MB

jupyter notebook doesn't start properly after downgrade (I removed support for it).
It's definitely not your change and I'll need to fix it in the main branch.
I will take a look in a few days in more details.

@mathbunnyru
Copy link
Member

@MatthiasJobst I fixed (actually, mitigated) the issue in the main branch and merged it here.
Hopefully, tests will pass and then I'll merge this PR.

@MatthiasJobst
Copy link
Contributor Author

Thanks. That was quick and solved the issue.

@mathbunnyru mathbunnyru merged commit 72ef9bc into jupyter:main Aug 13, 2023
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.

[BUG] - Healthcheck fails when using proxy
2 participants