feat: create TES task as k8s job #200

JaeAeich · 2024-07-21T10:58:07Z

PS: This is WIP, consider this PR for POC, this maybe/will be further broken down into smaller PR once a concensus on code is reached and everthing starts to work.

Summary by Sourcery

This pull request introduces the ability to create TES tasks as Kubernetes jobs. It includes a new Kubernetes client wrapper, a converter for TES tasks to Kubernetes jobs, and updates to the deployment configuration to support these new features.

New Features:
- Introduced a Kubernetes client wrapper to abstract interactions with the Kubernetes API.
- Added functionality to convert TES tasks to Kubernetes jobs using a new converter module.
- Implemented a new module for creating TES tasks, including the creation of Kubernetes jobs and config maps.
Enhancements:
- Updated the deployment configuration to include a taskmaster template for Kubernetes jobs.
- Enhanced the CreateTask function to convert TES tasks to Kubernetes jobs and print the resulting job object.
Deployment:
- Added a new taskmaster template and environment properties to the deployment configuration.

sourcery-ai · 2024-07-21T10:58:14Z

Reviewer's Guide by Sourcery

This pull request introduces the ability to create TES tasks as Kubernetes jobs. It includes new configurations, utility functions, and classes for managing Kubernetes interactions. The changes are primarily focused on integrating Kubernetes job creation and management into the existing TES task workflow.

File-Level Changes

Files	Changes
`tesk/api/kubernetes/client_wrapper.py` `tesk/api/kubernetes/converter.py` `tesk/api/kubernetes/constants.py` `tesk/api/kubernetes/template.py` `tesk/api/kubernetes/__init__.py`	Introduced Kubernetes client wrapper, conversion logic, constants, and templates for managing Kubernetes jobs.
`tesk/utils.py` `tesk/custom_config.py`	Added utility functions and custom configuration models for handling taskmaster settings and conversions.
`tesk/api/ga4gh/tes/controllers.py` `tesk/api/ga4gh/tes/task/create_task.py` `tesk/api/ga4gh/tes/task/__init__.py`	Updated TES task creation logic to integrate with Kubernetes job creation.

Tips

Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
Continue your discussion with Sourcery by replying directly to review comments.
You can change your review settings at any time by accessing your dashboard:
- Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
- Change the review language;
You can always contact us if you have any questions or feedback.

sourcery-ai

Hey @JaeAeich - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 7 issues found
🟡 Security: 1 issue found
🟢 Testing: all looks good
🟡 Complexity: 1 issue found
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

deployment/config.yaml

tesk/api/ga4gh/tes/controllers.py

tesk/exceptions.py

tesk/api/kubernetes/client_wrapper.py

tesk/api/kubernetes/template.py

tesk/exceptions.py

tesk/api/kubernetes/client_wrapper.py

JaeAeich · 2024-07-21T11:00:10Z

The CI will break, those are easy fixes, please ignore them and review the rest of the code and logic :)

codecov · 2024-07-21T11:00:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.21%. Comparing base (1de3bc2) to head (104aa05).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #200   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files           8        8           
  Lines         561      561           
=======================================
  Hits          551      551           
  Misses         10       10

