Skip to content

Gate nodesets on their deployOnAllNodeSets service jobs - #2139

Merged
openshift-merge-bot[bot] merged 1 commit into
openstack-k8s-operators:mainfrom
rabi:OSPRH-37768
Oct 8, 2026
Merged

openshift-merge-bot[bot] merged 1 commit into
openstack-k8s-operators:mainfrom
rabi:OSPRH-37768

Conversation

@rabi

@rabi rabi commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

A deployOnAllNodeSets service runs as a single AnsibleEE Job for the whole deployment.

DedupeServices kept such a service in the first nodeSet's service list only, so every later nodeSet lost its ordering relationship to it: dependsOn resolves against the nodeSet's own list and silently skipped the missing entry. Those nodeSets advanced past the shared Job while it was still mutating their hosts.

Keep the service in every listing nodeSet's plan, where it acts as a barrier. The deployment starts its Job before the nodeSet loop, once every service ordered before it in each listing nodeSet has completed, and a nodeSet blocks until that Job succeeded. Readiness is read from the Job rather than from conditions, which are rebuilt every reconcile, so a restart mid-deployment resumes from cluster state. A nodeSet that does not list the service neither waits for it nor delays it.

Applies to both serial and parallel execution.

Jira: OSPRH-37768

A deployOnAllNodeSets service runs as a single AnsibleEE Job for the whole
deployment.

DedupeServices kept such a service in the first nodeSet's service list
only, so every later nodeSet lost its ordering relationship to it:
dependsOn resolves against the nodeSet's own list and silently skipped the
missing entry. Those nodeSets advanced past the shared Job while it was
still mutating their hosts.

Keep the service in every listing nodeSet's plan, where it acts as a
barrier. The deployment starts its Job before the nodeSet loop, once every
service ordered before it in each listing nodeSet has completed, and a
nodeSet blocks until that Job succeeded. Readiness is read from the Job
rather than from conditions, which are rebuilt every reconcile, so a
restart mid-deployment resumes from cluster state. A nodeSet that does not
list the service neither waits for it nor delays it.

Applies to both serial and parallel execution.

Related: OSPRH-37768
Signed-off-by: Rabi Mishra <ramishra@redhat.com>
@openshift-ci
openshift-ci Bot requested review from dprince and fultonj October 5, 2026 03:37
@openshift-ci openshift-ci Bot added the approved label Oct 5, 2026
@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

OpenStackControlPlane CRD Size Report

Metric Value
CRD JSON size 338593 bytes (331KB)
Base branch size 338593 bytes
Change +0.00%
Status yellow — growing
Threshold reference
Color Range Meaning
🟢 green < 300KB Comfortable
🟡 yellow 300–400KB Growing
🟠 orange 400–750KB Concerning
🔴 red > 750KB Approaching 1.5MB etcd limit (cut in half to allow space for update)

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Central YAML (base), Organization UI (inherited)
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 2bed4363-c492-4d39-bd19-d25573817d4e
📥 Commits

Reviewing files that changed from the base of the PR and between f3a5bdb and 9d6a910.

📒 Files selected for processing (8)
  • docs/assemblies/proc_creating-a-custom-service.adoc
  • internal/controller/dataplane/openstackdataplanedeployment_controller.go
  • internal/dataplane/deployment.go
  • internal/dataplane/global.go
  • internal/dataplane/service.go
  • internal/dataplane/util/ansible_execution.go
  • test/functional/dataplane/base_test.go
  • test/functional/dataplane/openstackdataplanedeployment_controller_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Summary

Summary by CodeRabbit

  • New Features
    • Services configured to run across all node sets now use a shared deployment job.
    • Service ordering is coordinated across node sets: preceding services must finish before the shared job starts, and applicable node sets wait for it before continuing. Node sets that don’t include the service can proceed independently.
  • Bug Fixes
    • Deployments now stop retrying when a job exceeds its retry limit, and service readiness-check failures are reported rather than silently ignored.

Walkthrough

