feat: hearth controller — FournosCluster lifecycle operator - #1
Merged
Conversation
Migrated from fournos/fournos-cluster/ (PR #74) to standalone repo. - kopf operator watching kubeconfig secrets for auto-discovery - GPU discovery via target cluster node labels - Kueue ResourceFlavor and ClusterQueue management - Cluster locking with sentinel FournosJob and TTL expiry - Backfill fix: new GPU types added to all flavors (prevents 422) - Deployment manifests (namespace, SA, ClusterRole, Deployment) - CI workflow for tests and container image build/push - 87 unit tests passing
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough This pull request adds a Kopf-based Kubernetes operator (Hearth) that auto-discovers remote clusters from kubeconfig Secrets, performs GPU discovery, manages Kueue ResourceFlavors/ClusterQueues, enforces sentinel-based cluster locks with optional TTLs, provides CRD/Deployment/RBAC manifests, CI, container build, and unit tests. Changes Core Operator Implementation
Kubernetes Resources and Deployment
Container Build and CI
Test Suite
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
Containerfile (1)
5-12: 💤 Low valueConsider documenting the two-stage pip install pattern.
The build creates a temporary
hearth/__init__.pystub to install dependencies (lines 5-9), then copies the real source and installs with--no-deps(lines 11-12). While this pattern caches dependencies in a separate layer for faster rebuilds, it may not be immediately clear to maintainers why two install steps are necessary.If this pattern is retained, a brief inline comment explaining the caching benefit would improve clarity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Containerfile` around lines 5 - 12, Add a concise inline comment above the two-stage pip install sequence explaining the purpose: that a temporary stub hearth/__init__.py is created so dependencies from pyproject.toml can be installed and cached in an earlier layer (pip install --no-cache-dir .), then the real source is copied and installed with pip install --no-cache-dir --no-deps . This comment should reference the temporary stub creation (mkdir -p hearth && touch hearth/__init__.py), the initial install (pip install --no-cache-dir .), the removal (rm -rf hearth), and the final install after COPY hearth/ to clarify the caching/fast-rebuild intent for future maintainers.pyproject.toml (2)
10-15: ⚡ Quick winConsider tightening dependency version constraints.
All dependencies use
>=constraints, which allow any future major version. This can introduce breaking changes unexpectedly. Consider using~=(compatible release) or explicit upper bounds to improve reproducibility and reduce risk of runtime breakage.For example:
dependencies = [ "kopf~=1.37", "kubernetes~=31.0", "pydantic-settings~=2.7", "pyyaml~=6.0", ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 10 - 15, The dependency entries using open-ended ">=" constraints (kopf, kubernetes, pydantic-settings, pyyaml) should be tightened to prevent inadvertent breaking upgrades; update the four entries ("kopf", "kubernetes", "pydantic-settings", "pyyaml") to use compatible-release operator "~=" or explicit upper bounds (e.g., "~=1.37", "~=31.0", "~=2.7", "~=6.0") in pyproject.toml so installs are reproducible and limited to compatible minor/patch releases.
5-8: 💤 Low valueConsider adding license and author metadata.
The project metadata lacks
licenseandauthors/maintainersfields. Adding these would improve the package's completeness, especially if it may be published or shared outside the immediate team.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 5 - 8, Add missing package metadata to pyproject.toml by adding a license field and an authors (or maintainers) list under the [project] table; specifically update the existing [project] block (name, version, description) to include e.g. license = "MIT" (or SPDX identifier) and authors = [{name = "Your Name", email = "you@example.com"}] or a maintainers = [...] entry so the package has proper license and contact metadata for distribution and discovery..github/workflows/ci.yaml (1)
40-55: ⚡ Quick winConsider adding container security scanning before push.
The
build-and-pushjob builds and pushes the container image without running a vulnerability scan. Adding a scan step (e.g., Trivy, Grype, or Quay's built-in scanning) would help catch known CVEs before images reach the registry, improving the security posture of deployed operators.Example addition after line 42:
- name: Scan container run: | podman run --rm -v /var/run/docker.sock:/var/run/docker.sock \ aquasec/trivy image --severity HIGH,CRITICAL hearth:${{ github.sha }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml around lines 40 - 55, Add a container scan step between the "Build container" and "Push container" steps so images are vulnerability-scanned before being pushed; create a step named "Scan container" (or similar) that runs a scanner such as Trivy or Grype against hearth:${{ github.sha }} (e.g., aquasec/trivy image --severity HIGH,CRITICAL hearth:${{ github.sha }}), mount any required socket/dirs or use the scanner's CLI container, and configure the command to exit non-zero on findings (so the job fails on HIGH/CRITICAL CVEs) before the existing "Login to Quay" / "Push container" steps.manifests/crd.yaml (1)
90-104: 💤 Low valueConsider adding validation for GPU inventory items.
The
hardware.gpusarray items have no required fields, allowing empty objects. While the operator controls these writes, addingrequired: ["vendor", "model", "count"]would provide schema-level validation and clearer API contracts.📋 Suggested validation enhancement
items: type: object + required: + - vendor + - model + - count properties: vendor: type: string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@manifests/crd.yaml` around lines 90 - 104, The CRD schema for hardware.gpus currently allows empty objects; add schema-level validation by specifying required fields for each GPU item (e.g., required: ["vendor","model","count"]) under the gpus.items properties so hardware.gpus entries must include vendor, model and count; update the gpus array item definition (the gpus.items object in the CRD) to include the required list to enforce this constraint.deploy/deployment.yaml (2)
15-16: 💤 Low valueConsider adding pod-level security context for defense in depth.
While the container security context is well configured (lines 35-41), adding a pod-level
securityContextwould provide an additional layer of security constraints.🛡️ Optional pod security context
spec: serviceAccountName: hearth + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: hearth🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/deployment.yaml` around lines 15 - 16, Add a pod-level securityContext under the same spec that contains serviceAccountName (the block with "spec: serviceAccountName: hearth") to enforce cluster-wide pod constraints; define fields such as runAsNonRoot: true, fsGroup (e.g., 1000), readOnlyRootFilesystem: true, and seccompProfile/runtimeClass as appropriate to your cluster, while keeping the existing container-level securityContext intact (the container's securityContext settings should remain unchanged) so the pod-wide policies apply in addition to per-container controls.
29-34: ⚡ Quick winAdd a readiness probe to prevent premature traffic.
The deployment defines a liveness probe but no readiness probe. Without a readiness probe, the pod may be marked as ready and receive traffic before the operator is fully initialized, potentially causing request failures during startup.
🏥 Add readiness probe
livenessProbe: httpGet: path: /healthz port: 8080 initialDelaySeconds: 10 periodSeconds: 30 + readinessProbe: + httpGet: + path: /healthz + port: 8080 + initialDelaySeconds: 5 + periodSeconds: 10🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/deployment.yaml` around lines 29 - 34, Add a readinessProbe alongside the existing livenessProbe to prevent the pod from receiving traffic before initialization completes: in the same container spec that contains livenessProbe, add a readinessProbe block (httpGet to /healthz on port 8080 or another readiness-specific endpoint) with sensible timings (e.g., initialDelaySeconds a bit longer or tuned, periodSeconds/failureThreshold) so readiness checks start after startup and only mark the pod ready when the app is truly ready; reference the existing livenessProbe and ensure readinessProbe uses the same port and path (or a dedicated readiness path) and appropriate initialDelaySeconds/periodSeconds/failureThreshold values.hearth/operator.py (1)
26-26: 💤 Low valueConsider using
force=Trueor checking existing handlers.Calling
logging.basicConfig()at runtime may be silently ignored if logging handlers are already configured (e.g., by Kopf or test frameworks). This could lead to unexpected log-level behavior.Proposed fix to ensure configuration applies
- logging.basicConfig(level=log_level, format="%(asctime)s %(name)s %(levelname)s %(message)s") + logging.basicConfig( + level=log_level, + format="%(asctime)s %(name)s %(levelname)s %(message)s", + force=True + )Note:
force=Truerequires Python 3.8+. If you need Python 3.7 compatibility, clear existing handlers first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hearth/operator.py` at line 26, The logging setup call in hearth.operator.py uses logging.basicConfig(...) which can be ignored if handlers already exist; update the configuration to ensure it takes effect by either adding force=True to the logging.basicConfig call or, for Python <3.8 compatibility, clear existing handlers first (e.g., inspect logging.root.handlers and remove them) before calling logging.basicConfig; update the single call where logging.basicConfig is invoked so the intended log_level and format are reliably applied at runtime.hearth/__main__.py (1)
6-9: 💤 Low valueConsider making the liveness endpoint configurable.
The liveness endpoint is hardcoded to
http://0.0.0.0:8080/healthz. If the port needs to change (e.g., for local development or different deployment environments), it requires a code change.Proposed refactor to make it configurable
In
hearth/settings.py, add:liveness_port: int = 8080Then update
hearth/__main__.py:kopf.run( namespaces=[settings.namespace, settings.secrets_namespace], - liveness_endpoint="http://0.0.0.0:8080/healthz", + liveness_endpoint=f"http://0.0.0.0:{settings.liveness_port}/healthz", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hearth/__main__.py` around lines 6 - 9, Make the liveness endpoint configurable by adding a setting (e.g., liveness_port: int = 8080) in hearth/settings.py and updating the call to kopf.run in hearth/__main__.py to construct the endpoint from that setting instead of hardcoding it; replace the literal "http://0.0.0.0:8080/healthz" with a generated string like "http://0.0.0.0:{settings.liveness_port}/healthz" (or allow an override via settings.liveness_endpoint if you prefer full-URL configurability) so kopf.run uses the configured port at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 1-8: The workflow titled "CI" lacks explicit GitHub Actions
permissions; add a permissions block either at the workflow top-level (under the
existing "name: CI") or per-job to enforce least-privilege, e.g., set contents:
read and packages: write only if you push to GitHub Container Registry,
otherwise just contents: read for external registries like Quay.io; ensure the
permissions entry is present alongside the existing on/push/pull_request
configuration so jobs like the main CI job obey least-privilege.
In `@deploy/clusterrole.yaml`:
- Around line 22-24: The ClusterRole currently grants cluster-wide access to
"secrets"; remove the secrets rule from the ClusterRole and instead create a
namespaced Role named "hearth-secrets" in the "psap-secrets" namespace with
verbs ["get","list","watch","patch"] on resources ["secrets"], then create a
RoleBinding named "hearth-secrets" in the "psap-secrets" namespace that binds
that Role to the "hearth" ServiceAccount (subject: kind ServiceAccount, name
hearth, namespace hearth); finally, keep all other non-secrets rules in the
existing ClusterRole and add the new Role and RoleBinding to your manifest
collection (kustomization) so they are applied together.
In `@deploy/deployment.yaml`:
- Around line 19-20: Replace the non-deterministic image tag
"quay.io/rh_perfscale/hearth:latest" with a specific semantic version or
immutable digest (e.g., "quay.io/rh_perfscale/hearth:v0.1.0" or a commit
SHA/digest) and ensure the corresponding imagePullPolicy is appropriate (you may
change imagePullPolicy from Always to IfNotPresent when using immutable tags);
update the "image: quay.io/rh_perfscale/hearth:latest" entry and adjust
"imagePullPolicy" accordingly.
In `@hearth/core/gpu_discovery.py`:
- Around line 74-102: Wrap all failure paths in the kubeconfig-fetch/parse logic
so they raise GPUDiscoveryError instead of letting raw exceptions escape: when
catching client.exceptions.ApiException, convert non-404 ApiException into
GPUDiscoveryError with context (include secret_name and namespace) instead of
re-raising; when decoding secret.data["kubeconfig"] (base64 decode and
yaml.safe_load) catch decoding/yaml exceptions and raise GPUDiscoveryError
describing the failure; similarly, if the parsed value is a base64-encoded
string and the second decode/safe_load fails, catch and raise GPUDiscoveryError;
ensure the final type check for parsed still raises GPUDiscoveryError on invalid
YAML so all paths consistently produce GPUDiscoveryError rather than raw
exceptions.
In `@hearth/core/kueue.py`:
- Around line 86-106: In add_flavor_to_cluster_queue, when you inject
CLUSTER_SLOT_RESOURCE into the flavor's resources (the block building resources
and appending to flavors), also update the corresponding rg["coveredResources"]
to include CLUSTER_SLOT_RESOURCE (if not already present) so coveredResources
stays consistent with the flavor resources used by update_flavor_quotas; ensure
you dedupe (no duplicate entries) and modify resource_groups/rg before calling
self._k8s.patch_cluster_custom_object so the patched spec contains the updated
coveredResources.
In `@manifests/crd.yaml`:
- Around line 10-11: The CRD's plural name is incorrect: change the `plural`
value from "fournoscluster" to the proper Kubernetes plural "fournosclusters" so
API paths and kubectl resource names are correct; update the `plural` field in
the CRD definition (the entry named `plural`) accordingly to match the resource
kind/CRD `singular` and follow convention.
---
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 40-55: Add a container scan step between the "Build container" and
"Push container" steps so images are vulnerability-scanned before being pushed;
create a step named "Scan container" (or similar) that runs a scanner such as
Trivy or Grype against hearth:${{ github.sha }} (e.g., aquasec/trivy image
--severity HIGH,CRITICAL hearth:${{ github.sha }}), mount any required
socket/dirs or use the scanner's CLI container, and configure the command to
exit non-zero on findings (so the job fails on HIGH/CRITICAL CVEs) before the
existing "Login to Quay" / "Push container" steps.
In `@Containerfile`:
- Around line 5-12: Add a concise inline comment above the two-stage pip install
sequence explaining the purpose: that a temporary stub hearth/__init__.py is
created so dependencies from pyproject.toml can be installed and cached in an
earlier layer (pip install --no-cache-dir .), then the real source is copied and
installed with pip install --no-cache-dir --no-deps . This comment should
reference the temporary stub creation (mkdir -p hearth && touch
hearth/__init__.py), the initial install (pip install --no-cache-dir .), the
removal (rm -rf hearth), and the final install after COPY hearth/ to clarify the
caching/fast-rebuild intent for future maintainers.
In `@deploy/deployment.yaml`:
- Around line 15-16: Add a pod-level securityContext under the same spec that
contains serviceAccountName (the block with "spec: serviceAccountName: hearth")
to enforce cluster-wide pod constraints; define fields such as runAsNonRoot:
true, fsGroup (e.g., 1000), readOnlyRootFilesystem: true, and
seccompProfile/runtimeClass as appropriate to your cluster, while keeping the
existing container-level securityContext intact (the container's securityContext
settings should remain unchanged) so the pod-wide policies apply in addition to
per-container controls.
- Around line 29-34: Add a readinessProbe alongside the existing livenessProbe
to prevent the pod from receiving traffic before initialization completes: in
the same container spec that contains livenessProbe, add a readinessProbe block
(httpGet to /healthz on port 8080 or another readiness-specific endpoint) with
sensible timings (e.g., initialDelaySeconds a bit longer or tuned,
periodSeconds/failureThreshold) so readiness checks start after startup and only
mark the pod ready when the app is truly ready; reference the existing
livenessProbe and ensure readinessProbe uses the same port and path (or a
dedicated readiness path) and appropriate
initialDelaySeconds/periodSeconds/failureThreshold values.
In `@hearth/__main__.py`:
- Around line 6-9: Make the liveness endpoint configurable by adding a setting
(e.g., liveness_port: int = 8080) in hearth/settings.py and updating the call to
kopf.run in hearth/__main__.py to construct the endpoint from that setting
instead of hardcoding it; replace the literal "http://0.0.0.0:8080/healthz" with
a generated string like "http://0.0.0.0:{settings.liveness_port}/healthz" (or
allow an override via settings.liveness_endpoint if you prefer full-URL
configurability) so kopf.run uses the configured port at runtime.
In `@hearth/operator.py`:
- Line 26: The logging setup call in hearth.operator.py uses
logging.basicConfig(...) which can be ignored if handlers already exist; update
the configuration to ensure it takes effect by either adding force=True to the
logging.basicConfig call or, for Python <3.8 compatibility, clear existing
handlers first (e.g., inspect logging.root.handlers and remove them) before
calling logging.basicConfig; update the single call where logging.basicConfig is
invoked so the intended log_level and format are reliably applied at runtime.
In `@manifests/crd.yaml`:
- Around line 90-104: The CRD schema for hardware.gpus currently allows empty
objects; add schema-level validation by specifying required fields for each GPU
item (e.g., required: ["vendor","model","count"]) under the gpus.items
properties so hardware.gpus entries must include vendor, model and count; update
the gpus array item definition (the gpus.items object in the CRD) to include the
required list to enforce this constraint.
In `@pyproject.toml`:
- Around line 10-15: The dependency entries using open-ended ">=" constraints
(kopf, kubernetes, pydantic-settings, pyyaml) should be tightened to prevent
inadvertent breaking upgrades; update the four entries ("kopf", "kubernetes",
"pydantic-settings", "pyyaml") to use compatible-release operator "~=" or
explicit upper bounds (e.g., "~=1.37", "~=31.0", "~=2.7", "~=6.0") in
pyproject.toml so installs are reproducible and limited to compatible
minor/patch releases.
- Around line 5-8: Add missing package metadata to pyproject.toml by adding a
license field and an authors (or maintainers) list under the [project] table;
specifically update the existing [project] block (name, version, description) to
include e.g. license = "MIT" (or SPDX identifier) and authors = [{name = "Your
Name", email = "you@example.com"}] or a maintainers = [...] entry so the package
has proper license and contact metadata for distribution and discovery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 455d227b-8a4f-47b0-9d69-7530269a228f
📒 Files selected for processing (29)
.github/workflows/ci.yaml.gitignoreContainerfiledeploy/clusterrole.yamldeploy/clusterrolebinding.yamldeploy/deployment.yamldeploy/kustomization.yamldeploy/namespace.yamldeploy/sa.yamlhearth/__init__.pyhearth/__main__.pyhearth/constants.pyhearth/core/__init__.pyhearth/core/gpu_discovery.pyhearth/core/kueue.pyhearth/handlers/__init__.pyhearth/handlers/cluster.pyhearth/handlers/secret.pyhearth/operator.pyhearth/settings.pyhearth/state.pymanifests/crd.yamlpyproject.tomltests/__init__.pytests/unit/__init__.pytests/unit/test_cluster_handlers.pytests/unit/test_gpu_discovery.pytests/unit/test_kueue.pytests/unit/test_secret_handlers.py
Comment on lines
+1
to
+8
| name: CI | ||
|
|
||
| on: | ||
| push: | ||
| branches: [main] | ||
| pull_request: | ||
| branches: [main] | ||
|
|
Contributor
There was a problem hiding this comment.
Add explicit permissions to workflow jobs.
The workflow does not specify explicit permissions for either job. GitHub Actions best practice recommends setting least-privilege permissions to reduce the attack surface if the workflow is compromised.
Consider adding at the workflow or job level:
permissions:
contents: read
packages: write # if pushing to GitHub Container RegistryFor Quay.io, contents: read is sufficient since authentication uses external secrets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yaml around lines 1 - 8, The workflow titled "CI" lacks
explicit GitHub Actions permissions; add a permissions block either at the
workflow top-level (under the existing "name: CI") or per-job to enforce
least-privilege, e.g., set contents: read and packages: write only if you push
to GitHub Container Registry, otherwise just contents: read for external
registries like Quay.io; ensure the permissions entry is present alongside the
existing on/push/pull_request configuration so jobs like the main CI job obey
least-privilege.
Comment on lines
+19
to
+20
| image: quay.io/rh_perfscale/hearth:latest | ||
| imagePullPolicy: Always |
Contributor
There was a problem hiding this comment.
Replace :latest image tag with a specific version.
Using :latest with imagePullPolicy: Always creates non-deterministic deployments and makes rollbacks difficult. Production deployments should use semantic version tags (e.g., v0.1.0 or commit SHAs) for reproducibility and proper version tracking.
🏷️ Recommended fix
containers:
- name: hearth
- image: quay.io/rh_perfscale/hearth:latest
- imagePullPolicy: Always
+ image: quay.io/rh_perfscale/hearth:v0.1.0 # or commit SHA
+ imagePullPolicy: IfNotPresent📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| image: quay.io/rh_perfscale/hearth:latest | |
| imagePullPolicy: Always | |
| image: quay.io/rh_perfscale/hearth:v0.1.0 # or commit SHA | |
| imagePullPolicy: IfNotPresent |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/deployment.yaml` around lines 19 - 20, Replace the non-deterministic
image tag "quay.io/rh_perfscale/hearth:latest" with a specific semantic version
or immutable digest (e.g., "quay.io/rh_perfscale/hearth:v0.1.0" or a commit
SHA/digest) and ensure the corresponding imagePullPolicy is appropriate (you may
change imagePullPolicy from Always to IfNotPresent when using immutable tags);
update the "image: quay.io/rh_perfscale/hearth:latest" entry and adjust
"imagePullPolicy" accordingly.
📝 CodeRabbit Chat: Implement requested code changes
- Scope secrets access to psap-secrets namespace via Role/RoleBinding instead of cluster-wide ClusterRole (least privilege) - Normalize all kubeconfig-read failures to GPUDiscoveryError so the handler's except path always fires (non-404 API errors, base64/yaml decode failures) - Keep coveredResources in sync when add_flavor_to_cluster_queue injects cluster-slot resource - Add troubleshooting entry for sentinel "Pipeline not found" error - Fix workload namespace in troubleshooting section - Add unit tests for all three code fixes (91 tests passing)
Run ruff format across all source and test files to pass CI format check. Also adds ImageStream trigger annotation to deployment and execution_namespace setting for sentinel job namespace configuration.
CI uses --cov flags but pytest-cov was missing from pyproject.toml, causing pytest to fail with "unrecognized arguments" (exit code 4).
Self-contained ArgoCD setup — no fournos-gitops changes needed. AppProject restricts hearth to its own namespace and resource types. ImageStream polls quay.io for new images; trigger annotation on Deployment handles automatic rollouts. Bootstrap: oc apply -f argocd/ -n openshift-gitops
kpouget
reviewed
May 18, 2026
kpouget
reviewed
May 18, 2026
Collaborator
|
thanks Mehul, just need to remove the paragraph on PipelineNotFound, and fix the plural name of FournosCluster and will be good 👍🏻 |
kpouget
reviewed
May 18, 2026
The CRD plural field must end with 's' to follow Kubernetes naming conventions. Updates metadata.name, RBAC resources, Python constant, README, and sentinel job prefix (hearth-lock- instead of cluster-lock-).
Set kubernetes.client.rest and urllib3 loggers to WARNING regardless of HEARTH_LOG_LEVEL, preventing full HTTP response bodies (including kubeconfig secrets) from being dumped at DEBUG level.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
README.md (1)
32-38: 💤 Low valueAdd language identifier to fenced code block.
The CD flow diagram lacks a language identifier. Adding
textsatisfies linters and improves rendering consistency.Suggested fix
-``` +```text PR merged to main -> GitHub Actions builds + pushes quay.io/rh_perfscale/hearth:latest -> OpenShift ImageStream detects new image (~15 min poll) -> image trigger rolls out new pod -> ArgoCD ignores image field (ignoreDifferences) -``` +```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 32 - 38, The fenced code block in README.md that contains the CD flow diagram is missing a language identifier; update the opening fence for that block in the README (the block that starts with ```) to use ```text so linters and renderers treat it as plain text and rendering is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@README.md`:
- Around line 32-38: The fenced code block in README.md that contains the CD
flow diagram is missing a language identifier; update the opening fence for that
block in the README (the block that starts with ```) to use ```text so linters
and renderers treat it as plain text and rendering is consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1dab1b9-6341-41ed-af98-486e27c17e7f
📒 Files selected for processing (21)
README.mdargocd/app-hearth.yamlargocd/appproject.yamldeploy/clusterrole.yamldeploy/deployment.yamldeploy/imagestream.yamldeploy/kustomization.yamldeploy/secrets-role.yamldeploy/secrets-rolebinding.yamlhearth/constants.pyhearth/core/gpu_discovery.pyhearth/core/kueue.pyhearth/handlers/cluster.pyhearth/operator.pyhearth/settings.pymanifests/crd.yamlpyproject.tomltests/unit/test_cluster_handlers.pytests/unit/test_gpu_discovery.pytests/unit/test_kueue.pytests/unit/test_secret_handlers.py
✅ Files skipped from review due to trivial changes (5)
- deploy/imagestream.yaml
- deploy/secrets-rolebinding.yaml
- deploy/secrets-role.yaml
- argocd/app-hearth.yaml
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (9)
- hearth/constants.py
- manifests/crd.yaml
- hearth/settings.py
- tests/unit/test_secret_handlers.py
- tests/unit/test_gpu_discovery.py
- hearth/core/gpu_discovery.py
- hearth/handlers/cluster.py
- tests/unit/test_kueue.py
- tests/unit/test_cluster_handlers.py
kopf watches all registered resource types in both configured namespaces. Add Role/RoleBinding for secrets in the hearth namespace so kopf's watch loop doesn't spam APIForbiddenError on startup.
Collaborator
|
/lgtm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fournos/fournos-cluster/(PR #74) to standalone repofournos_cluster→hearth, env prefixHEARTH_, namespacehearthhearth(no more ArgoCD fights with sharedfournos-clusterrole)quay.io/rh_perfscale/hearthWhat's included
hearth/— kopf operator (auto-discovery, GPU detection, Kueue management, locking, TTL)tests/— 87 unit testsdeploy/— Kustomize manifests (namespace, SA, ClusterRole, Deployment)manifests/crd.yaml— FournosCluster CRD.github/workflows/ci.yaml— CI pipelineVerified on psap-automation cluster
hearthnamespacefournos-fg(2x NVIDIA-L40S) andathena-fire(8x NVIDIA)ignoreDifferencesPR (openshift-psap/fournos-gitops#10)Test plan
Summary by CodeRabbit
New Features
Chores
Tests
Documentation