Allow clusterless jobs - #99
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds ChangesClusterless job execution
FORGE deployment orchestration
WIP runtime workflow
Kopf object logging
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FournosJob
participant Lifecycle
participant Resolving
participant Kueue
participant Execution
participant Tekton
FournosJob->>Lifecycle: submit clusterless spec
Lifecycle->>Resolving: transition to Resolving
Resolving->>Resolving: set Admitted and [clusterless]
Resolving-->>Kueue: do not create Workload
Resolving->>Execution: reconcile admitted job
Execution->>Tekton: create PipelineRun
Tekton-->>Execution: use kubeconfig placeholder
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
goes along openshift-psap/forge#123 |
|
/test deploy-fournos-wip |
|
/test deploy-fournos-wip |
1 similar comment
|
/test deploy-fournos-wip |
|
/test deploy-fournos-wip |
|
sample clusterless deployment there: the job submission is out of scope for this pair of PR, it will come next. |
|
/test deploy-fournos-wip |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/forge/deploy/orchestration/deploy.py (1)
707-782: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
forgeconfig section inconfig.yamlis not consumed bydeploy_workflow_config()indeploy.py. The root cause is a missing wiring between the configuration and the code that should read it —deploy_workflow_config()hardcodes"base","images","workflows", and"kustomization.yaml"instead of reading theforge.source_base,forge.images_path,forge.workflows_path, andforge.skip_filesvalues.
tests/forge/deploy/orchestration/deploy.py#L707-L782: replace hardcoded paths and skip-file names withconfig.project.get_config()calls forfournos_deploy.forge.source_base,fournos_deploy.forge.images_path,fournos_deploy.forge.workflows_path, andfournos_deploy.forge.skip_files.tests/forge/deploy/orchestration/config.yaml#L72-L85: no change needed to the config itself; it is correctly defined but currently dead. Once the code reads these values, the config becomes the source of truth.🤖 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 `@tests/forge/deploy/orchestration/deploy.py` around lines 707 - 782, Update deploy_workflow_config() in tests/forge/deploy/orchestration/deploy.py to read fournors_deploy.forge.source_base, images_path, workflows_path, and skip_files via config.project.get_config() instead of hardcoding "base", directory names, or "kustomization.yaml"; apply these configured values when constructing paths and filtering manifests. No direct change is needed in tests/forge/deploy/orchestration/config.yaml lines 72-85 because its existing forge settings are already correct and become active through this wiring.fournos/core/tekton.py (1)
95-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the placeholder kubeconfig secret name into a named constant.
"fournos-clusterless-placeholder"is duplicated betweentekton.py(line 99) andtests/test_clusterless.py(line 137). A named constant inconstants.pywould prevent drift if the value changes.♻️ Proposed refactor
# In fournos/core/constants.py +CLUSTERLESS_KUBECONFIG_PLACEHOLDER = "fournos-clusterless-placeholder" # In fournos/core/tekton.py { "name": "kubeconfig-secret", "value": kubeconfig_secret if kubeconfig_secret - else "fournos-clusterless-placeholder", + else CLUSTERLESS_KUBECONFIG_PLACEHOLDER, }, # In tests/test_clusterless.py - assert kubeconfig_param == "fournos-clusterless-placeholder", ( + assert kubeconfig_param == CLUSTERLESS_KUBECONFIG_PLACEHOLDER, (🤖 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 `@fournos/core/tekton.py` around lines 95 - 100, Define a named constant in constants.py for the clusterless kubeconfig placeholder value, then update the kubeconfig-secret construction in tekton.py to use that constant instead of the inline string. Update tests/test_clusterless.py to reference the same constant, preserving the existing fallback value.
🤖 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 `@Fournos_Design_Document.md`:
- Line 125: Update the hardware requirement statement in the design document’s
exclusive/non-exclusive job rules to match the README and tests: non-exclusive
jobs may omit hardware and still succeed. Preserve the existing exceptions for
exclusive cluster-locked and clusterless jobs, and ensure the wording no longer
claims hardware is always required for non-exclusive jobs.
---
Nitpick comments:
In `@fournos/core/tekton.py`:
- Around line 95-100: Define a named constant in constants.py for the
clusterless kubeconfig placeholder value, then update the kubeconfig-secret
construction in tekton.py to use that constant instead of the inline string.
Update tests/test_clusterless.py to reference the same constant, preserving the
existing fallback value.
In `@tests/forge/deploy/orchestration/deploy.py`:
- Around line 707-782: Update deploy_workflow_config() in
tests/forge/deploy/orchestration/deploy.py to read
fournors_deploy.forge.source_base, images_path, workflows_path, and skip_files
via config.project.get_config() instead of hardcoding "base", directory names,
or "kustomization.yaml"; apply these configured values when constructing paths
and filtering manifests. No direct change is needed in
tests/forge/deploy/orchestration/config.yaml lines 72-85 because its existing
forge settings are already correct and become active through this wiring.
🪄 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: 53a79085-a7da-4e34-882b-0365785dad28
📒 Files selected for processing (14)
Fournos_Design_Document.mdMakefileREADME.mdfournos/core/tekton.pyfournos/handlers/execution.pyfournos/handlers/lifecycle.pyfournos/handlers/resolving.pyfournos/operator.pymanifests/crd.yamltests/forge/deploy/orchestration/ci.pytests/forge/deploy/orchestration/config.yamltests/forge/deploy/orchestration/deploy.pytests/test_clusterless.pytests/test_resolving.py
|
/test ? |
|
/test deploy-fournos-wip-from-forge-wip |
|
@kpouget: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/test deploy-fournos-wip-from-forge-wip |
|
/test deploy-fournos-wip-from-forge-wip |
| "name": "kubeconfig-secret", | ||
| "value": kubeconfig_secret | ||
| if kubeconfig_secret | ||
| else "fournos-clusterless-placeholder", |
There was a problem hiding this comment.
Can we make kubeconfig-secret an optional param in the Tekton pipeline spec with a default of "" and skip injecting it when None?
There was a problem hiding this comment.
not possible unfortunately, we need the value for requesting the kubeconfig secret, and it can't work with an empty value
(I tried that dead end twice already 😛)
|
|
||
| clusterless = spec.get("clusterless", False) | ||
| if clusterless: | ||
| # Skip Kueue entirely for clusterless jobs - go directly to Admitted phase |
There was a problem hiding this comment.
this bypass happens at the very end, so every clusterless job still spins up a resolve Job and waits for it before skipping Kueue
There was a problem hiding this comment.
yes, the resolve step is important to fill the fjob with the secrets required for running the job (in addition to telling the HW request -- that we don't use so far)
so it's on purpose that the resolve job runs first 👍🏻 (and it's not queued, so it should run immediately and take just a few seconds)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/forge/deploy/orchestration/deploy.py (1)
852-859: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant alias function.
rebuild_workflow_images()is a straight pass-through torebuild_forge_images(). Per the CLI (tests/forge/deploy/orchestration/cli.py), therebuild_workflowcommand already callsrebuild_forge_images()directly, so this alias appears to exist only fordeploy()'s internal step naming. Consider callingrebuild_forge_images()directly fromdeploy()instead of maintaining a duplicate wrapper.🤖 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 `@tests/forge/deploy/orchestration/deploy.py` around lines 852 - 859, Remove the redundant rebuild_workflow_images() wrapper and update deploy() to invoke rebuild_forge_images() directly for that internal step. Preserve the existing CLI behavior and return-value handling.
🤖 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 `@config/fournos-validation/workflows/tasks.yaml`:
- Line 9: The Task currently passes an empty secretName into its volume
configuration, which can cause Pod creation to fail. Update the Task definition
around the secretName parameter and its referenced volume so clusterless runs
skip the Task or use a valid placeholder secret name with optional handling;
preserve normal secret-backed execution when a real parameter is provided.
In `@tests/forge/deploy/orchestration/deploy.py`:
- Around line 706-781: Update deploy_workflow_config() to read the FORGE
deployment settings for source_base, images_path, workflows_path, and skip_files
from the configured fournous_deploy.forge configuration instead of hardcoding
"gitops/base", "images", "workflows", and "kustomization.yaml". Use those
configured values when constructing forge_source, selecting manifests, and
filtering skipped files, while preserving the existing deployment flow and
return behavior.
- Around line 796-801: Update the deployment flow around rebuild_forge_images()
and the force_rebuild configuration: either use force_rebuild to conditionally
gate the image refresh/import operation, or remove the configuration lookup and
“Force refresh” log if refreshes should always run. Ensure no unused flag
remains.
---
Nitpick comments:
In `@tests/forge/deploy/orchestration/deploy.py`:
- Around line 852-859: Remove the redundant rebuild_workflow_images() wrapper
and update deploy() to invoke rebuild_forge_images() directly for that internal
step. Preserve the existing CLI behavior and return-value handling.
🪄 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: 57b75672-4791-495c-9edf-fcefb064704c
📒 Files selected for processing (9)
config/fournos-validation/workflows/pipeline-validate-only.yamlconfig/fournos-validation/workflows/tasks.yamldev/mock-pipelines/pipeline-full.yamldev/mock-pipelines/pipeline-run-only.yamldev/mock-pipelines/tasks.yamlfournos/core/tekton.pyfournos/handlers/lifecycle.pytests/forge/deploy/orchestration/config.yamltests/forge/deploy/orchestration/deploy.py
🚧 Files skipped from review as they are similar to previous changes (2)
- fournos/handlers/lifecycle.py
- tests/forge/deploy/orchestration/config.yaml
0825de8 to
bed40d2
Compare
|
/test deploy-fournos-wip-from-forge-wip |
|
/test deploy-fournos-wip-from-forge-wip |
|
/test deploy-fournos-wip-from-forge-wip |
Co-authored-by: alberto <aperdomo@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/forge/deploy/orchestration/deploy.py (1)
730-782: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
deploy_workflow_config()ignores the configured FORGE GitOps layout.The deployment config documents
source_base,images_path,workflows_path, andskip_filesas configurable underfournos_deploy.forge. This function instead hardcodes the paths directly in Python. Any change to these config values will silently have no effect, breaking the config/code contract.Please dynamically fetch these values from the config to construct paths.
♻️ Proposed fix to read paths from the configuration
- forge_source = Path(forge_home) / "fournos" / "gitops" / "base" + forge_config = config.project.get_config("fournos_deploy.forge") + source_base = forge_config.get("source_base", "base") + images_path = forge_config.get("images_path", "images") + workflows_path = forge_config.get("workflows_path", "workflows") + skip_files = set(forge_config.get("skip_files", ["kustomization.yaml"])) + + fournos_dir = Path(forge_home) / "fournos" + forge_source = fournos_dir / "gitops" / source_base if not forge_source.exists(): raise ValueError(f"FORGE GitOps directory not found: {forge_source}") logger.info(f"Deploying workflow config from: {forge_source}") logger.info(f"Target namespace: {namespace}") # Deploy images (ImageStreams) - images_dir = forge_source / "images" + images_dir = forge_source / images_path if images_dir.exists(): image_manifests = [ - f for f in images_dir.glob("*.yaml") if f.name != "kustomization.yaml" + f for f in images_dir.glob("*.yaml") if f.name not in skip_files ] image_manifest_paths = [ - str(f.relative_to(forge_source.parent.parent)) for f in image_manifests + str(f.relative_to(fournos_dir)) for f in image_manifests ] if image_manifest_paths: logger.info(f"Deploying {len(image_manifest_paths)} image manifest(s)") result = _deploy_manifest_list( image_manifest_paths, namespace, - forge_source.parent.parent, + fournos_dir, set(), # No skip kinds for workflow config "image-manifest", ) if result != 0: return result # Deploy workflows (Pipelines and Tasks) - workflows_dir = forge_source / "workflows" + workflows_dir = forge_source / workflows_path if workflows_dir.exists(): workflow_manifests = [ - f for f in workflows_dir.glob("*.yaml") if f.name != "kustomization.yaml" + f for f in workflows_dir.glob("*.yaml") if f.name not in skip_files ] workflow_manifest_paths = [ - str(f.relative_to(forge_source.parent.parent)) for f in workflow_manifests + str(f.relative_to(fournos_dir)) for f in workflow_manifests ] if workflow_manifest_paths: logger.info( f"Deploying {len(workflow_manifest_paths)} workflow manifest(s)" ) result = _deploy_manifest_list( workflow_manifest_paths, namespace, - forge_source.parent.parent, + fournos_dir, set(), # No skip kinds for workflow config "workflow-manifest", ) if result != 0: return result🤖 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 `@tests/forge/deploy/orchestration/deploy.py` around lines 730 - 782, Update deploy_workflow_config() to read source_base, images_path, workflows_path, and skip_files from the configured fournos_deploy.forge settings instead of hardcoding “fournos/gitops/base”, “images”, “workflows”, and only excluding kustomization.yaml. Use source_base to construct forge_source, the configured subpaths to locate manifests, and skip_files when filtering manifests while preserving the existing deployment and error-handling flow.
🤖 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 `@README.md`:
- Line 145: Update the fenced code block containing the lifecycle diagram in
README.md to specify the text language, preserving the diagram content
unchanged.
---
Duplicate comments:
In `@tests/forge/deploy/orchestration/deploy.py`:
- Around line 730-782: Update deploy_workflow_config() to read source_base,
images_path, workflows_path, and skip_files from the configured
fournos_deploy.forge settings instead of hardcoding “fournos/gitops/base”,
“images”, “workflows”, and only excluding kustomization.yaml. Use source_base to
construct forge_source, the configured subpaths to locate manifests, and
skip_files when filtering manifests while preserving the existing deployment and
error-handling flow.
🪄 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: f8871b01-a4dc-4798-9780-d2f4e6158d9d
📒 Files selected for processing (15)
Fournos_Design_Document.mdMakefileREADME.mdconfig/fournos-validation/workflows/tasks.yamlfournos/core/tekton.pyfournos/handlers/execution.pyfournos/handlers/lifecycle.pyfournos/handlers/resolving.pyfournos/operator.pymanifests/crd.yamltests/forge/deploy/orchestration/ci.pytests/forge/deploy/orchestration/config.yamltests/forge/deploy/orchestration/deploy.pytests/test_clusterless.pytests/test_resolving.py
🚧 Files skipped from review as they are similar to previous changes (9)
- fournos/operator.py
- tests/forge/deploy/orchestration/ci.py
- Fournos_Design_Document.md
- fournos/handlers/resolving.py
- fournos/handlers/lifecycle.py
- fournos/handlers/execution.py
- tests/test_clusterless.py
- tests/test_resolving.py
- tests/forge/deploy/orchestration/config.yaml
|
/lgtm |
|
thanks @albertoperdomo2 , merging |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kpouget The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation