Skip to content
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

feat: add k8s client wrapper and constants #201

Merged
merged 21 commits into from
Sep 5, 2024
Merged

feat: add k8s client wrapper and constants #201

merged 21 commits into from
Sep 5, 2024

Conversation

JaeAeich
Copy link

@JaeAeich JaeAeich commented Aug 9, 2024

Summary by Sourcery

Introduce a Kubernetes client wrapper, add a new OpenAPI specification for the TES API, and implement a new Filer service for handling file transfers. Enhance documentation and configuration files, and add unit tests for job handling.

New Features:

  • Introduce a Kubernetes client wrapper to simplify interactions with the Kubernetes API.
  • Add a new OpenAPI specification for the Task Execution Service (TES) API.
  • Implement a new Filer service for handling file transfers, including support for FTP, HTTP, and S3 protocols.

Enhancements:

  • Update the README.md to include new badges for code coverage, documentation status, license, Python version, development status, contributors, and security tools.
  • Refactor the taskmaster service to improve job handling and PVC management.
  • Enhance the values.yaml configuration to include resource limits and security context settings for Kubernetes deployments.

Documentation:

  • Revise local_ftp.md documentation for better readability and formatting.
  • Update README.md to reflect the new structure and additional badges.
  • Add a new example JSON file inputHelloWorld.json for demonstrating task execution.

Tests:

  • Add unit tests for the Job class in test_services/test_job.py to cover various job states and error conditions.

@JaeAeich JaeAeich changed the base branch from master to main August 9, 2024 05:42
Copy link

sourcery-ai bot commented Aug 9, 2024

Reviewer's Guide by Sourcery

This pull request introduces a Kubernetes client wrapper, new constants, and several new services and models for the Task Execution Service (TES) API. It also includes significant updates to the documentation and Helm chart configurations, adding resource limits, security contexts, and updated ingress settings. Additionally, new unit tests and example input JSON files have been added.

File-Level Changes

Files Changes
deployment/documentation/local_ftp.md
README.md
Reformatted documentation files for better readability and consistency.
deployment/charts/tesk/values.yaml
deployment/charts/tesk/templates/common/tesk-deployment.yaml
deployment/charts/tesk/templates/ingress/ingress-rules.yaml
deployment/charts/tesk/templates/openshift/oc-route.yaml
deployment/charts/tesk/templates/storage/openstack.yaml
Updated Helm chart configurations to include resource limits, security contexts, and updated ingress settings.
tesk/api/specs/task_execution_service.117cd92.openapi.yaml
tesk/api/ga4gh/tes/models.py
tesk/api/kubernetes/wrapper.py
Added new API specifications, models, and Kubernetes client wrapper for TESK.
tesk/services/filer.py
tesk/services/taskmaster.py
Added new services for file handling and task management.
tests/test_unit/test_services/test_job.py Added new unit tests for Job service.
docs/examples/localftp/taskWithIO.json
docs/examples/inputHelloWorld.json
Added new example input JSON files for tasks.

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.

@JaeAeich JaeAeich requested review from uniqueg and lvarin and removed request for uniqueg and lvarin August 9, 2024 05:42
tesk/constants.py Outdated Show resolved Hide resolved
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aligning with my comments on #203, I would focus one PR on config params/constants. Make sure that everything is streamlined and let's merge that first.

Then focus this PR on the k8s client only.

@JaeAeich
Copy link
Author

JaeAeich commented Aug 9, 2024

Aligning with my comments on #203, I would focus one PR on config params/constants. Make sure that everything is streamlined and let's merge that first.

Then focus this PR on the k8s client only.

@uniqueg
I have stuffed them together because thats where I needed it, ie I cannot write client package, without some of these constants. It would not make sense to suddeny add all the constanst first and then build k8s client upon that?

If you still think its a better idea to merge consts first then sure, thoughts?

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.21%. Comparing base (bab9853) to head (dc0ba73).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #201   +/-   ##
=======================================
  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.

@JaeAeich
Copy link
Author

JaeAeich commented Aug 9, 2024

@uniqueg can you please checkout this error due to which for the past month I haven't been able to update deps and ignore safety or bandits errors.

