-
Notifications
You must be signed in to change notification settings - Fork 5
K8s: fix logs, root-controller binding and namespace creation #725
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
base: develop
Are you sure you want to change the base?
Conversation
|
@wanyunSu please fill in the metadata |
MRiganSUSX
left a comment
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.
Hi @wanyunSu ,
Thank you for making these improvements. I have comments, ordered by priority:
- Since you changed the command for log redirects to use process substitution, which is a Bash feature, we must change the container command interpreter to bash.
command=["/bin/bash", "-c"], - For the pod_ip injection, the regex is too aggressive. It works for this case, but there could be changes in the future where we pass multiple different parameters to the cmd, and this would replace all of them. It should be better aimed for the url it is trying to change (ie localhost, 127. ips etc). Something like this would be more appropriate:
modified_arg = re.sub(
r"([a-zA-Z]+://)(localhost|0\.0\.0\.0|127\.0\.0\.1)(:\d+)",
r"\g<1>${POD_IP}\g<3>",
arg
)
- the namespace deletion timeout should use its own (separate) timeout var
- regarding the 'service port': this is too generic, there are many services around and we should make the naming clear. It should be immediately identifiable, such as
headless_discovery_portor similar. This is both in the PM but also for the json config. - you nicely added the usage of labels, but with your change it is now being calculated and applied three times! both in
_create_podand in_build_pod_main_container. This is also clear from the logs:
this should be calculated / applied once, and then passed on as needed.
- minor: the function
_ensure_namespace_existsdoes not actually 'ensure' that the namespace exists. It simply checks if is possible to create a namespace (there isn't one rn that is not terminating). I suggest renaming to something that better describes what the function does, ie_prepare_new_session_namespaceor similar. - function
_build_pod_main_containeris a lot more complex now. I suggest refactoring to factor out the new env setting functionality, something like:
def _build_container_env(self, boot_request, tree_labels) -> list[client.V1EnvVar]:
- in addition to the $USER var, do we need to also pass $HOME ? currently it is empty inside the pod (defaulted to "/").
|
Thanks @MRiganSUSX for the comments! Please find below the resolution for each of them:
|
Description
To test it: change the config to use an actual hostname other than 'localhost'.
Now the order is : Terminating → wait until 404 → create namespace → wait Active → create pods
_build_container_env.Type of change
Key checklist
python -m pytest)pre-commit run --all-files)Further checks
(Indicate issue here: # (issue))