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

Add a new lifetime property to the container process #1049

Merged

Conversation

cdavernas
Copy link
Member

Please specify parts of this PR update:

  • Specification
  • Schema
  • Examples
  • Extensions
  • Use Cases
  • Community
  • CTK
  • Other

Discussion or Issue link:
#1013

What this PR does:

  • Adds a new lifetime property to the container process

@cdavernas cdavernas added change: documentation Improvements or additions to documentation. It won't impact a version change. change: feature New feature or request. Impacts in a minor version change area: spec Changes in the Specification area: examples labels Jan 8, 2025
@cdavernas cdavernas added this to the v1.0.0 milestone Jan 8, 2025
@cdavernas cdavernas self-assigned this Jan 8, 2025
@cdavernas cdavernas linked an issue Jan 8, 2025 that may be closed by this pull request
Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

Should this control not be on the platform instead? In which cases would a user like to control the infra behavior from the workflow? I can see plenty since users are creative, but it is a dangerous mix of functional and non-functional requirements.

@cdavernas
Copy link
Member Author

Should this control not be on the platform instead?

Not really, no, as this should be configurable on a per-container basis. Some containers will just run dumb work which no one cares about double checking, whereas others might be critical and require auditing.
The PR addresses an issue that was long opened, and which addresses a couple of professional requirements.

@ricardozanini
Copy link
Member

Not really, no, as this should be configurable on a per-container basis

I agree. The platform should know the containers a given workflow would spawn, and an operator/admin could manage it.

My point is that in the long term, we will add new things like maxMemory, cpu, etc. Replicating what platforms like Kubernetes or even Docker Compose have already declared.

@cdavernas
Copy link
Member Author

cdavernas commented Jan 8, 2025

@ricardozanini Well, resource management is another story imho. The PR is just addressing some high level lifecycle management concerns.

Anyhow, we can close the PR and related issue as won't implement if you prefer 😉

@ricardozanini
Copy link
Member

No, I think it's valuable. I am just raising a concern about what can grow more complex if we open Pandora's box. We can keep this one as long as we don't start transforming the container object into a Kubernete's container/pod mimic.

@cdavernas cdavernas merged commit 897019c into serverlessworkflow:main Jan 9, 2025
3 checks passed
@cdavernas cdavernas deleted the feat-container-lifetime branch January 9, 2025 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: examples area: spec Changes in the Specification change: documentation Improvements or additions to documentation. It won't impact a version change. change: feature New feature or request. Impacts in a minor version change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add a property to configure the lifetime of spawned containers
2 participants