-
Notifications
You must be signed in to change notification settings - Fork 31
feat: create TES task as k8s job #200
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: main
Are you sure you want to change the base?
Changes from 12 commits
6a578b0
4edd278
009ca53
84dfc46
2429b0f
cebb7b2
eb652f4
89e2dfb
f9b630b
5f44926
ad17a00
2b7a251
da720a7
38f2b46
0cd3110
aaad8fb
46bbe2a
099c528
d96b6ae
d545d24
9d1b89c
e2adc6f
104aa05
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this used for? Consider giving it a more expressive (and less ambiguous - compressed could indicate sth like ZIP or GZ) name. Also consider giving it a file extension ( |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| {"inputs": ["{\"name\": null, \"description\": null, \"url\": \"s3://my-object-store/file1\", \"path\": \"/data/file1\", \"type\": \"FILE\", \"content\": null, \"streamable\": null}"], "outputs": ["{\"name\": null, \"description\": null, \"url\": \"s3://my-object-store/outfile-1\", \"path\": \"/data/outfile\", \"path_prefix\": null, \"type\": \"FILE\"}"], "volumes": ["/vol/A/"], "resources": {"disk_gb": 40.0}, "executors": [{"api_version": "batch/v1", "kind": "Job", "metadata": {"annotations": {"tes-task-name": "string"}, "creation_timestamp": null, "deletion_grace_period_seconds": null, "deletion_timestamp": null, "finalizers": null, "generate_name": null, "generation": null, "labels": {"job-type": "executor", "taskmaster-name": "task-fef96900-6da7-4672-a1ec-14bf3d720c8f", "executor-no": "0"}, "managed_fields": null, "name": "newname", "namespace": null, "owner_references": null, "resource_version": null, "self_link": null, "uid": null}, "spec": {"active_deadline_seconds": null, "backoff_limit": null, "backoff_limit_per_index": null, "completion_mode": null, "completions": null, "manual_selector": null, "max_failed_indexes": null, "parallelism": null, "pod_failure_policy": null, "pod_replacement_policy": null, "selector": null, "suspend": null, "template": {"metadata": {"annotations": null, "creation_timestamp": null, "deletion_grace_period_seconds": null, "deletion_timestamp": null, "finalizers": null, "generate_name": null, "generation": null, "labels": null, "managed_fields": null, "name": "newname", "namespace": null, "owner_references": null, "resource_version": null, "self_link": null, "uid": null}, "spec": {"active_deadline_seconds": null, "affinity": null, "automount_service_account_token": null, "containers": [{"args": null, "command": ["/bin/sh", "-c", " < /data/file1 > /tmp/stdout.log 2> /tmp/stderr.log"], "env": [{"name": "BLASTDB", "value": "/data/GRC38", "value_from": null}, {"name": "HMMERDB", "value": "/data/hmmer", "value_from": null}], "env_from": null, "image": null, "image_pull_policy": null, "lifecycle": null, "liveness_probe": null, "name": "newname", "ports": null, "readiness_probe": null, "resize_policy": null, "resources": {"claims": null, "limits": null, "requests": {"cpu": 4.0, "memory": 8589934592.0}}, "restart_policy": null, "security_context": null, "startup_probe": null, "stdin": null, "stdin_once": null, "termination_message_path": null, "termination_message_policy": null, "tty": null, "volume_devices": null, "volume_mounts": null, "working_dir": "/data/"}], "dns_config": null, "dns_policy": null, "enable_service_links": null, "ephemeral_containers": null, "host_aliases": null, "host_ipc": null, "host_network": null, "host_pid": null, "host_users": null, "hostname": null, "image_pull_secrets": null, "init_containers": null, "node_name": null, "node_selector": null, "os": null, "overhead": null, "preemption_policy": null, "priority": null, "priority_class_name": null, "readiness_gates": null, "resource_claims": null, "restart_policy": "Never", "runtime_class_name": null, "scheduler_name": null, "scheduling_gates": null, "security_context": null, "service_account": null, "service_account_name": null, "set_hostname_as_fqdn": null, "share_process_namespace": null, "subdomain": null, "termination_grace_period_seconds": null, "tolerations": null, "topology_spread_constraints": null, "volumes": null}}, "ttl_seconds_after_finished": null}, "status": null}]} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ host_name: "" | |
| # | ||
|
|
||
| # 'openstack' or 's3' | ||
| storage: none | ||
| storage: s3 | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: turn back to none.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏽
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be described in more detail somewhere. Also, if |
||
|
|
||
| # Configurable storage class. | ||
| storageClass: | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above, consider moving to dedicated data folder. Also, consider using a file naming pattern that, when read left to right, goes from more generic to more specific, e.g., Even better to add something more expressive to explain the use case of these files, i.e., |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| H4sIAJUer2YC/9VWS3PbNhD+KxqeQ1N+xLYync4kbfqapIe0tzqDAYElhQivAKAsx+P/3gVJieDD7qWXXDTCt+A+v93FYya0bYLP3qz+yR7vMk0V3OFBN1K+Wt1lHDxzwgZhdAo3TsbjXeYv3xSFeshN+QVYyH0wDopKSDi/y+JFS8O2u1lwGmgqCg8WOtEvv39432HM6AA6pKZ8cEAVLeXJr6fs86tVZprwvzuOOqOD+TPO9+JBSKyDShxSA9OoOmf3RjYKOmcLPBRvixZ34E3jWCt5zLjwO1KX+P9qfbZ+QjkcgDXoWvvlY0atIHtwHoNCICtpYNtif47+ZDuhecT+MGU8Kgg0+tzqpVqbQGMqOjsBfB6o3+UxafEjzLHQdRYtMsx2vEmCQIcDVfYUHOZUQiurHWVALDhhOPGAVeN+4dqCikpoKsU3cMn9GjQ4GoD07ozhLtYek7QE2QXxxZR5THb0/5imGHkMTFEfwJ3Ca2OtoNpcb9br/JrTm/zq+uYip+fA8vOrsrrkNxdrdltlScpzbeK36zYrimpaAyeVAJmGerSg4b7920PeYn6GW+YeIyFIFXCgu2L3kmP9k6r2Eg+yIlLo3QA1gh87IF6wwLrqsiD2QDhQjvdhXo+Ssp2pojYlwjNwLCZBCsFhuMCMsn0lleGwJEisYIoaKtG6xH7CUiSCA6ko9g3vDKTxW+qolCCFVwmIpIofNA45ZqRgD2OZAysxvwoHxUw+N+8bzJTmAxAAvUe6tcl7oU+OwX4fDTFU4fslKq0wGyIk5aTYico0WGgPbi/QPmWsPQezA51SUgeKSk+T0tV+RFhMDO/Gbyl04bcxAzmLv6vVD6tkOa1+XBVB2cIHjvP+TJp6dTFA4FyE2uENet8ZOyb33Ye3f/3987uodE9l02Kd5l8//XR5e8JJ5YwaMjQo+O3jx/efFhVslQK3rKB3JcUQEQp5MDkSi4dZy0hRAXtgElJoj1zzHvebKeFFFlnjwognlIulT5FAyPKZ8fECZJIK5VM/cDSNtH9tsIH6y7aJm/Js3a47ZVxUe/v6drO5vHq9ucAF+tQZCNQtDQrWOOQaaZ8ch2QutvcbOw0Aiy/05EiMTtsHd47Cdu4mJuagTTk+E/7jxsSzkDZA93DAnonk9zO87Y0EvjcOHwI14cIN3MlainDtY6yVqJPBhNjUPOj41jr1W+zsxADYLQ5eHNpk1HG9dGt8IDjOqIcpKiybIBpCdHeCWsEnSONnNsZzMeE2VtVBmpA4TxZd1bjRJvO1heYLxCRfGRx/W6R4spAc4D5pCzrbVbgN3GicHRGCRMcWGZsfWqfG2b80e6ftMeN29ifsuzHhkBe4bBYNebYF3kic9It45M/EgxeaZTyWnxVMTUEgx0IS6kn1lae9taVx9TvDoHd+sqd8U3KjaNqPaWe9vI2Dkf0aHYGYRFM/EG9jHWKg+ChGC2HWdEegHS8hyKMFQit0gsQV5jGR6QbER0Vz+uzz07/JtQSDdQ0AAA== |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,9 +4,10 @@ requires = ["poetry-core"] | |
|
|
||
| [tool.bandit] | ||
| skips = [ | ||
| "B101", # Use of asserts is discouraged as it is removed during byte generation, we don't need that! | ||
| "B108", # Insecure usage of temp file/directory, false positive. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But are you sure that this will always be false positives? |
||
| "B321", # FTP-related functions are being called. | ||
| "B402", # A FTP-related module is being imported. | ||
| "B108" # Insecure usage of temp file/directory, false positive. | ||
| "B402" # A FTP-related module is being imported. | ||
| ] | ||
|
|
||
| [tool.poetry] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,12 @@ | |
|
|
||
| import logging | ||
|
|
||
| # from connexion import request # type: ignore | ||
| from foca.utils.logging import log_traffic # type: ignore | ||
|
|
||
| from tesk.api.ga4gh.tes.models import TesTask | ||
| from tesk.api.ga4gh.tes.service_info.service_info import ServiceInfo | ||
| from tesk.api.ga4gh.tes.task.create_task import CreateTesTask | ||
| from tesk.exceptions import BadRequest, InternalServerError | ||
|
|
||
| # Get logger instance | ||
| logger = logging.getLogger(__name__) | ||
|
|
@@ -26,14 +28,22 @@ def CancelTask(id, *args, **kwargs) -> dict: # type: ignore | |
|
|
||
| # POST /tasks | ||
| @log_traffic | ||
| def CreateTask(*args, **kwargs) -> dict: # type: ignore | ||
| def CreateTask(**kwargs) -> dict[str, str]: # type: ignore | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still need the |
||
| """Create task. | ||
|
|
||
| Args: | ||
| *args: Variable length argument list. | ||
| **kwargs: Arbitrary keyword arguments. | ||
| """ | ||
| pass | ||
| try: | ||
| request_body = kwargs.get("body") | ||
| if request_body is None: | ||
| logger.error("Nothing received in request body.") | ||
| raise BadRequest("No request body received.") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Connexion should handle |
||
| tes_task = TesTask(**request_body) | ||
| namespace = "tesk" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have just seen that this value is hardwired to I am not sure what others think, but this will be an issue for CSC
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't be an issue I plan on removing it from here, I'll add this to docs config or as env variable so when launching the api anyone can configure it. |
||
| return CreateTesTask(tes_task, namespace).response() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Except when calling well known builtin functions, I think it improves readibility a lot to name params when passing args. It also helps prevent bugs that may arise from using positional args. Also, in this case, it avoids the need to define a variable just to be clear about what you are passing. So consider using: return CreateTesTask(task=tes_task, namespace="tesk").response() |
||
| except Exception as e: | ||
| raise InternalServerError from e | ||
|
Comment on lines
+37
to
+46
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error handling looks wrong/weird to me:
|
||
|
|
||
|
|
||
| # GET /tasks/service-info | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| """Task API controller logic.""" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| """TESK API module for creating a task.""" | ||
|
|
||
| import logging | ||
|
|
||
| from tesk.api.ga4gh.tes.models import TesResources, TesTask | ||
| from tesk.api.kubernetes.client_wrapper import KubernetesClientWrapper | ||
| from tesk.api.kubernetes.constants import Constants | ||
| from tesk.api.kubernetes.convert.converter import TesKubernetesConverter | ||
| from tesk.constants import TeskConstants | ||
| from tesk.exceptions import KubernetesError | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class CreateTesTask: | ||
| """Create TES task.""" | ||
|
|
||
| # TODO: Add user to the class when auth implemented in FOCA | ||
| def __init__(self, task: TesTask, namespace=TeskConstants.tesk_namespace): | ||
| """Initialize the CreateTask class. | ||
|
|
||
| Args: | ||
| task: TES task to create. | ||
| namespace: Kubernetes namespace where the task is created. | ||
| """ | ||
| self.task = task | ||
| # self.user = user | ||
| self.kubernetes_client_wrapper = KubernetesClientWrapper() | ||
| self.namespace = namespace | ||
| self.tes_kubernetes_converter = TesKubernetesConverter(self.namespace) | ||
| self.constants = Constants() | ||
|
|
||
| def create_task(self) -> dict[str, str]: | ||
| """Create TES task.""" | ||
| attempts_no = 0 | ||
| while attempts_no < self.constants.job_create_attempts_no: | ||
| try: | ||
| attempts_no += 1 | ||
| resources = self.task.resources | ||
|
|
||
| minimum_ram_gb = self.kubernetes_client_wrapper.minimum_ram_gb() | ||
| if not self.task.resources: | ||
| self.task.resources = TesResources(cpu_cores=int(minimum_ram_gb)) | ||
| if resources and resources.ram_gb and resources.ram_gb < minimum_ram_gb: | ||
| self.task.resources.ram_gb = minimum_ram_gb | ||
|
|
||
| task_master_job = ( | ||
| self.tes_kubernetes_converter.from_tes_task_to_k8s_job( | ||
| self.task, | ||
| # self.user | ||
| ) | ||
| ) | ||
|
|
||
| task_master_config_map = ( | ||
| self.tes_kubernetes_converter.from_tes_task_to_k8s_config_map( | ||
| self.task, | ||
| task_master_job, | ||
| # self.user | ||
| ) | ||
| ) | ||
|
|
||
| # Create ConfigMap and Job | ||
| _ = self.kubernetes_client_wrapper.create_config_map( | ||
| task_master_config_map | ||
| ) | ||
| created_job = self.kubernetes_client_wrapper.create_job(task_master_job) | ||
|
|
||
| assert created_job.metadata is not None | ||
| assert created_job.metadata.name is not None | ||
|
|
||
| return {"id": created_job.metadata.name} | ||
|
|
||
| except KubernetesError as e: | ||
| if ( | ||
| not e.is_object_name_duplicated() | ||
| or attempts_no >= self.constants.job_create_attempts_no | ||
| ): | ||
| raise e | ||
|
|
||
| except Exception as exc: | ||
| logging.error("ERROR: In createTask", exc_info=True) | ||
| raise exc | ||
| return {} # Dummy return to silence mypy | ||
|
|
||
| def response(self) -> dict[str, str]: | ||
| """Create response.""" | ||
| return self.create_task() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| """Kubernetes API module.""" |
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.
Was just trying to understand k8s flow, please ignore, maybe will remove later.
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.
Fine for me, but the rendered version does not look good to me:
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.
In principle it's nice to have such a schema in the docs.
I am, however, wondering about the FTP secret. Maybe this will become clearer to me after looking at the code below, but seeing it represented like this in the doc, I have the concern that FTP is somehow treated special, when, ideally, storage solutions should all be treated in an abstract manner, like previously discussed: abstract storage handler and individual implementations for different storage/file transfer solutions. And that should probably extend to managing secrets/credentials as well, no?