Skip to content

Cleaned simple kubernetes manager #66

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 5 commits into
base: master
Choose a base branch
from

Conversation

iripiri
Copy link
Contributor

@iripiri iripiri commented Jul 1, 2025

Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
@iripiri iripiri requested review from Copilot and stv0g July 1, 2025 16:04
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a simplified Kubernetes manager and job schema, refactors event watcher initialization, and updates related configurations.

  • Added a JSON schema for “Simple Kubernetes Job” definitions.
  • Implemented KubernetesManagerSimple with a parameter builder and creation logic.
  • Refactored thread setup, logging, and configuration loading in the core controller and component classes.

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
villas/controller/schemas/manager/kubernetes-simple/create.yaml New JSON schema for simple Kubernetes jobs
villas/controller/controller.py Added TimeoutError catch and updated warning log
villas/controller/components/simulators/kubernetes.py Renamed stop parameter and added explicit state transition
villas/controller/components/managers/kubernetes_simple.py New simple manager and build_parameters helper
villas/controller/components/managers/kubernetes.py Removed unused watcher threads; improved namespace setup and event filtering
villas/controller/components/manager.py Registered kubernetes-simple in the factory
villas/controller/component.py Changed schema validation/storage logic and status mapping
etc/params_k8s_dpsim.yaml Aligned activeDeadlineSeconds and ttlSecondsAfterFinished to 1h
etc/config_simplekub.yaml Added configuration snippet for the simple Kubernetes manager
Comments suppressed due to low confidence (2)

villas/controller/components/simulators/kubernetes.py:197

  • [nitpick] The parameter name 'message' is inconsistent with other simulator methods using 'payload'; consider renaming it to 'payload' for clarity.
    def stop(self, message):

villas/controller/component.py:155

  • This mapping now returns raw schema dicts, but the status consumer may expect validator objects with a .schema attribute. Ensure downstream code handles the new format or restore the previous .schema access.
                name: v for name, v in self.schema.items()

@@ -84,6 +84,8 @@ def _drain_publish_queue(self):
self.producer.publish(body, **kwargs)
except queue.Empty:
pass
except TimeoutError:
LOGGER.warn('TimeoutError, let kombu reconnect..')
Copy link
Preview

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

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

Use LOGGER.warning instead of the deprecated LOGGER.warn for consistency with the Python logging API.

Suggested change
LOGGER.warn('TimeoutError, let kombu reconnect..')
LOGGER.warning('TimeoutError, let kombu reconnect..')

Copilot uses AI. Check for mistakes.

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


def build_parameters(sim_name, jobname, image, adls=3600, privileged=False, name=None, uuid=None):
Copy link
Preview

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

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

[nitpick] The parameter 'adls' is unclear—consider renaming it to 'active_deadline_seconds' to improve readability.

Suggested change
def build_parameters(sim_name, jobname, image, adls=3600, privileged=False, name=None, uuid=None):
def build_parameters(sim_name, jobname, image, active_deadline_seconds=3600, privileged=False, name=None, uuid=None):

Copilot uses AI. Check for mistakes.

Comment on lines +87 to +88
except TimeoutError:
LOGGER.warn('TimeoutError, let kombu reconnect..')
Copy link
Preview

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

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

[nitpick] Include the caught exception in the log message to aid debugging, e.g., LOGGER.warning('TimeoutError occurred, reconnecting: %s', err).

Suggested change
except TimeoutError:
LOGGER.warn('TimeoutError, let kombu reconnect..')
except TimeoutError as err:
LOGGER.warn('TimeoutError occurred, let kombu reconnect: %s', err)

Copilot uses AI. Check for mistakes.

iripiri added 4 commits July 2, 2025 09:42
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
…mespace

Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
Signed-off-by: iripiri <ikoester@eonerc.rwth-aachen.de>
@iripiri iripiri force-pushed the cleaned-simple-kubernetes-manager branch from a62b7c2 to d78de89 Compare July 2, 2025 07:43
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.

1 participant