Function allowlist for Executor and manual Graph management #1146
Conversation
diptanu
force-pushed
the
executor-containers
branch
from
38dc7bf to
864f356
Compare
| @@ -24,11 +24,14 @@ def __init__( | |||
| self, | |||
| executor_id: str, | |||
| code_path: Path, | |||
| compute_graph: str, | |||
There was a problem hiding this comment.
This needs to be optional.
| @@ -28,11 +33,19 @@ def __init__( | |||
| self.config_path = config_path | |||
| self._logger = structlog.get_logger(module=__name__) | |||
|
|
|||
| hostname = socket.gethostname() | |||
| ip_address = socket.gethostbyname(hostname) | |||
There was a problem hiding this comment.
This looks error prone as we don't guarantee that this IP address is reachable by server if we obtain it this way. It's better to pass it in CLI args into Executor. Because this depends on deployment environment. I'm also not sure why we need to pass this IP address to the Server because so far Server wasn't calling Executor but Executor was calling Server which is guaranteed to have a hostname reachable by Executors.
server/src/http_objects.rs
Outdated
| @@ -525,11 +524,21 @@ pub struct InvocationId { | |||
| pub id: String, | |||
| } | |||
|
|
|||
| #[derive(Debug, Serialize, Deserialize, ToSchema)] | |||
| pub struct FunctionContainer { | |||
There was a problem hiding this comment.
In current OpenSource terms this is FunctionExecutor.
| ip_address = socket.gethostbyname(hostname) | ||
|
|
||
| functions = [] | ||
| if namespace is not None and compute_graph is not None and function is not None and graph_version is not None: |
There was a problem hiding this comment.
tbh I expected all these fields to be optional and passed directly to Server and it does filtering on its side with whatever provided values. E.g. if only namespace and compute_graph are set then server sees this in the ExecutorMetadata and then routes tasks for the namespace and the compute_graph to this Executor.
server/src/http_objects.rs
Outdated
| #[derive(Debug, Serialize, Deserialize, ToSchema)] | ||
| pub struct ExecutorMetadata { | ||
| pub id: String, | ||
| pub executor_version: String, | ||
| pub addr: String, | ||
| pub functions: Vec<FunctionContainer>, |
There was a problem hiding this comment.
These are not instantiated FunctionContainers/FunctionExecutors. Because of this I'd call these things as Executor filtering labels. Server will need to eventually track FunctionContainers/FunctionExecutors that really exist. Not mixing the filtering labels and existing FunctionContainers/FunctionExecutors would be nice imo.
diptanu
force-pushed
the
executor-containers
branch
from
a005315 to
6fdec9e
Compare
| @@ -27,11 +33,31 @@ def __init__( | |||
| self.config_path = config_path | |||
| self._logger = structlog.get_logger(module=__name__) | |||
|
|
|||
| hostname = socket.gethostname() | |||
| ip_address = socket.gethostbyname(hostname) | |||
There was a problem hiding this comment.
These lines also result in requiring to run Executor under sudo, fyi.
eabatalov
force-pushed
the
executor-containers
branch
from
54eb620 to
f08a42d
Compare
eabatalov
changed the title
eabatalov
force-pushed
the
executor-containers
branch
from
f08a42d to
474ea2a
Compare
Making the executors only run functions of specified compute graphs. The functions are specified as `indexify-cli executor --function <namespace>:<workflow>:<compute_func>:<version> --function ...`. The allowlist is used with Executors in production setups. E.g. only run a certain function in a Kubernetes POD with an Executor. This is a default production setup because each function has different resource usage and it’s easier to size containers per function. Same with other container configs like secrets, roles, volumes. `indexify-cli executor --dev` mode is still present. It makes server try to run any function on the executor. This is convenient for development of Indexify and for integration testing. Co-authored-by: Diptanu Gon Choudhury <[email protected]> Co-authored-by: Eugene Batalov <[email protected]>
eabatalov
force-pushed
the
executor-containers
branch
3 times, most recently
from
03185ba to
477d8b9
Compare
This allows users to implement versioning of their graphs with any semantic they want. Use a random uuid by default for the graph versions so if a user doesn't want to manage versions manually we just always update the graph on each RemoteGraph.deploy().
eabatalov
force-pushed
the
executor-containers
branch
from
477d8b9 to
cf28112
Compare
eabatalov
changed the title
Making the executors only run functions of specified compute graphs.
The functions are specified as
indexify-cli executor --function <namespace>:<workflow>:<compute_func>:<version> --function ....The allowlist is used with Executors in production setups. E.g. only run
a certain function in a Kubernetes POD with an Executor.
This is a default production setup because each function has different
resource usage and it’s easier to size containers per function.
Same with other container configs like secrets, roles, volumes.
indexify-cli executor --devmode is still present. It makes server tryto run any function on the executor. This is convenient for development
of Indexify and for integration testing.
The new --function argument requires explicit control of function versions by the user.
Change Graph version type from u32 to string and allow user to set it in SDK.
This allows users to implement versioning of their graphs with any semantic they want.
Use a random uuid by default for the graph versions so if a user doesn't want to manage
versions manually we just always update the graph on each RemoteGraph.deploy().
Co-authored-by: Diptanu Gon Choudhury [email protected]
Co-authored-by: Eugene Batalov [email protected]