Skip to content

chore(t8s-cluster): migrate config, ignore-volume-az is not enough#1679

Merged
cwrau merged 1 commit intomainfrom
chore/t8s-cluster/migrate-pv-zone-config
Sep 10, 2025
Merged

chore(t8s-cluster): migrate config, ignore-volume-az is not enough#1679
cwrau merged 1 commit intomainfrom
chore/t8s-cluster/migrate-pv-zone-config

Conversation

@cwrau
Copy link
Member

@cwrau cwrau commented Sep 10, 2025

Otherwise the PVs get a nodeSelector

Summary by CodeRabbit

  • Chores

    • Updated storage controller plugin configuration to disable topology awareness for volumes.
  • Impact

    • Volume provisioning no longer considers topology hints, leading to more uniform behavior across nodes and zones.
    • Improves compatibility on clusters lacking topology labels or mixed-zone setups.
    • If you relied on topology-aware scheduling for storage placement, review your workload and storage class configurations.

Copilot AI review requested due to automatic review settings September 10, 2025 09:05
@cwrau cwrau enabled auto-merge September 10, 2025 09:05
@coderabbitai
Copy link

coderabbitai bot commented Sep 10, 2025

Walkthrough

Introduces an extraArgs block to the cinder CSI controller plugin in the Helm chart, adding the flag --with-topology=false. No other structural changes to controller or node plugin templates or tolerations.

Changes

Cohort / File(s) Summary of Changes
Cinder CSI Controller Args
charts/t8s-cluster/templates/workload-cluster/cinder-csi-plugin/cinder-csi-plugin.yaml
Added extraArgs under controllerPlugin with --with-topology=false; no other modifications.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The current title is vague and does not clearly describe the main change, which is adding the --with-topology=false argument to the Cinder CSI plugin configuration; it refers generically to “migrate config” and “ignore-volume-az” without specifying the plugin or the purpose of the change, so a teammate scanning history would not immediately understand the key update. Rename the title to explicitly summarize the primary change, for example: “chore(t8s-cluster): add --with-topology=false to cinder-csi-plugin to prevent PV nodeSelector.”
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

Poem

I nudge a flag with whiskered care,
A tiny tweak in YAML air.
No nodes disturbed, no charts awry—
Just topology told “don’t apply.”
Thump-thump goes CI’s steady drum,
Small hop made, deployments hum. 🐇✨

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8532f2f and 6f0a3fc.

📒 Files selected for processing (1)
  • charts/t8s-cluster/templates/workload-cluster/cinder-csi-plugin/cinder-csi-plugin.yaml (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: lint helm chart (t8s-cluster)
🔇 Additional comments (1)
charts/t8s-cluster/templates/workload-cluster/cinder-csi-plugin/cinder-csi-plugin.yaml (1)

55-56: Ensure --with-topology=false is applied via csi.plugin.extraArgs and on the node plugin
Upstream openstack-cinder-csi chart v2.32.0 doesn’t expose csi.plugin.extraArgs or csi.plugin.nodePlugin.extraArgs in its default values, so the existing controllerPlugin.extraArgs block may be ignored. Move --with-topology=false under csi.plugin.extraArgs and replicate it in csi.plugin.nodePlugin.extraArgs; confirm your chart version supports these keys to avoid unknown-flag failures.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/t8s-cluster/migrate-pv-zone-config

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.

❤️ Share

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

Copy link

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 adds a configuration to disable topology-aware scheduling for the OpenStack Cinder CSI controller plugin. The change prevents persistent volumes from getting node selectors that could interfere with proper volume scheduling.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@cwrau cwrau added this pull request to the merge queue Sep 10, 2025
Merged via the queue into main with commit 55b14c6 Sep 10, 2025
32 of 35 checks passed
@cwrau cwrau deleted the chore/t8s-cluster/migrate-pv-zone-config branch September 10, 2025 09:08
github-merge-queue bot pushed a commit that referenced this pull request Sep 26, 2025
🤖 I have created a release *beep* *boop*
---


##
[9.4.0](t8s-cluster-v9.3.1...t8s-cluster-v9.4.0)
(2025-09-26)


### Features

* **t8s-cluster/auth:** add staff authentication
([#1703](#1703))
([0fbd753](0fbd753))
* **t8s-cluster:** implement autoscaling
([#1686](#1686))
([b5de270](b5de270))


### Miscellaneous Chores

* **t8s-cluster:** migrate config, ignore-volume-az is not enough
([#1679](#1679))
([55b14c6](55b14c6))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
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.

4 participants