Operator's Windows container support #1079
Conversation
| return nil | ||
| } | ||
|
|
||
| func (w *windows) createStorageClass() error { |
There was a problem hiding this comment.
Can we move this to the storage_class component instead of adding it to this windows component?
There was a problem hiding this comment.
The reason why this was kept here is we can have control over creation of this storage class on detection of windows only, do we still want to move it?
| return getErr | ||
| } | ||
|
|
||
| // pxutil.ApplyStorageClusterSettingsToPodSpec(cluster, &daemonSet.Spec.Template.Spec) |
There was a problem hiding this comment.
We need to add this, otherwise the DS will get created on nodes where pod is not supposed to be deployed or it won't honor the image registry, pull policy, etc from the stroageclass.
There was a problem hiding this comment.
The only reason this is commented right now is because this is overwrting nodeselector from storagecluster, for windows support we have nodeaffinity as os=linux and for this daemonset, we need nodeaffinity as os=windows.
This is planned to be taken in next PR.
| "--csi-address=$(ADDRESS)", | ||
| "--leader-election=true", | ||
| "--leader-election-type=" + leaderElectionType, | ||
| "--http-endpoint=:8080", |
There was a problem hiding this comment.
Is this arg needed only for windows?
| @@ -675,13 +678,13 @@ func getCSIDeploymentSpec( | |||
| deployment.Spec.Template.Spec.Containers, | |||
| v1.Container{ | |||
| Name: csiAttacherContainerName, | |||
| Image: attacherImage, | |||
| Image: "registry.k8s.io/sig-storage/csi-attacher:v4.3.0", | |||
There was a problem hiding this comment.
This image should come from the csi_generator like the other csi images. Let's not hard code it here.
There was a problem hiding this comment.
Hold off on adding the attacher sidecar until I see why we need it. I am not convinced we need volume attachment objects and the volume attacher for windows support.
There was a problem hiding this comment.
We don't need the above sidecar for now.
| @@ -190,6 +190,9 @@ func (g *CSIGenerator) GetCSIConfiguration() *CSIConfiguration { | |||
| cv.IncludeCsiDriverInfo = true | |||
| } | |||
|
|
|||
| cv.IncludeAttacher = true | |||
There was a problem hiding this comment.
Let's make this conditional and not set to true all the time.
nikita-bhatia
force-pushed
the
windows_support
branch
from
3ffd7fc
to
43aefda
Compare
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
nikita-bhatia
force-pushed
the
windows_support
branch
from
da9ed40
to
388b2df
Compare
…eattachments/status
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
deploy/windows/win.yaml
Outdated
| - hostPath: | ||
| path: \\.\pipe\csi-proxy-filesystem-v1 | ||
| type: '' | ||
| name: csi-proxy-fs-pipe-v1 |
There was a problem hiding this comment.
Looks like you copied this code from the SMB CSI Driver:
https://github.com/kubernetes-csi/csi-driver-smb/blob/d18eaf64b2ebd2ec299d1090c4262d02cd0ac161/deploy/csi-smb-node-windows.yaml#L120
Let's remove deadcode we're not using when copying external code
There was a problem hiding this comment.
Same for other volumes below... remove volumes that are not mounted anywhere
|
This PR is stale because it has been in review for 3 days with no activity. |
Changes done :
“nodeSelector:
kubernetes.io/os: linux”
Testing :
Tested on OCP cluster for with mix of linux and windows node
Few changes will be taken in next phase , this is initial PR to get Portworx support on windows node.