TESK ❯ python tesk/app.py 
Traceback (most recent call last):
  File "/home/jaebuntu/Desktop/ecaai/TESK/tesk/app.py", line 7, in <module>
    from connexion import FlaskApp
  File "/home/jaebuntu/Desktop/ecaai/TESK/.venv/lib/python3.11/site-packages/connexion/__init__.py", line 32, in <module>
    from .apps.flask_app import FlaskApp
  File "/home/jaebuntu/Desktop/ecaai/TESK/.venv/lib/python3.11/site-packages/connexion/apps/flask_app.py", line 151, in <module>
    class FlaskJSONEncoder(json.JSONEncoder):
                           ^^^^^^^^^^^^^^^^
AttributeError: module 'flask.json' has no attribute 'JSONEncoder'

To recreate:

  • Checkout this branch.
  • Create a new python env so it doesn;t have any deps installed.
  • Run poetry update
  • Run python tesk/app.py

Notice if you hadn;t updated the deps the server will run fine but if you do it will fail. I suspect werzeug or some FOCA deps to be the issue. Lemme know if I am missing something and there is some way I can update the deps without it crashing the server.

@uniqueg
Copy link
Member

uniqueg commented Aug 19, 2024

AttributeError: module 'flask.json' has no attribute 'JSONEncoder'

My guess is that either or all of Flask, werkzeug, pydantic or Connexion are updated beyond a supported major version when you run poetry update. I guess it's easy to check what changes before and after poetry update. Flask, Werkzeug and Connexion MUST be below v3. Pydantic MUST be below v2.

@uniqueg
Copy link
Member

uniqueg commented Aug 19, 2024

Aligning with my comments on #203, I would focus one PR on config params/constants. Make sure that everything is streamlined and let's merge that first.
Then focus this PR on the k8s client only.

@uniqueg I have stuffed them together because thats where I needed it, ie I cannot write client package, without some of these constants. It would not make sense to suddeny add all the constanst first and then build k8s client upon that?

If you still think its a better idea to merge consts first then sure, thoughts?

I'm of course perfectly fine with adding things iteratively, if and when they are needed. My issue was not with config params and constants being added here, but rather with the structuring and convolution of config params and constants (mixing up consts and params, not exposing params via config.yaml, having more than one module for defining constants). I thought figuring this out in a separate PR would make things easier. But if you can manage to pull this off in the same PR, it's fine.

tesk/api/kubernetes/constants.py Outdated Show resolved Hide resolved
@JaeAeich
Copy link
Author

AttributeError: module 'flask.json' has no attribute 'JSONEncoder'

My guess is that either or all of Flask, werkzeug, pydantic or Connexion are updated beyond a supported major version when you run poetry update. I guess it's easy to check what changes before and after poetry update. Flask, Werkzeug and Connexion MUST be below v3. Pydantic MUST be below v2.

Yes its werkzeug, but it needs to be updated because its causing issues in vulnerebility CI and Im ignoring them. Its not just because I wanna update that dep but when I install otherdeps too, poetry usually ups a minor version of deps that can be and this breaks the server. I will revert changes and force install from a working locked dependency but this needs to be fixed in FOCA!

@JaeAeich
Copy link
Author

@uniqueg I think i should push the docs in this one itself?? Else I think this PR is done, please review as soon as pos.

@JaeAeich JaeAeich requested a review from uniqueg August 20, 2024 05:16
@uniqueg
Copy link
Member

uniqueg commented Aug 26, 2024

AttributeError: module 'flask.json' has no attribute 'JSONEncoder'

My guess is that either or all of Flask, werkzeug, pydantic or Connexion are updated beyond a supported major version when you run poetry update. I guess it's easy to check what changes before and after poetry update. Flask, Werkzeug and Connexion MUST be below v3. Pydantic MUST be below v2.

Yes its werkzeug, but it needs to be updated because its causing issues in vulnerebility CI and Im ignoring them. Its not just because I wanna update that dep but when I install otherdeps too, poetry usually ups a minor version of deps that can be and this breaks the server. I will revert changes and force install from a working locked dependency but this needs to be fixed in FOCA!

Yes, it's a known issue and I absolutely agree with you. Unfortunately, using Werkzeug and/or Flask v3 requires us to migrate FOCA to Connexion 3 - and that isn't a piece of cake. I have started with this but I'm afraid that realistically I may not be able to finish that up before October.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
tesk/api/kubernetes/constants.py Outdated Show resolved Hide resolved
tesk/constants.py Show resolved Hide resolved
tesk/exceptions.py Outdated Show resolved Hide resolved
@uniqueg
Copy link
Member

uniqueg commented Aug 30, 2024

@uniqueg I think i should push the docs in this one itself?? Else I think this PR is done, please review as soon as pos.

Forgot to answer this. I generally think it's important to follow Continuous Documentation practices, i.e., making sure that the docs are always as complete and up-to-date as possible by updating docs in every PR where necessary/applicable.

But exceptionally I'm okay with not doing it here - if you plan to push a docs PR very soon :)

Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor issues I still see:

  • Please remove the type definitions from the models' Attributes sections. We don't want to define types in two different places, because sooner or later they will diverge. And it's best to keep only the type hints from function signature or - in this case - the model definition, because they are closer to being code.
  • Consider defining a root class ConfigModel that extends BaseModel and sets frozen to True. Then have all constants classes inherit from ConfigModel. That way, we don't need to add a config class to each constant class. I'm not insisting on this though, so if you wanna skip this, fine.

I don't think it will need a re-review from me afterwards, so just go ahead and merge when you are done with this.

@JaeAeich
Copy link
Author

JaeAeich commented Sep 4, 2024

@uniqueg There are two on my concerns with this PR which I would llike your thoughts on:

  • Freezing werkzeug: The most effective and hassel free solution is to freeze the version that works in puproject.toml, but you think otherwise, which is fair but IDK whats wrong, I am assuming werkzeug has a breaking change in the minor update (I suspect it propogates from their Flask version, as you can see from the error message). So if freezing is out of the question, then we are left with the way I have been dealing with it for the past months, using lock files version. This is all fine but it becomes very problamatic when I want to update anyother dep and lock file changes, this again brings back the same error, which then I have to hunt in long lock file.
  • Kubernetes package: All the func that k8s package and its constants provide are secluded in api module, which indicates that this k8s package only deals with api, but if/when in future we will be overhauling service/core module, we could reuse some of the method, and constants from this k8s module. Should I shift it from tesk/api/kubernetes to tesk/kubernetes?

@uniqueg uniqueg changed the title feat: add k8s client wrapper and needed constants. feat: add k8s client wrapper and constants Sep 4, 2024
@uniqueg
Copy link
Member

uniqueg commented Sep 4, 2024

@uniqueg There are two on my concerns with this PR which I would llike your thoughts on:

  • Freezing werkzeug: The most effective and hassel free solution is to freeze the version that works in puproject.toml, but you think otherwise, which is fair but IDK whats wrong, I am assuming werkzeug has a breaking change in the minor update (I suspect it propogates from their Flask version, as you can see from the error message). So if freezing is out of the question, then we are left with the way I have been dealing with it for the past months, using lock files version. This is all fine but it becomes very problamatic when I want to update anyother dep and lock file changes, this again brings back the same error, which then I have to hunt in long lock file.
  • Kubernetes package: All the func that k8s package and its constants provide are secluded in api module, which indicates that this k8s package only deals with api, but if/when in future we will be overhauling service/core module, we could reuse some of the method, and constants from this k8s module. Should I shift it from tesk/api/kubernetes to tesk/kubernetes?
  1. Go ahead and do what works best for you. If you pin the version, please add a comment pointing to one or more appropriate issue(s), so that it will be easier to remember removing it when upgrading FOCA in the future.
  2. Yes, I think that's a good idea.

For both issues: Consider merging the PR as is, then opening new PRs for these changes. At least for the second issue, I think that would make sense.

@JaeAeich JaeAeich merged commit 5d385ba into main Sep 5, 2024
11 checks passed
@JaeAeich JaeAeich deleted the k8s_and_consts branch September 5, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants