WINC-991: Windows CSI node plugin support - #2072
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe enhancement proposal defines operator-managed Windows CSI node plugins for Azure File and VMware vSphere. It covers DaemonSets, CSI Proxy integration, image builds, testing, lifecycle behavior, failure reporting, and support procedures. ChangesWindows CSI support
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant csi-operator
participant WindowsDaemonSet
participant CSIProxy
participant CSIDriver
csi-operator->>WindowsDaemonSet: Generate and manage Windows node plugin
WindowsDaemonSet->>CSIProxy: Use Windows node storage interface
CSIProxy->>CSIDriver: Provide node operations for Azure File or VMware vSphere
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
mansikulkarni96
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
enhancements/windows-containers/windows-CSI-sriver-support.md (1)
131-133: 🩺 Stability & Availability | 🔵 TrivialSpecify the driver and CSI Proxy compatibility matrix.
Stable CSI Proxy APIs do not define the exact proxy binary, supported API groups, security baseline, or upgrade ordering. Record minimum supported CSI Proxy versions for Azure File and vSphere. Test proxy and driver version skew before GA.
🤖 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 `@enhancements/windows-containers/windows-CSI-sriver-support.md` around lines 131 - 133, Expand the CSI proxy compatibility guidance in the Windows CSI driver support documentation to define a driver-to-proxy compatibility matrix, including minimum supported CSI Proxy versions for Azure File and vSphere, supported API groups, security baseline, and upgrade ordering. Add a requirement to test proxy/driver version skew before GA, replacing the current general statement about stable APIs and WMCO-managed upgrades.
🤖 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 `@enhancements/windows-containers/windows-CSI-sriver-support.md`:
- Around line 12-13: Replace the placeholder WINC-TBD value in the tracking-link
configuration with the actual traceable enhancement or implementation issue key
before merging.
- Around line 247-257: Add a direct GA graduation-criteria section in the
proposal, separate from the existing Tech Preview -> GA criteria. Define
measurable acceptance requirements covering supported Azure File and VMware
vSphere drivers, Windows build compatibility, image provenance, end-to-end test
coverage, upgrade behavior, failure reporting, and support documentation.
- Around line 261-276: Expand the “Upgrade / Downgrade Strategy” section to
define a safe migration for legacy manually deployed Windows CSI DaemonSets,
including their resource names, ownership or adoption behavior, conflict
detection, rollout/removal order, and rollback procedure. Ensure the workflow
prevents simultaneous node plugins from sharing CSI registration or socket
paths, rather than relying only on administrators deleting resources before
upgrade.
- Around line 234-245: Expand the GA E2E plan to define separate coverage for
both initial drivers, Azure File and vSphere, rather than only referencing the
Azure WMCO helper. For each driver, verify operator-owned Windows DaemonSets,
pod readiness on Windows nodes, CSINode registration, PVC
creation/mount/write/persistence across pod restarts, and deletion cleanup,
while explicitly removing manual DaemonSet deployment from the tests.
- Around line 285-289: Update the “Downgrades” guidance to explicitly block
unsupported CSI driver operator downgrades rather than allowing Windows
DaemonSets to be silently removed. If downgrade support is intended, document
retaining the existing Windows node service until workloads are drained and the
replacement is healthy.
- Around line 128-130: Update the multi-arch manifest risk description near the
Windows DaemonSet image behavior to state that a manifest without a Windows
variant causes the Windows runtime to report an image-pull/no-matching-manifest
failure, rather than pulling a Linux image. Describe the mitigation as
validating that the released or mirrored image reference includes a
Windows-compatible descriptor.
- Around line 303-307: Expand the “Failure Modes” section to distinguish
clusters with no eligible Windows nodes from clusters where expected Windows CSI
driver pods fail: define the non-degraded ClusterOperator conditions and metrics
for zero desired pods, the degraded conditions and metrics for failures on
eligible nodes, and add tests covering both cases.
- Around line 224-230: Replace the proposed negative-field and API-schema-only
acceptance criteria for the csi-operator generator tests with explicit
assertions on each generated Windows DaemonSet. Verify scheduling,
Windows-compatible paths, image variant resolution, hostProcess security,
tolerations, named-pipe configuration, C:\csi and C:\registration paths, and CSI
registration values for every Windows-enabled driver.
- Around line 85-89: Update the Windows DaemonSet documentation to use valid
YAML escaping for the SYSTEM account, such as single quotes or a doubled
backslash. Clarify that windowsOptions.hostProcess applies to the entire pod,
and explicitly document the HostProcess and hostNetwork requirements for the CSI
driver, node-driver-registrar, and liveness-probe containers, including why each
requirement is needed.
- Around line 290-294: Revise the “Version Skew Strategy” section to remove the
claim that Linux and Windows CSI DaemonSets update atomically or cannot
experience version skew. Document the separate DaemonSet rollout behavior,
including the Operator’s rollout order and health gates, or specify expected
temporary skew and recovery steps for partial failures.
- Around line 213-218: Update the Windows image contract documentation around
the Buildah multi-stage build and manifest list to identify the supported
WMCO/node Windows builds and pin them to the promoted Windows image variant,
rather than implying nanoserver:ltsc2022 supports every node. Add test coverage
that validates the complete promoted multi-architecture manifest list, including
the Windows variants and their OS/build compatibility.
- Around line 134-137: Update the Windows DaemonSet guidance around the direct
Azure credentials Secret mount to require security mitigations before treating
it as preferred, including node-level BitLocker and constrained Secret file
ACLs/path verification for the HostProcess/System driver container, or require a
Windows credentials injector instead. Apply the same requirement to the
duplicate direct-mount preference in the Alternatives section.
- Around line 61-69: Expand the “Non-Goals” or Windows storage support
documentation with an Azure File/vSphere support matrix covering volume mode,
access mode, NFS versus SMB, raw block mapping, and filesystem expansion.
Clearly identify unsupported Windows StorageClass capabilities and preserve the
distinction between supported existing classes and unsupported feature
combinations.
---
Nitpick comments:
In `@enhancements/windows-containers/windows-CSI-sriver-support.md`:
- Around line 131-133: Expand the CSI proxy compatibility guidance in the
Windows CSI driver support documentation to define a driver-to-proxy
compatibility matrix, including minimum supported CSI Proxy versions for Azure
File and vSphere, supported API groups, security baseline, and upgrade ordering.
Add a requirement to test proxy/driver version skew before GA, replacing the
current general statement about stable APIs and WMCO-managed upgrades.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7538ef3f-b90b-49b6-9cc4-f99a36886bdd
📒 Files selected for processing (1)
enhancements/windows-containers/windows-CSI-sriver-support.md
| * If multi-arch manifest lists are not ready for the release payload, the Windows DaemonSets would pull Linux images | ||
| that cannot run on Windows nodes. This is mitigated by coordinating with the Konflux team to ensure multi-arch | ||
| builds are integrated before the feature ships. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file section =="
sed -n '110,140p' enhancements/windows-containers/windows-CSI-sriver-support.md 2>/dev/null || true
echo "== docker/moby docs snippets if available locally =="
python3 - <<'PY'
import subprocess, textwrap
target="C:\\Program Files\\docker\\docker-daemon.json"
print("target_exists_path=", target)
PY
echo "== search for Windows CSI/Sriver related content =="
rg -n "Windows Container|manifest list|image-pull|no image|Linux images|multi-arch|CSI|Sriver|sriver" enhancements/windows-containers/windows-CSI-sriver-support.md .github README.md 2>/dev/null | head -80 || trueRepository: openshift/enhancements
Length of output: 13403
🌐 Web query:
Microsoft Docker Windows containers run your first container image manifest list Windows node image pull error
💡 Result:
When you encounter an error stating no match for platform in manifest or no matching manifest for [OS/arch] in the manifest list entries while pulling a Windows container image, it indicates that the Docker daemon cannot find an image version in the remote repository that is compatible with your host's operating system version or container mode [1][2][3]. To resolve this issue, consider the following troubleshooting steps: 1. Check your Container Mode: Ensure your Docker daemon is correctly set to Windows container mode [4][5][3]. If you are running in Linux container mode, you cannot pull or run Windows-based images [5][3]. You can verify this by checking the Docker Desktop system tray icon or by running docker info and reviewing the Operating System and Architecture fields [1][4]. 2. Verify OS Compatibility: Windows containers require a high degree of compatibility between the host OS build and the container image base OS version [1][6][2]. If you are using a newer image (such as Windows Server 2022) on an older host (such as Windows 10 or Windows Server 2019), the pull will fail because the manifest does not contain a platform entry that matches your specific host build [1][2]. Ensure you are pulling a tag that matches your host environment (e.g., ltsc2019, ltsc2022) [1][5]. 3. Inspect the Manifest: You can inspect the available platforms for a specific image by running the following command [7][8]: docker manifest inspect : Review the output to see if there is a matching entry for your host's OS and architecture under the platform section [7][8]. 4. Avoid Incompatible Snapshotters: If you are using containerd or specialized runtimes, ensure you are not using incompatible storage snapshotters, such as the Stargz snapshotter, which is not supported for Windows containers and can trigger misleading manifest errors [1]. 5. Use Platform-Specific Tags: Rather than pulling the latest tag, which may default to a platform version incompatible with your host, explicitly pull an image version known to be compatible with your Windows build [1][5]. If the error persists, verify your host's version using winver or systeminfo and ensure it meets the requirements for the specific base image you are attempting to pull [1][6][2].
Citations:
- 1: pull image from PS command on Docker Desktop Window 10Ent microsoft/Windows-Containers#452
- 2: Improve the error message when pulling images on incompatible Windows OS version containerd/containerd#6989
- 3: https://devopsaitoolkit.com/blog/docker-error-image-operating-system-cannot-be-used-on-this-platform/
- 4: https://stackoverflow.com/questions/57143386/not-able-to-pull-windows-container-images-on-docker-on-windows-10
- 5: https://stackoverflow.com/questions/48066994/docker-no-matching-manifest-for-windows-amd64-in-the-manifest-list-entries
- 6: Cannot pull containers from mcr.microsoft.com due to platform matching containerd/containerd#7439
- 7: https://learn.microsoft.com/en-us/azure/container-registry/push-multi-architecture-images
- 8: https://github.com/MicrosoftDocs/azure-management-docs/blob/main/articles/container-registry/push-multi-architecture-images.md
Describe the Windows image-pull behavior correctly.
If the multi-arch manifest list has no Windows variant, the Windows runtime reports an image-pull/no-matching-manifest failure; it does not silently select the Linux image. Update this risk to say validate the released/mirrored image reference for a Windows-compatible descriptor.
🤖 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 `@enhancements/windows-containers/windows-CSI-sriver-support.md` around lines
128 - 130, Update the multi-arch manifest risk description near the Windows
DaemonSet image behavior to state that a manifest without a Windows variant
causes the Windows runtime to report an image-pull/no-matching-manifest failure,
rather than pulling a Linux image. Describe the mitigation as validating that
the released or mirrored image reference includes a Windows-compatible
descriptor.
Source: MCP tools
| The manual Windows CSI DaemonSet deployment workflow described in the [csi-proxy enhancement](csi-proxy.md) will be | ||
| superseded by this enhancement. Users who have manually deployed Windows CSI DaemonSets will need to remove them | ||
| before the operator-managed DaemonSets are deployed, to avoid conflicts. Documentation should guide users through | ||
| this transition. | ||
|
|
||
| ### Upgrade / Downgrade Strategy | ||
|
|
||
| #### Cluster upgrades | ||
|
|
||
| When upgrading to a version that includes operator-managed Windows CSI DaemonSets, the DaemonSets will be created | ||
| automatically. If the cluster has Windows nodes, the CSI driver pods will start on those nodes. If the cluster has | ||
| no Windows nodes, the DaemonSets will exist with zero desired pods and have no effect. | ||
|
|
||
| Cluster administrators who have manually deployed Windows CSI DaemonSets (following the workflow from the csi-proxy | ||
| enhancement) should remove them before upgrading, to avoid conflicts with the operator-managed DaemonSets. Release | ||
| notes should clearly document this requirement. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define a safe migration from manually deployed DaemonSets.
Requiring administrators to delete old DaemonSets conflicts with the no-manual-deployment goal. The proposal does not define legacy resource names, ownership, conflict detection, rollout order, or rollback. Two node plugins can race when they share CSI registration or socket paths. Define an explicit adoption/removal workflow, or use unique resources during migration.
🤖 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 `@enhancements/windows-containers/windows-CSI-sriver-support.md` around lines
261 - 276, Expand the “Upgrade / Downgrade Strategy” section to define a safe
migration for legacy manually deployed Windows CSI DaemonSets, including their
resource names, ownership or adoption behavior, conflict detection,
rollout/removal order, and rollback procedure. Ensure the workflow prevents
simultaneous node plugins from sharing CSI registration or socket paths, rather
than relying only on administrators deleting resources before upgrade.
| ### Version Skew Strategy | ||
|
|
||
| Because the Windows and Linux DaemonSets are managed by the same operator and use images from the same multi-arch | ||
| manifest list, there is no version skew between Linux and Windows CSI components. Both DaemonSets are updated | ||
| atomically by the operator during cluster upgrades. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg 'enhancements/windows-containers/windows-CSI-sriver-support.md|csi-driver-smb|cifs|smb|daemonset|operator' || true
echo
echo "== relevant snippet =="
if [ -f enhancements/windows-containers/windows-CSI-sriver-support.md ]; then
wc -l enhancements/windows-containers/windows-CSI-sriver-support.md
sed -n '250,320p' enhancements/windows-containers/windows-CSI-sriver-support.md | cat -n
fi
echo
echo "== search for atomic/version skew/operator/daemonset mentions =="
rg -n "atomic|version skew|DaemonSet|operator|manifest list|multi-arch|rollout|health gate|partial-failure|recovery" . --glob '!vendor/**' --glob '!node_modules/**' || trueRepository: openshift/enhancements
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact tracked file =="
git ls-files -- 'enhancements/windows-containers/windows-CSI-sriver-support.md' -- 'enhancements/windows-containers/*windows*csi*.md' -- '*win*storage*.md' -- 'enhancements/*windows*' || true
echo
echo "== targeted file availability =="
if [ -f enhancements/windows-containers/windows-CSI-sriver-support.md ]; then
echo "FOUND"
wc -l enhancements/windows-containers/windows-CSI-sriver-support.md
sed -n '260,305p' enhancements/windows-containers/windows-CSI-sriver-support.md | cat -n
else
echo "NOT FOUND"
fi
echo
echo "== targeted daemonset/operator/atomic/version skew mentions in windows enhancements =="
rg -n -C 3 'atomic|version skew|DaemonSet|operator|manifest list|multi-arch|rollout|health gate|temporary|partial-failure|recovery' enhancements/windows-containers || trueRepository: openshift/enhancements
Length of output: 50381
🌐 Web query:
Kubernetes DaemonSet separate resources roll out independently atomic update two DaemonSets multi-arch nodeSelector
💡 Result:
In Kubernetes, you can manage multi-architecture workloads by deploying separate DaemonSets, one for each architecture (e.g., amd64 and arm64), using nodeSelectors or nodeAffinity to target the appropriate nodes [1]. This approach allows you to roll out updates to each architecture independently [2]. Key considerations for this strategy include: 1. Architecture-Specific DaemonSets: You define separate DaemonSet manifests with identical app labels (for organization) but unique names, nodeSelectors, and container images optimized for the target architecture [1]. The DaemonSet controller treats these as distinct objects, meaning updating one does not affect the other [2]. 2. Independent Rollouts: Because each DaemonSet is a separate controller resource, their update strategies (RollingUpdate or OnDelete) are configured and executed independently [3][4]. You can trigger a rollout for the arm64 DaemonSet without impacting the amd64 DaemonSet [2]. 3. Atomic Updates: DaemonSet rolling updates are controlled by the.spec.updateStrategy.rollingUpdate configuration, specifically the maxUnavailable parameter [3][5]. While this ensures "atomic" behavior on a per-node basis (replacing an old pod with a new one according to your limits), the "atomicity" is scoped to the individual DaemonSet object [2]. If you have two DaemonSets, the cluster-wide state will transition as each controller processes its respective updates [2]. 4. Advanced Scheduling: While nodeSelector is sufficient for simple requirements, you may use nodeAffinity to implement more complex scheduling rules (e.g., combining architecture requirements with other node labels like spot instance status or hardware features) [6]. For managing complex multi-arch environments, some organizations also use admission webhooks that automatically inject necessary tolerations or affinity rules based on container image metadata, reducing the need for manual orchestration of multiple DaemonSet objects [7][8].
Citations:
- 1: https://reintech.io/blog/building-multi-architecture-kubernetes-clusters
- 2: https://adhdecode.com/containers-kubernetes/workload-controllers/daemonset-update-strategies-rolling/
- 3: https://kubernetes.io/docs/tasks/manage-daemon/update-daemon-set/
- 4: https://kubernetes.recipes/recipes/deployments/kubernetes-daemonset-update-strategies/
- 5: https://kubernetes.io/docs/reference/kubernetes-api/apps/daemon-set-v1/
- 6: https://codelit.io/blog/kubernetes-daemonset-guide
- 7: https://www.artur-rodrigues.com/tech/2024/02/19/kubernetes-multiarcher.html
- 8: https://github.com/msvticket/kube-arch-webhook
Do not treat Linux and Windows CSI DaemonSets as atomic upgrades.
Kubernetes treats each DaemonSet as a separate controller resource, so the Linux and Windows nodes can be at different pod versions during rollout. Either define rollout order and health gates using the Operator’s upgrade model, or document expected temporary version skew and partial-failure recovery.
🤖 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 `@enhancements/windows-containers/windows-CSI-sriver-support.md` around lines
290 - 294, Revise the “Version Skew Strategy” section to remove the claim that
Linux and Windows CSI DaemonSets update atomically or cannot experience version
skew. Document the separate DaemonSet rollout behavior, including the Operator’s
rollout order and health gates, or specify expected temporary skew and recovery
steps for partial failures.
Source: MCP tools
a3fb046 to
c01b1e5
Compare
This enhancement proposes adding Windows node DaemonSets to CSI driver operators (csi-operator, vmware-vsphere-csi-driver-operator), enabling persistent storage for Windows workloads through standard PVCs. This supersedes the manual DaemonSet deployment workflow from the csi-proxy enhancement, now that Buildah supports building Windows container images.
c01b1e5 to
8b34711
Compare
|
/cc: @openshift/openshift-team-storage |
jrvaldes
left a comment
jrvaldes
left a comment
There was a problem hiding this comment.
Thanks @mansikulkarni96 for working on this. PTAL the comments?
| * Achieving full feature parity with Linux storage. Windows does not support all volume modes, access modes, and | ||
| volume features available on Linux. See the support matrix below. | ||
| * Modifying CSI driver controllers. Controllers run on the Linux control plane and are OS-agnostic. | ||
| * HyperShift support. Windows nodes are only supported on standalone clusters. |
There was a problem hiding this comment.
no need to mention this. this is a WMCO limitation, to the CSI proposal
| A single multi-OS DaemonSet is not feasible. Windows and Linux pods require fundamentally different security models | ||
| (`windowsOptions.hostProcess` vs `securityContext.privileged`), different filesystem paths (`C:\var\lib\kubelet` vs | ||
| `/var/lib/kubelet`), and different volume mounts (no `/dev`, `/sys/fs`, or `/etc/selinux` on Windows). Two separate | ||
| DaemonSets is the standard upstream pattern for multi-OS CSI deployments. |
There was a problem hiding this comment.
provide links/ref for this
| automatically pulls the variant matching its platform, so the same image reference (e.g. `${DRIVER_IMAGE}`) works | ||
| for both DaemonSets without any special configuration. | ||
|
|
||
| Two CSI drivers are targeted for the initial implementation: |
There was a problem hiding this comment.
The aim is to create user stories for the two driver plugins available.
mansikulkarni96
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@mansikulkarni96: This pull request references WINC-1993 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. DetailsIn response to this:
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. |
|
/cc: @gnufied |
mansikulkarni96
changed the title
|
@mansikulkarni96: This pull request references WINC-991 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 spike to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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. |
3 participants
This enhancement proposes adding Windows node DaemonSets to CSI driver operators (csi-operator, vmware-vsphere-csi-driver-operator), enabling persistent storage for Windows workloads through standard PVCs. This supersedes the manual DaemonSet deployment workflow from the csi-proxy enhancement, now that Buildah supports building Windows container images.
Summary by CodeRabbit