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.
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
External Cluster Environments #1244
base: main
Are you sure you want to change the base?
External Cluster Environments #1244
Changes from 21 commits
49985f5
d491508
686ed39
7e90611
2daf2cc
ff5aab5
20a5c56
0d43bec
875fafa
5d1d732
19f92c7
e0df4e2
5e23382
c9b90ce
0e0ae6f
9a9416e
9ba9ab7
0a4c6fb
d8b55b6
94ab66e
3514129
4fbe8b7
51256b9
8107dac
26cba1a
55065ca
3282e14
047538e
7c06f2b
c909106
a1a9e46
8554343
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is more about the introduction of the
services/external
directory. One thing we need to keep in mind is that for EG 4.0, most all of these changes will move togateway_provisioners
. Since it ONLY has the equivalent ofservices/processproxies
and since this doesn't seem like an "external service", I'm inclined to suggest we simply include these files alongsideservices/processproxies/k8s.py
. I don't think they warrant their own location within the package since they are solely used byKubernetesProcessProxy
and its subclasses (and the launcher). Would there be an issue with moving these intoservices/processproxies
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I think that's fine, I can move them into processproxies and go from there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this! Thank you for making this better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem necessary at the moment and would recommend its removal unless we find it necessary. Sometimes the linting can demand this kind of thing (which is annoying).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't
get_remote_client
default toFalse
sinceEG_USER_REMOTE_CLUSTER
does the same?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why this code is in
CustomResourceProcessProxy
and notKubernetesProcessProxy
since the former is a subclass of the latter and it strikes me as necessary forKubernetesProcessProxy
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we refactor this filename to include an underscore for word separation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The following applies to lines L240-L254 below...)
Today, if the user is bringing their own namespace via
KERNEL_NAMESPACE
it is their responsibility to ensure proper operation within the namespace. With this PR, and ifEG_USE_REMOTE_CLUSTER
is True, I'm assuming the same requirement holds. Are there any pieces of new functionality that would preclude users from bringing their own namespace (where that namespace resides in the remote cluster)?Clearly, both
EG_SHARED_NAMESPACE
= True andEG_USE_REMOTE_CLUSTER
= True conflict. Perhaps we should check forEG_SHARED_NAMESPACE
= True when obtaining the k8s client and either log and return the local client or raise an exception?((Just to note, in
gateway_provisioners
the default value forEG_SHARED_NAMESPACE
is True to allow for easier deployments outside of something like since they are available to anyjupyter_client
application.))There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I realize should be documented better is that remote cluster operation requires the namespace to be already created before we launch a kernel there. This was mostly a security concern, as long as operations are isolated within a namespace in a remote cluster it's fine, but creating a namespace is a cluster level operation which can be scary for operators to enable.
Agreed on this, we should log a warning then ignore it if
EG_USE_REMOTE_CLUSTER
is enabledThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is strictly for external clusters, could we rename this to something like:
_create_external_service_account_if_not_exists
or_create_remote_service_account_if_not_exists
. Since the other configurable options refer to "external", I guess I'd prefer the former.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to access the "name" of the external cluster? Seems really helpful to include that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A similar name change to that suggested previously would be nice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should
role_yaml_path
be validated forNone
and the file's existence?This line (and any validation) should be moved within the
if kernel_cluster_role not in remote_cluster_role_names:
block at L379 so that it's only performed if needed.