feat: move remote desktop to nginx #250
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
blairdrummond
changed the title
blairdrummond
changed the title
Jose-Matsuda
approved these changes
May 5, 2021
There was a problem hiding this comment.
LGTM. As for the upstreaming of translations to noVNC I can get that done easily. I originally had Nancy translate the text a while ago but I'm sending it to Wendy as well just to check.
|
From sprint planning:
|
|
I ran into this build error as well in #248 for remote-desktop. This change seemed to solve the error. It's resolved in my PR so we might need to coordinate which gets merged in first |
|
Closes #251 |
|
@JessicaBarh Let's merge yours first, then I'll rebase ontop of that. Please lmk when yours is merged in |
ca-scribner
reviewed
May 7, 2021
* Move injection of NB_PREFIX in containers from TrackedContainer object to container() fixture and pull default NB_PREFIX value from environment (similar to IMAGE_NAME). * Add debug printing to test_server_alive to more clearly show which tests pass/fail
* Add dev/imagename endpoint in Makefile to make it easier to run local versions of notebooks. Also handles NB_PREFIX explicitly and prompts users with the correct local link to log into their server * Add NB_PREFIX to the test endpoint of Makefile to allow for testing with/without NB_PREFIX
Add clickable link to dev output
Suggested edits to test suite to use NB_PREFIX
ca-scribner
approved these changes
May 18, 2021
conftest.py
Outdated
| @@ -18,7 +18,7 @@ | |||
| def http_client(): | |||
| """Requests session with retries and backoff.""" | |||
| s = requests.Session() | |||
| retries = Retry(total=6, backoff_factor=1) | |||
| retries = Retry(total=6, backoff_factor=5) | |||
There was a problem hiding this comment.
Achieves the same cumulative time, but the wait between the first several backoffs is shorter so things that don't need the extra time will be a little quicker.
Suggested change
| retries = Retry(total=6, backoff_factor=5) | |
| retries = Retry(total=9, backoff_factor=1) |
This was referenced May 18, 2021
3 tasks
JaeggerJose
pushed a commit
to wycc/cgu-kubeflow-containers
that referenced
this pull request
Dec 30, 2024
Also: * Add `dev` endpoint to Makefile and NB_PREFIX to tests * Increase connection retries in test suite to give vnc time to start * Add clickable link to `make/dev` output Co-authored-by: Andrew Scribner <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The PR moves remote-desktop off the jupyter-proxy it was previously on, and onto an nginx-based proxy, significantly reducing lag (previous implementation was pretty intolerable). It is not in perfect state yet, but ready for review and iteration. Some notable things:
/toolsor/proxyendpoints that jupyter or the old remote-desktop used to run services. This might be considered a regression, or a security feature (@brendangadd ?), especially since apps can still be tested within a local browser.Relevant: #194 , #190, #208, #251
Tests / Quality Checks
Automated Testing/build and deployment
auto-deploytag to the PR before pushing in order to build and push the image to ACR so you can test it in cluster as a custom image?JupyterLab extensions
jupyter labextension listfrom inside the notebook)?VS Code tests
Code review
auto-deploytag to your PR before your most recent push to this repo? This causes CI to build the image and push to our ACR, letting reviewers access the built image without having to create it themselvesk8scc01covidacr.azurecr.io/machine-learning-notebook-cpu:746d058e2f37e004da5ca483d121bfb9e0545f2b)?