Flag	Coverage Δ
test_unit	`98.21% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

deployment/config.yaml

tesk/api/ga4gh/tes/controllers.py

tesk/api/kubernetes/template.py

tesk/custom_config.py

JaeAeich · 2024-08-03T16:18:45Z

README.md

+┌─────────────────────────────────────────────────────────┐
+│                         Kubernetes                      │
+│                                                         │
+│  ┌────────────────────────────┐  ┌───────────────────┐  │
+│  │ Secret: ftp-secret         │  │ ConfigMap/PVC     │  │
+│  │ - username                 │  │ - JSON_INPUT.gz   │  │
+│  │ - password                 │  │                   │  │
+│  └──────────▲─────────────────┘  └───────▲───────────┘  │
+│             │                            |              │
+│             │                            |              │
+│             │                            |              │
+│   ┌─────────┴────────────────────────────┴────────────┐ │
+│   │                Job: taskmaster                    │ │
+│   │ ┌───────────────────────────────────────────────┐ │ │
+│   │ │ Pod: taskmaster                               │ │ │
+│   │ │ - Container: taskmaster                       │ │ │
+│   │ │   - Env: TESK_FTP_USERNAME                    │ │ │
+│   │ │   - Env: TESK_FTP_PASSWORD                    │ │ │
+│   │ │   - Args: -f /jsoninput/JSON_INPUT.gz         │ │ │
+│   │ │   - Mounts: /podinfo                          │ │ │
+│   │ │             /jsoninput                        │ │ │
+│   │ └───────────────────────────────────────────────┘ │ │
+│   └───────────────────────────────────────────────────┘ │
+│                                                         │
+└─────────────────────────────────────────────────────────┘
+


Was just trying to understand k8s flow, please ignore, maybe will remove later.

Fine for me, but the rendered version does not look good to me:

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?

JaeAeich · 2024-08-03T16:19:12Z

deployment/charts/tesk/values.yaml

@@ -9,7 +9,7 @@ host_name: ""
 #

 # 'openstack' or 's3'
-storage: none
+storage: s3


TODO: turn back to none.

This should probably be described in more detail somewhere. Also, if none is an allowed value, this should be listed, and not just openstack and s3.

lvarin · 2024-08-05T07:56:45Z

README.md

+┌─────────────────────────────────────────────────────────┐
+│                         Kubernetes                      │
+│                                                         │
+│  ┌────────────────────────────┐  ┌───────────────────┐  │
+│  │ Secret: ftp-secret         │  │ ConfigMap/PVC     │  │
+│  │ - username                 │  │ - JSON_INPUT.gz   │  │
+│  │ - password                 │  │                   │  │
+│  └──────────▲─────────────────┘  └───────▲───────────┘  │
+│             │                            |              │
+│             │                            |              │
+│             │                            |              │
+│   ┌─────────┴────────────────────────────┴────────────┐ │
+│   │                Job: taskmaster                    │ │
+│   │ ┌───────────────────────────────────────────────┐ │ │
+│   │ │ Pod: taskmaster                               │ │ │
+│   │ │ - Container: taskmaster                       │ │ │
+│   │ │   - Env: TESK_FTP_USERNAME                    │ │ │
+│   │ │   - Env: TESK_FTP_PASSWORD                    │ │ │
+│   │ │   - Args: -f /jsoninput/JSON_INPUT.gz         │ │ │
+│   │ │   - Mounts: /podinfo                          │ │ │
+│   │ │             /jsoninput                        │ │ │
+│   │ └───────────────────────────────────────────────┘ │ │
+│   └───────────────────────────────────────────────────┘ │
+│                                                         │
+└─────────────────────────────────────────────────────────┘
+


Fine for me, but the rendered version does not look good to me:

lvarin · 2024-08-05T07:58:56Z

deployment/charts/tesk/values.yaml

@@ -9,7 +9,7 @@ host_name: ""
 #

 # 'openstack' or 's3'
-storage: none
+storage: s3


lvarin · 2024-08-05T11:48:31Z

tesk/api/ga4gh/tes/controllers.py

+            logger.error("Nothing received in request body.")
+            raise BadRequest("No request body received.")
+        tes_task = TesTask(**request_body)
+        namespace = "tesk"


I have just seen that this value is hardwired to tesk. In the java implementation, the TESK API is running in the same namespace that it will create the jobs, so you can deploy it in a shared cluster and call the namespace whatever you want.

I am not sure what others think, but this will be an issue for CSC

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.

uniqueg

Didn't review the whole PR, but here are some comments for now

uniqueg · 2024-08-06T10:46:20Z

README.md

+┌─────────────────────────────────────────────────────────┐
+│                         Kubernetes                      │
+│                                                         │
+│  ┌────────────────────────────┐  ┌───────────────────┐  │
+│  │ Secret: ftp-secret         │  │ ConfigMap/PVC     │  │
+│  │ - username                 │  │ - JSON_INPUT.gz   │  │
+│  │ - password                 │  │                   │  │
+│  └──────────▲─────────────────┘  └───────▲───────────┘  │
+│             │                            |              │
+│             │                            |              │
+│             │                            |              │
+│   ┌─────────┴────────────────────────────┴────────────┐ │
+│   │                Job: taskmaster                    │ │
+│   │ ┌───────────────────────────────────────────────┐ │ │
+│   │ │ Pod: taskmaster                               │ │ │
+│   │ │ - Container: taskmaster                       │ │ │
+│   │ │   - Env: TESK_FTP_USERNAME                    │ │ │
+│   │ │   - Env: TESK_FTP_PASSWORD                    │ │ │
+│   │ │   - Args: -f /jsoninput/JSON_INPUT.gz         │ │ │
+│   │ │   - Mounts: /podinfo                          │ │ │
+│   │ │             /jsoninput                        │ │ │
+│   │ └───────────────────────────────────────────────┘ │ │
+│   └───────────────────────────────────────────────────┘ │
+│                                                         │
+└─────────────────────────────────────────────────────────┘
+


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?

uniqueg · 2024-08-06T10:48:13Z

compressed_data

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 (.json, .txt ?). Finally, consider arranging test data in a separate folder data/ or tests/data/.

uniqueg · 2024-08-06T10:49:26Z

deployment/charts/tesk/values.yaml

@@ -9,7 +9,7 @@ host_name: ""
 #

 # 'openstack' or 's3'
-storage: none
+storage: s3


This should probably be described in more detail somewhere. Also, if none is an allowed value, this should be listed, and not just openstack and s3.

deployment/config.yaml

uniqueg · 2024-08-06T10:56:56Z

encoded_data.txt

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., data_encoded.txt and data_compressed.txt instead of encoded_data.txt and compressed_data.txt. This helps keeping related data together when sorted alphanumerically.

Even better to add something more expressive to explain the use case of these files, i.e., data_USE_CASE_encoded.txt and data_USE_CASE_compressed.txt.

uniqueg · 2024-08-06T10:58:48Z

pyproject.toml

@@ -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.


But are you sure that this will always be false positives?
If not, better to disable per line/block/module

uniqueg · 2024-08-06T11:00:47Z

tesk/api/ga4gh/tes/controllers.py

@@ -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


Do we still need the type: ignore here after implementation?

uniqueg · 2024-08-06T11:06:56Z

tesk/api/ga4gh/tes/controllers.py

+    try:
+        request_body = kwargs.get("body")
+        if request_body is None:
+            logger.error("Nothing received in request body.")
+            raise BadRequest("No request body received.")
+        tes_task = TesTask(**request_body)
+        namespace = "tesk"
+        return CreateTesTask(tes_task, namespace).response()
+    except Exception as e:
+        raise InternalServerError from e


This error handling looks wrong/weird to me:

The generic catching of Exception means that every exception will raise a 500 error, no? So there is no chance of the route ever raising a 400, 401, 403 etc. error. Surely this is not what we want.

FOCA already handles errors in application context appropriately. Specifically, all errors not listed in the exceptions dictionary are raised as 500 errors (see here), so this code is redundant.

uniqueg · 2024-08-06T11:08:58Z

tesk/api/ga4gh/tes/controllers.py

+        if request_body is None:
+            logger.error("Nothing received in request body.")
+            raise BadRequest("No request body received.")


Connexion should handle BadRequest/400 errors automatically, unless that functionality is specifically disabled in the config (which it isn't). So no need for this code, I believe (can be easily tested).

uniqueg · 2024-08-06T11:13:33Z

tesk/api/ga4gh/tes/controllers.py

+        namespace = "tesk"
+        return CreateTesTask(tes_task, namespace).response()


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()

feat: create task

6a578b0

JaeAeich requested a review from uniqueg July 21, 2024 10:58

JaeAeich requested review from lvarin and jemaltahir July 21, 2024 10:58

sourcery-ai bot reviewed Jul 21, 2024

View reviewed changes

chore: update poetry lock

4edd278

lvarin requested changes Jul 22, 2024

View reviewed changes

uniqueg changed the title ~~feat: create tes task as k8s job~~ feat: create TES task as k8s job Jul 27, 2024

JaeAeich added 4 commits July 30, 2024 17:20

review

009ca53

merge

84dfc46

WIP

2429b0f

create task

cebb7b2

JaeAeich force-pushed the createTask branch from d3bc588 to cebb7b2 Compare August 3, 2024 16:16

JaeAeich commented Aug 3, 2024

View reviewed changes

JaeAeich added 6 commits August 3, 2024 22:13

make ruff happy

eb652f4

make typos happy

89e2dfb

make mypy happy

f9b630b

make bandit happy

5f44926

fix config

ad17a00

minor

2b7a251

lvarin requested changes Aug 7, 2024

View reviewed changes

uniqueg requested changes Aug 7, 2024

View reviewed changes

JaeAeich added 2 commits August 7, 2024 22:50

chore: WIP list task

da720a7

chore: clean up a bit

38f2b46

JaeAeich mentioned this pull request Aug 9, 2024

chore: add taskmaster env in FOCA config #203

Closed

fix error

0cd3110

JaeAeich added 5 commits August 21, 2024 17:15

fix: add fixed image and add image to containers

aaad8fb

fix: missing commands

46bbe2a

fix: missing name of exec

099c528

fix: update the image

d96b6ae

fix minor

d545d24

JaeAeich force-pushed the createTask branch from 6fa35ac to d545d24 Compare August 28, 2024 09:27

lvarin added 3 commits August 28, 2024 13:06

change image for taskmaster

9d1b89c

fix: When restartPolicy or resouces.limits are missing

e2adc6f

fix: Code is using tabs, instead of 4 spaces

104aa05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create TES task as k8s job #200

feat: create TES task as k8s job #200

JaeAeich commented Jul 21, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jul 21, 2024 •

edited

Loading

sourcery-ai bot left a comment

JaeAeich commented Jul 21, 2024

codecov bot commented Jul 21, 2024 •

edited

Loading

JaeAeich Aug 3, 2024

lvarin Aug 5, 2024

uniqueg Aug 6, 2024

JaeAeich Aug 3, 2024

lvarin Aug 5, 2024

uniqueg Aug 6, 2024

lvarin Aug 5, 2024

lvarin Aug 5, 2024

lvarin Aug 5, 2024

JaeAeich Aug 7, 2024

uniqueg left a comment

uniqueg Aug 6, 2024

uniqueg Aug 6, 2024

uniqueg Aug 6, 2024

uniqueg Aug 6, 2024

uniqueg Aug 6, 2024

uniqueg Aug 6, 2024

uniqueg Aug 6, 2024

uniqueg Aug 6, 2024

uniqueg Aug 6, 2024

		namespace = "tesk"
		return CreateTesTask(tes_task, namespace).response()

feat: create TES task as k8s job #200

Are you sure you want to change the base?

feat: create TES task as k8s job #200

Conversation

JaeAeich commented Jul 21, 2024 • edited by sourcery-ai bot Loading

Summary by Sourcery

sourcery-ai bot commented Jul 21, 2024 • edited Loading

Reviewer's Guide by Sourcery

File-Level Changes

sourcery-ai bot left a comment

Choose a reason for hiding this comment

JaeAeich commented Jul 21, 2024

codecov bot commented Jul 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uniqueg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaeAeich commented Jul 21, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jul 21, 2024 •

edited

Loading

codecov bot commented Jul 21, 2024 •

edited

Loading