The controller now runs each DeployOnAllNodeSets service as a deployment-scoped job. NodeSets that list the service wait for earlier services before it and for its completion before later services. NodeSets that omit it do not wait for or delay the job.

Changes

Global service barriers

Layer / File(s) Summary
Plan services and check global barriers
internal/dataplane/service.go, internal/dataplane/util/ansible_execution.go, internal/dataplane/global.go, docs/assemblies/proc_creating-a-custom-service.adoc
ServicePlan records global services and the NodeSets that list them. Global job checks use deployment-scoped labels and check whether preceding services have completed. The documentation describes the ordering behavior.
Wait for and start global services
internal/dataplane/deployment.go
The deployer skips global services in per-NodeSet service levels. It checks the shared job before advancing and can start the deployment-scoped service.
Orchestrate barriers across NodeSets
internal/controller/dataplane/openstackdataplanedeployment_controller.go, test/functional/dataplane/base_test.go, test/functional/dataplane/openstackdataplanedeployment_controller_test.go
The controller prepares service levels and deployers, starts global services when their barriers open, and reports when a NodeSet is waiting. Functional tests cover cases where both or only one NodeSet lists the service.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Reconciler
  participant ServicePlan
  participant GlobalServiceGate
  participant NodeSetDeployer
  participant GlobalServiceJob
  Reconciler->>ServicePlan: Build service plan and service levels
  Reconciler->>GlobalServiceGate: Check global service barrier
  GlobalServiceGate-->>Reconciler: Return whether preceding jobs completed
  Reconciler->>GlobalServiceJob: Start global service when barrier opens
  Reconciler->>NodeSetDeployer: Deploy NodeSet services
  NodeSetDeployer->>GlobalServiceJob: Check global service completion
  GlobalServiceJob-->>NodeSetDeployer: Return job status
  NodeSetDeployer-->>Reconciler: Return readiness or barrier wait
Loading

Merge Risk: ⚪ Minimal · up to 9d6a9

No actionable merge-blocking issue remains from the reviewed change. The reported Job backoff-limit panic does not occur on the controller’s lookup path.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 7 files. (1 skipped: 1 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes gating node sets on shared deployOnAllNodeSets service jobs, which is the main change.
Description check ✅ Passed The description directly explains the service barrier behavior, node-set ordering, restart handling, and serial and parallel execution changes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@centosinfra-prod-github-app

Copy link
Copy Markdown

@abays

abays commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Could we account for AnsibleEE Jobs expiring when preserveJobs: false? Those Jobs can be garbage-collected after their TTL. serviceJobCompleted treats a missing predecessor Job as incomplete, so if one nodeset's predecessor expires while another listing nodeset is still working through earlier services, this gate may stay closed indefinitely. Similarly, if the shared Job completes but the controller is unavailable until it expires, globalServiceCompleted will keep waiting for a Job that is gone.

Would it make sense to persist completion state somewhere durable, or retain these Jobs until the relevant barriers have passed? I may be missing a cleanup detail that already covers this.

@rabi

rabi commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Could we account for AnsibleEE Jobs expiring when preserveJobs: false? Those Jobs can be garbage-collected after their TTL. serviceJobCompleted treats a missing predecessor Job as incomplete, so if one nodeset's predecessor expires while another listing nodeset is still working through earlier services, this gate may stay closed indefinitely. Similarly, if the shared Job completes but the controller is unavailable until it expires, globalServiceCompleted will keep waiting for a Job that is gone.

Would it make sense to persist completion state somewhere durable, or retain these Jobs until the relevant barriers have passed? I may be missing a cleanup detail that already covers this.

Yeah, that's an existing issue not related to this.. There is a separate PR for it #2159

@rabi

rabi commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Yeah, that's an existing issue not related to this.. There is a separate PR for it #2159

Normally we don't see preserveJobs: false being used, but I've fixed it.

@abays abays left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci Bot added the lgtm label Oct 8, 2026
@openshift-ci

openshift-ci Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abays, rabi

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-merge-bot
openshift-merge-bot Bot merged commit 455746f into openstack-k8s-operators:main Oct 8, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants