Skip to content

DPTP-5114: Split scanner CPU/memory request and limit configuration - #87

Merged
openshift-merge-bot[bot] merged 2 commits into
openshift:mainfrom
richardsonnick:resource-request-limit
Jul 30, 2026
Merged

DPTP-5114: Split scanner CPU/memory request and limit configuration#87
openshift-merge-bot[bot] merged 2 commits into
openshift:mainfrom
richardsonnick:resource-request-limit

Conversation

@richardsonnick

@richardsonnick richardsonnick commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Replaces SCANNER_CPU and SCANNER_MEM with SCANNER_CPU_{REQUEST,LIMIT} SCANNER_MEM_{REQUEST,LIMIT} while keeping the (now deprecated) SCANNER_CPU and SCANNER_MEM for for backwards compat with existing workflows.

Replace the SCANNER_CPU and SCANNER_MEM variables with separate
SCANNER_CPU_REQUEST/LIMIT and SCANNER_MEM_REQUEST/LIMIT variables in
deploy.sh and the scanner job template. This allows a lower CPU request
(e.g. 500m) for scheduling while keeping a higher limit (4) for bursting.
The old SCANNER_CPU and SCANNER_MEM variables are retained as deprecated
fallbacks that set both request and limit when the new split variables
are not explicitly configured.
@openshift-ci
openshift-ci Bot requested review from rhmdnd and smith-xyz July 30, 2026 15:22
@openshift-ci

openshift-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardsonnick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2026
@richardsonnick richardsonnick changed the title Split scanner CPU/memory request and limit configuration DPTP-5114: Split scanner CPU/memory request and limit configuration Jul 30, 2026
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 30, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 30, 2026

Copy link
Copy Markdown

@richardsonnick: This pull request references DPTP-5114 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Replaces SCANNER_CPU and SCANNER_MEM with SCANNER_CPU_{REQUEST,LIMIT} SCANNER_MEM_{REQUEST,LIMIT} while keeping the (now deprecated) SCANNER_CPU and SCANNER_MEM for for backwards compat with existing workflows.

Instructions 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 openshift-eng/jira-lifecycle-plugin repository.

@qodo-for-rh-openshift

qodo-for-rh-openshift Bot commented Jul 30, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 10 rules

Grey Divider


Remediation recommended

1. Unvalidated request/limit mismatch ✓ Resolved 🐞 Bug ☼ Reliability
Description
deploy_scanner_job now allows SCANNER_*_REQUEST and SCANNER_*_LIMIT to be set independently and
renders them into the Job without validation. If a user sets a request higher than its limit, the
manifest will be rejected during oc apply API validation and the script provides no targeted
preflight check to catch it earlier with a clearer message.
Code

deploy.sh[R219-225]

+    SCANNER_CPU_REQUEST="${SCANNER_CPU_REQUEST:-${SCANNER_CPU:-500m}}"
+    SCANNER_CPU_LIMIT="${SCANNER_CPU_LIMIT:-${SCANNER_CPU:-4}}"
+    SCANNER_MEM_REQUEST="${SCANNER_MEM_REQUEST:-${SCANNER_MEM:-4Gi}}"
+    SCANNER_MEM_LIMIT="${SCANNER_MEM_LIMIT:-${SCANNER_MEM:-4Gi}}"
    SCANNER_PARALLEL="${SCANNER_PARALLEL:-4}"
    ARTIFACT_WAIT="${ARTIFACT_WAIT:-30}"
-    sed -e "s|\\\${SCANNER_IMAGE}|${SCANNER_IMAGE}|g" -e "s|\\\${NAMESPACE}|${NAMESPACE}|g" -e "s|\\\${JOB_NAME}|${JOB_NAME}|g" -e "s|\\\${NAMESPACE_FILTER_ARG}|${NAMESPACE_FILTER_ARG}|g" -e "s|\\\${LIMIT_IPS_ARG}|${LIMIT_IPS_ARG}|g" -e "s|\\\${STARTTLS_PORTS_ARG}|${STARTTLS_PORTS_ARG}|g" -e "s|\\\${SCANNER_CPU:-4}|${SCANNER_CPU}|g" -e "s|\\\${SCANNER_MEM:-4Gi}|${SCANNER_MEM}|g" -e "s|\\\${SCANNER_PARALLEL:-4}|${SCANNER_PARALLEL}|g" -e "s|\\\${ARTIFACT_WAIT:-300}|${ARTIFACT_WAIT}|g" "$JOB_TEMPLATE" | oc apply -f -
+    sed -e "s|\\\${SCANNER_IMAGE}|${SCANNER_IMAGE}|g" -e "s|\\\${NAMESPACE}|${NAMESPACE}|g" -e "s|\\\${JOB_NAME}|${JOB_NAME}|g" -e "s|\\\${NAMESPACE_FILTER_ARG}|${NAMESPACE_FILTER_ARG}|g" -e "s|\\\${LIMIT_IPS_ARG}|${LIMIT_IPS_ARG}|g" -e "s|\\\${STARTTLS_PORTS_ARG}|${STARTTLS_PORTS_ARG}|g" -e "s|\\\${SCANNER_CPU_REQUEST:-500m}|${SCANNER_CPU_REQUEST}|g" -e "s|\\\${SCANNER_CPU_LIMIT:-4}|${SCANNER_CPU_LIMIT}|g" -e "s|\\\${SCANNER_MEM_REQUEST:-4Gi}|${SCANNER_MEM_REQUEST}|g" -e "s|\\\${SCANNER_MEM_LIMIT:-4Gi}|${SCANNER_MEM_LIMIT}|g" -e "s|\\\${SCANNER_PARALLEL:-4}|${SCANNER_PARALLEL}|g" -e "s|\\\${ARTIFACT_WAIT:-300}|${ARTIFACT_WAIT}|g" "$JOB_TEMPLATE" | oc apply -f -
Relevance

●● Moderate

They accept input-validation adds, but req/limit comparison may be deemed unnecessary complexity.

PR-#74
PR-#40

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR introduces independent request/limit variables in deploy.sh and uses them to substitute
distinct resources.requests and resources.limits fields in the Job template, but there is no
validation step before piping the result into oc apply (so invalid combinations fail at
apply-time).

deploy.sh[215-226]
scanner-job.yaml.template[45-51]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`deploy.sh` now supports separate request/limit values, but it doesn’t validate that the rendered Job is API-valid before applying it. This can lead to user-facing `oc apply` failures when requests/limits are inconsistent.

## Issue Context
The script renders `scanner-job.yaml.template` via `sed` and pipes directly to `oc apply -f -`, so invalid resource configurations (or other template issues) only show up as an apply-time failure.

## Fix Focus Areas
- deploy.sh[215-226]
- scanner-job.yaml.template[45-51]

## Suggested fix
- Render the template to a temp file.
- Run `oc apply --dry-run=server -f <rendered>` (or `oc create --dry-run=server`) and on failure, print a specific error like: "Invalid Job manifest (check SCANNER_*_REQUEST/LIMIT values)" and exit.
- Only run the real `oc apply` after the dry-run succeeds.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Brittle sed placeholder matching 🐞 Bug ⚙ Maintainability
Description
deploy.sh sed substitutions match template placeholders including their default literals (e.g.
${SCANNER_CPU_REQUEST:-500m}). If the template defaults change or an alternate template uses
different defaults, the placeholders may stop matching and ${...} strings can be left in the
applied YAML.
Code

deploy.sh[225]

+    sed -e "s|\\\${SCANNER_IMAGE}|${SCANNER_IMAGE}|g" -e "s|\\\${NAMESPACE}|${NAMESPACE}|g" -e "s|\\\${JOB_NAME}|${JOB_NAME}|g" -e "s|\\\${NAMESPACE_FILTER_ARG}|${NAMESPACE_FILTER_ARG}|g" -e "s|\\\${LIMIT_IPS_ARG}|${LIMIT_IPS_ARG}|g" -e "s|\\\${STARTTLS_PORTS_ARG}|${STARTTLS_PORTS_ARG}|g" -e "s|\\\${SCANNER_CPU_REQUEST:-500m}|${SCANNER_CPU_REQUEST}|g" -e "s|\\\${SCANNER_CPU_LIMIT:-4}|${SCANNER_CPU_LIMIT}|g" -e "s|\\\${SCANNER_MEM_REQUEST:-4Gi}|${SCANNER_MEM_REQUEST}|g" -e "s|\\\${SCANNER_MEM_LIMIT:-4Gi}|${SCANNER_MEM_LIMIT}|g" -e "s|\\\${SCANNER_PARALLEL:-4}|${SCANNER_PARALLEL}|g" -e "s|\\\${ARTIFACT_WAIT:-300}|${ARTIFACT_WAIT}|g" "$JOB_TEMPLATE" | oc apply -f -
Relevance

●● Moderate

Brittleness is real, but refactoring sed/template placeholders may be seen as out-of-scope.

PR-#6
PR-#55

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The sed patterns in deploy.sh include the default portion in the match string, and the template
embeds the same default literals; the replacement therefore depends on these literals staying
identical across files/templates.

deploy.sh[223-226]
scanner-job.yaml.template[45-51]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `sed` search strings hardcode the template’s default literals (like `:-500m`). This couples `deploy.sh` and the template tightly and makes future edits/error-prone.

## Issue Context
The checked-in template currently matches, so it works today, but the coupling is fragile (especially since `JOB_TEMPLATE` is configurable).

## Fix Focus Areas
- deploy.sh[223-226]
- scanner-job.yaml.template[45-51]

## Suggested fix
- Change the template placeholders to stable tokens without embedded defaults (e.g. `${SCANNER_CPU_REQUEST}`, `${SCANNER_CPU_LIMIT}`, etc.).
- Keep all defaulting logic exclusively in `deploy.sh` (already present).
- Update `sed` patterns to match the stable tokens (e.g. `s|\\\${SCANNER_CPU_REQUEST}|...|g`).
This removes reliance on template default literals and reduces future drift risk.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

  • Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

…ly to catch invalid env var values before deployment.
@openshift-ci

openshift-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

@richardsonnick: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions 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.

@smith-xyz

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2026
@openshift-merge-bot
openshift-merge-bot Bot merged commit fdf7f7a into openshift:main Jul 30, 2026
4 checks passed
redhat-chai-bot added a commit to redhat-chai-bot/openshift_release that referenced this pull request Aug 4, 2026
Align the step-registry env vars with the upstream tls-scanner change
(openshift/tls-scanner#87) that splits SCANNER_CPU / SCANNER_MEMORY
into separate request and limit variables.

New variables (both run and hypershift-run refs):
  SCANNER_CPU_REQUEST  / SCANNER_CPU_LIMIT   (default 4 / 4)
  SCANNER_MEM_REQUEST  / SCANNER_MEM_LIMIT   (default 4Gi / 4Gi)

New guest-cluster variables (hypershift-run ref only):
  SCANNER_CPU_GUEST_REQUEST  / SCANNER_CPU_GUEST_LIMIT   (default 1 / 1)
  SCANNER_MEM_GUEST_REQUEST  / SCANNER_MEM_GUEST_LIMIT   (default 2Gi / 2Gi)

The original SCANNER_CPU, SCANNER_MEMORY, SCANNER_CPU_GUEST, and
SCANNER_MEMORY_GUEST are preserved as deprecated fallbacks so that
existing CI configs continue to work without changes.

This unblocks openshift#82553 (add tls-scanner to
cluster-monitoring-operator e2e) which uses the new split vars to set
a lower CPU request for scheduling while keeping a higher burst limit.
redhat-chai-bot added a commit to redhat-chai-bot/openshift_release that referenced this pull request Aug 4, 2026
Align the step-registry env vars with the upstream tls-scanner change
(openshift/tls-scanner#87) that splits SCANNER_CPU / SCANNER_MEMORY
into separate request and limit variables.

New variables (both run and hypershift-run refs):
  SCANNER_CPU_REQUEST  / SCANNER_CPU_LIMIT   (default 4 / 4)
  SCANNER_MEM_REQUEST  / SCANNER_MEM_LIMIT   (default 4Gi / 4Gi)

New guest-cluster variables (hypershift-run ref only):
  SCANNER_CPU_GUEST_REQUEST  / SCANNER_CPU_GUEST_LIMIT   (default 1 / 1)
  SCANNER_MEM_GUEST_REQUEST  / SCANNER_MEM_GUEST_LIMIT   (default 2Gi / 2Gi)

The original SCANNER_CPU, SCANNER_MEMORY, SCANNER_CPU_GUEST, and
SCANNER_MEMORY_GUEST are preserved as deprecated fallbacks so that
existing CI configs continue to work without changes.

This unblocks openshift#82553 (add tls-scanner to
cluster-monitoring-operator e2e) which uses the new split vars to set
a lower CPU request for scheduling while keeping a higher burst limit.
redhat-chai-bot added a commit to redhat-chai-bot/openshift_release that referenced this pull request Aug 6, 2026
Align the step-registry env vars with the upstream tls-scanner change
(openshift/tls-scanner#87) that splits SCANNER_CPU / SCANNER_MEMORY
into separate request and limit variables.

New variables (both run and hypershift-run refs):
  SCANNER_CPU_REQUEST  / SCANNER_CPU_LIMIT   (default 4 / 4)
  SCANNER_MEM_REQUEST  / SCANNER_MEM_LIMIT   (default 4Gi / 4Gi)

New guest-cluster variables (hypershift-run ref only):
  SCANNER_CPU_GUEST_REQUEST  / SCANNER_CPU_GUEST_LIMIT   (default 1 / 1)
  SCANNER_MEM_GUEST_REQUEST  / SCANNER_MEM_GUEST_LIMIT   (default 2Gi / 2Gi)

The original SCANNER_CPU, SCANNER_MEMORY, SCANNER_CPU_GUEST, and
SCANNER_MEMORY_GUEST are preserved as deprecated fallbacks so that
existing CI configs continue to work without changes.

This unblocks openshift#82553 (add tls-scanner to
cluster-monitoring-operator e2e) which uses the new split vars to set
a lower CPU request for scheduling while keeping a higher burst limit.
openshift-merge-bot Bot pushed a commit to openshift/release that referenced this pull request Aug 7, 2026
…82865)

Align the step-registry env vars with the upstream tls-scanner change
(openshift/tls-scanner#87) that splits SCANNER_CPU / SCANNER_MEMORY
into separate request and limit variables.

New variables (both run and hypershift-run refs):
  SCANNER_CPU_REQUEST  / SCANNER_CPU_LIMIT   (default 4 / 4)
  SCANNER_MEM_REQUEST  / SCANNER_MEM_LIMIT   (default 4Gi / 4Gi)

New guest-cluster variables (hypershift-run ref only):
  SCANNER_CPU_GUEST_REQUEST  / SCANNER_CPU_GUEST_LIMIT   (default 1 / 1)
  SCANNER_MEM_GUEST_REQUEST  / SCANNER_MEM_GUEST_LIMIT   (default 2Gi / 2Gi)

The original SCANNER_CPU, SCANNER_MEMORY, SCANNER_CPU_GUEST, and
SCANNER_MEMORY_GUEST are preserved as deprecated fallbacks so that
existing CI configs continue to work without changes.

This unblocks #82553 (add tls-scanner to
cluster-monitoring-operator e2e) which uses the new split vars to set
a lower CPU request for scheduling while keeping a higher burst limit.

Co-authored-by: Chai Bot <ship-help-github@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants