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

Create a generic callbacks class for KubernetesPodOperator #35714

Merged
merged 21 commits into from
Jan 20, 2024

Conversation

hussein-awala
Copy link
Member

@hussein-awala hussein-awala commented Nov 17, 2023

This PR adds a set of callbacks for KubernetesPodOperator called when the pod starts, terminates, and is cleaned up, and as when the client is initiated.

It provides some valuable params, like the pod spec, the Kubernetes client, and the current mode (sync or async); it could be helpful in different use cases like:

  • create some resources based on the created pod and clean up them when the pod is deleted (for ex, a K8S service)
  • patch some created resources to link them to the pod and make it the owner of these resources
  • call an external system to trigger an event, push metrics...

The current implementation is only used in sync mode, but I provide the mode param for the future support for async mode. This support is blocked by the way of serializing the trigger kwargs, where all the provided types and methods are serialized as a string, and it's a bit difficult to recreate the original object without a custom serializer or impacting the event loop (for example provide the class path and import it in the triggerer)

@boring-cyborg boring-cyborg bot added area:providers provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues labels Nov 17, 2023
@hussein-awala hussein-awala marked this pull request as ready for review January 9, 2024 23:55
@hussein-awala hussein-awala changed the title [WIP] Create a generic callbacks class for KubernetesPodOperator Create a generic callbacks class for KubernetesPodOperator Jan 9, 2024
@eladkal
Copy link
Contributor

eladkal commented Jan 10, 2024

Also doc paragraph about this new cool ability is good to have

@hussein-awala
Copy link
Member Author

@eladkal could you check the Google provider check? is it's complicated, I can bump the min cncf-kubernetes version to avoid this complexity.

@hussein-awala hussein-awala requested a review from ashb as a code owner January 13, 2024 00:03
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

❤️ it

@potiuk
Copy link
Member

potiuk commented Jan 14, 2024

There is a test failure though :(

@hussein-awala
Copy link
Member Author

I just tested on a real cluster and found that the type I provide to the trigger is converted to a string. I even tried providing an object instance but had the same problem. That's because we don't serialize the trigger kwargs, but we keep sqlalchemy convert them to string (ExtendedJSON).

I need to revert the async part and keep it only for the sync part, and later we can add support for async callbacks once we find a clean solution for the issue; I will take this into account while working on the kwargs encryption.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants