Feature: Plugin V2: Running plugin on host #461
Conversation
timotheeguerin
changed the title
timotheeguerin
changed the title
|
This PR should also probably address #424 |
|
Oh yeah think I already solve that one actually |
|
This still need docs. I started doing some plugin docs in #467 which can be a starting point. Also, it seems like |
| @@ -147,7 +146,8 @@ def _add_plugins(self): | |||
| execute='{0}/{1}'.format(plugin.name, plugin.execute), | |||
| args=plugin.args, | |||
| env=plugin.env, | |||
| runOn=plugin.run_on.value, | |||
| target=plugin.target.value, | |||
| target_role=plugin.target_role.value, | |||
There was a problem hiding this comment.
This is a breaking change. All existing plugins use the runOn field which breaks them. This change should update all existing plusings and maybe deprecate runOn instead of remove it for now?
There was a problem hiding this comment.
Since plugins don't exist in the latest release (v0.6.0), this shouldn't be a breaking change. I think going forward we should limit deprecation to the cross release changes.
Anyone using plugins today is using master, and should not expect the same level of stability as if they were upgrading from a previous minor release.
| | `target` | required | str | Where the file should be dropped relative to the plugin working directory | | ||
| | `local_path` | required | str | Path to the local file you want to upload(Could form the plugins parameters) | | ||
|
|
||
| ### `TextPluginFile` |
There was a problem hiding this comment.
Is this distinction necessary? Couldn't a plugin_file just support either a local_path or content?
There was a problem hiding this comment.
Well text plugin file can then take a string not only a string.io then there is no way to know if the string is the content or the path to the file
aztk/node_scripts/setup_host.sh
Outdated
| @@ -2,19 +2,21 @@ | |||
|
|
|||
| # Entry point for the start task. It will install all dependencies and start docker. | |||
| # Usage: | |||
| # setup_node.sh [container_name] [gpu_enabled] [docker_repo] [docker_cmd] | |||
| # setup_host.sh [container_name] [gpu_enabled] [docker_repo] [docker_cmd] | |||
There was a problem hiding this comment.
This needs to be updated to #setup_host.sh [container_name] [repo_name]. I keeping docker in the name of the variable for the docker repo is also easier to read. Maybe docker_repo_name is best?
| cmd.open_port(7077) # Spark Master | ||
| cmd.open_port(7337) # Spark Shuffle Service | ||
| cmd.open_port(4040) # Job UI | ||
| cmd.open_port(18080) # Spark History Server UI |
There was a problem hiding this comment.
Don't think this is the right place to do it, but we should only open the history server port if we detect that the user has configured to cluster to have it enabled (i.e. the proper keys are set in spark-defaults.conf when the cluster is created).
Otherwise, users might expect history server to be up when they have no configured their spark-defaults.conf correctly.
There was a problem hiding this comment.
well this just opens the port on the docker container this shouldn't hurt. Though we could only display it in the ssh login if enabled. Maybe we could have the history server as a plugin too
| @@ -30,7 +32,13 @@ if ! host $HOSTNAME ; then | |||
| echo $(hostname -I | awk '{print $1}') $HOSTNAME >> /etc/hosts | |||
| fi | |||
|
|
|||
| if [ $gpu_enabled == "True" ]; then | |||
| # Install docker-compose | |||
There was a problem hiding this comment.
this isn't actually used in this PR. Can we comment it out/remove it until it is since this will add to cluster startup time?
There was a problem hiding this comment.
Pablo is using it in his plugin. This allow plugins right now to use docker compose
| @@ -19,6 +17,7 @@ def connect(hostname, | |||
| username=None, | |||
| password=None, | |||
| pkey=None): | |||
| import paramiko | |||
There was a problem hiding this comment.
it was being imported on the cluster and we don;t install it on the cluster. Other options is to install on the cluster
There was a problem hiding this comment.
when/why was it being imported on the cluster?
There was a problem hiding this comment.
dependency with module
Traceback (most recent call last):
File "/mnt/batch/tasks/startup/wd/aztk/node_scripts/main.py", line 2, in <module>
import aztk.spark
File "/mnt/batch/tasks/startup/wd/aztk/spark/__init__.py", line 2, in <module>
from .client import Client
File "/mnt/batch/tasks/startup/wd/aztk/spark/client.py", line 5, in <module>
from aztk.client import Client as BaseClient
File "/mnt/batch/tasks/startup/wd/aztk/client.py", line 15, in <module>
import aztk.utils.ssh as ssh_lib
File "/mnt/batch/tasks/startup/wd/aztk/utils/ssh.py", line 12, in <module>
import paramiko
ImportError: No module named 'paramiko'
fix #462
This also sets the core to get started on #391(Plugin can install their own container)
fix #424 Plugin setup does not respect
worker_on_master