Repository navigation
Draft PR - Kubernetes based interactive tools integration in openVRE - #59
yasirmaqsood wants to merge 17 commits into
Conversation
Merge origin/dev and apply openvre-bj batch prefixes, interactive job routing, RStudio workspace sync, SGE setup skip, and Keycloak user claim defaults.
Route interactive sessions through /interactive-tool/{container}/ in
actions-home.js and TR_fileInteractive.htm.
There was a problem hiding this comment.
This file is not used anywhere
There was a problem hiding this comment.
Why not having the intermediate page as the rest of the interactive tools and dealing with the exact URL redirection at front_end/openVRE/public/phplib/interactive_tools.inc.php?
There was a problem hiding this comment.
Simplify functions using auxiliar functions if needed and split file to avoid having more than ~200 lines, 400 max.
| } | ||
|
|
||
|
|
||
| protected function enqueueInteractiveK8s($tool, $cpus, $memory) |
There was a problem hiding this comment.
$cpus and $memory are not used
| $dataDir = $user['id'] . "/" . $user['activeProject']; | ||
| $dataDirP = $GLOBALS['dataDir'] . "/" . $dataDir; | ||
|
|
||
| foreach ($tool['infrastructure']['volumes'] as $hostDir => $containerDir) { |
There was a problem hiding this comment.
$containerDir is not used. Should it be part of the volume map as it is for Docker?
| } | ||
|
|
||
| if (is_null($launcherType) && is_numeric($pid)) { | ||
| if (strpos((string)$pid, "openvre-ix-") === 0) { |
There was a problem hiding this comment.
Why not checking only $launcherType == "kubernetes_interactive"?
| throw new NotFoundException("Job ID not provided."); | ||
| } | ||
|
|
||
| if (strpos((string)$pid, "openvre-ix-") === 0) { |
There was a problem hiding this comment.
Why not checking only $launcherType == "kubernetes_interactive"?
| mkdir("$dataDirP/repository", 0775); | ||
| } | ||
|
|
||
| // RStudio workspace . |
There was a problem hiding this comment.
Remove this. Interactive tool folder is created at registerToolVolumeDirs
| ? $userInfo['roles'] | ||
| : explode(',', (string)$userInfo['roles']); | ||
| } else { | ||
| $user['roles'] = $user['roles'] ?? []; |
There was a problem hiding this comment.
User roles should come from the OIDC token. If no roles are present we should not keep the old ones.
| $user['lastLogin'] = moment(); | ||
| $user['secretsId'] = $userInfo['sub']; | ||
| $user['roles'] = explode(',', $userInfo['roles']); | ||
| $user['secretsId'] = $userInfo['sub'] ?? ($user['secretsId'] ?? ''); |
There was a problem hiding this comment.
sub attribute in OIDC token should always be present
| $projects = getProjects_byOwner(); | ||
| syncProjectFiles($projects); | ||
|
|
||
| // Sync rstudio_data files from disk into Mongo. |
There was a problem hiding this comment.
Remove this. It is done before an interactive tool is run
There was a problem hiding this comment.
As this Dockerfile is almost identical to the one of front_end service, keep one Dockerfile and use some ARG that reads from the env file for steps that differ. Remember to do the check in setup.sh also.
There was a problem hiding this comment.
Follow this other comment https://github.com/inab/openVRE-core-dev/pull/59/changes#r4514566395
There was a problem hiding this comment.
Simplify functions using auxiliar functions if needed and split file to avoid having more than ~200 lines, 400 max.
This is the draft PR to review changes made in openVRE to embed changes for interactive tools to be run on kubernetes setup. The code was updated to handle interactive tools of both types i.e web apps and desktop apps (handled by guacamole).