Skip to content

Add manager for simple Kubernetes job creation #47

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

Open
wants to merge 92 commits into
base: master
Choose a base branch
from

Conversation

stv0g
Copy link
Contributor

@stv0g stv0g commented Dec 2, 2022

In GitLab by @iripiri on Sep 23, 2021, 13:35

relates to #26

@iripiri iripiri force-pushed the simple-kubernetes-manager branch from 7a3a130 to 0c166d5 Compare August 14, 2023 14:38
iripiri and others added 29 commits August 14, 2023 16:42
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
…eter and config file

Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
…added to the controller

Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
rather set their state to stopped

Signed-off-by: Iris Koester <iris.m.koester@gmail.com>
@stv0g stv0g changed the title add manager for simple kubernetes job creation Add manager for simple kubernetes job creation Mar 25, 2025
iripiri added 8 commits March 26, 2025 08:23
…t and test stages

Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
@stv0g stv0g changed the title Add manager for simple kubernetes job creation Add manager for simple Kubernetes job creation Mar 31, 2025
iripiri added 9 commits March 31, 2025 10:34
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
@stv0g
Copy link
Contributor Author

stv0g commented Apr 2, 2025

Hi @iripiri,

Could we somehow break this PR up? Its containing over 80 commits which is almost impossible to review..

Or, I see a lot of small bug fixing commits, could you squash those into the respective commits to which they logically belong? (Not everthing into a single commit, as this would be equally hard to review :/).

@iripiri
Copy link
Contributor

iripiri commented Apr 3, 2025

Hi @stv0g,
I will do so! Though I'm not completely done here, I am testing & fixing right now, it's been a long time..
Could you review #61 first? That one is done

iripiri and others added 5 commits April 22, 2025 16:04
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>

pipeline fixes

Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>

updatet setuptools version to support license expression

Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>

update license definition

Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>

read version dynamically

Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>

fixes

Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>

move version to correct init file

Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
…t to find controller executable

Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
@stv0g
Copy link
Contributor Author

stv0g commented May 21, 2025

Could you review #61 first? That one is done

#61 is merged.

I think we are almost ready to merge this one, after the conflicts have been resolved.

Copy link
Contributor Author

@stv0g stv0g left a comment

Choose a reason for hiding this comment

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

First little round of review is done.

Ideally, we should split this PR into two:

  • Migration to pyproject.toml
  • New kubernetes-simple component

@@ -0,0 +1,17 @@
---
broker:
url: amqp://admin:vieQuoo2sieDahHee8ohM5aThaibiPei@villas-broker:5672/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a production credential? Looks like your RabbitMQ deployment in K8s?

'jsonschema>=4.1.0',
'psutil',
'pyusb'
],
data_files=[
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cant we add this to pyproject.toml as well?

Glob expressions are supported there as well:

https://setuptools.pypa.io/en/latest/userguide/datafiles.html#package-data

Comment on lines +179 to +180
# job isn't immediately deleted
# let the user see that something is happening
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
# job isn't immediately deleted
# let the user see that something is happening
# Job isn't immediately deleted.
# Let the user see that something is happening.

@@ -173,6 +176,9 @@ def _delete_job(self):
self.job = None
self.properties['job_name'] = None
self.properties['pod_names'] = []
# job isn't immediately deleted
# let the user see that something is happening
time.sleep(7)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but this is a bad.. Why should we artifically block execution here?
This also stops the controller from processing other actions and stuff..

from villas.controller.components.managers.kubernetes import KubernetesManager
from villas.controller.components.simulators.kubernetes import KubernetesJob

parameters_simple = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please turn this into a function which constructs the dictionary and returns it.

The patching of the struct below is not nice, since it modifies the global variable.

name = params.get('name')
privileged = params.get('privileged', False)
uuid = params.get('uuid')
self.logger.info('uuid:')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please improve the logging message here. This does not need to be spread across two invocations.

Comment on lines +61 to +62
super().create(payload)
return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
super().create(payload)
return
return super().create(payload)

Comment on lines +64 to +79
parameters = parameters_simple
parameters['name'] = sim_name
job = parameters['properties']['job']
job['metadata']['name'] = jobname
job['spec']['activeDeadlineSeconds'] = adls
job_container = job['spec']['template']['spec']['containers'][0]
job_container['image'] = image
job_container['securityContext']['privileged'] = privileged

parameters['job'] = job

if name:
parameters['name'] = name

if uuid:
parameters['uuid'] = uuid
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As noted above, please construct the parameters within a dedicated function.

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.

3 participants