feat: TaintToleration and NodeSelector for CSI components csi-attacher/csi-provisioner/csi-resizer/csi-snapshotter#4251
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds Kubernetes CSI-specific settings (taint tolerations and node selector), registers their types, exposes datastore getters, loads them in the driver to configure CSI sidecar deployments, and extends the settings controller with dedicated update handlers and runtime-object collection for CSI components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
semenas
changed the title
semenas
force-pushed
the
add_k_csi_nodeselector
branch
from
b9285a7 to
3cab97e
Compare
The default number of replicas of each CSI component in Longhorn is 3.
@semenas Could you elaborate more on the statement? |
That's right, but if we decide to restrict to run these CSI components pods on "Control Plane" nodes, we have to use setting defaultSettings.systemManagedComponentsNodeSelector. In this case this setting affected other system managed pods like instance manager and deamon sets longhron-csi-plugin, engine-image-ei and so on. But these "other" components have to be run on all nodes where we are planning to mount a volume, e.g. "Control Plane" and "Workers" nodes. And the same situation about setting defaultSettings.taintToleration. |
mantissahz
force-pushed
the
add_k_csi_nodeselector
branch
from
8c51126 to
2c8891d
Compare
derekbit
force-pushed
the
add_k_csi_nodeselector
branch
2 times, most recently
from
59dee4e to
ac467da
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
types/setting.go (2)
64-90: CSI setting names are inconsistent and will not compileYou currently define:
SettingNameCSISidecarComponentTaintTolerationSettingNameSystemManagedCSISidecarComponentsNodeSelectorbut elsewhere in this file and in other packages you use:
SettingNameTaintTolerationKubernetesCSISettingNameSystemManagedComponentsNodeSelectorKubernetesCSIThe latter two are not defined in the
SettingNameconst block, yet they’re referenced in:
SettingNameListsettingDefinitionsvalidateSettingString- controller and app code
This will fail to compile and also creates ambiguity about the actual CR setting names exposed to users.
I recommend standardizing on the
*KubernetesCSIidentifiers and matching the string values to the documentation (e.g.*-kubernetes-csi), removing the unused*CSISidecar*names.- SettingNameTaintToleration = SettingName("taint-toleration") - SettingNameSystemManagedComponentsNodeSelector = SettingName("system-managed-components-node-selector") - SettingNameCSISidecarComponentTaintToleration = SettingName("csi-sidecar-taint-toleration") - SettingNameSystemManagedCSISidecarComponentsNodeSelector = SettingName("system-managed-csi-sidecar-components-node-selector") + SettingNameTaintToleration = SettingName("taint-toleration") + SettingNameSystemManagedComponentsNodeSelector = SettingName("system-managed-components-node-selector") + SettingNameTaintTolerationKubernetesCSI = SettingName("taint-toleration-kubernetes-csi") + SettingNameSystemManagedComponentsNodeSelectorKubernetesCSI = SettingName("system-managed-components-node-selector-kubernetes-csi")With that in place, the existing uses of
SettingNameTaintTolerationKubernetesCSIandSettingNameSystemManagedComponentsNodeSelectorKubernetesCSIin:
SettingNameListsettingDefinitionsvalidateSettingString
become consistent and no further renames are needed.You can then drop the unused
SettingNameCSISidecarComponentTaintToleration/SettingNameSystemManagedCSISidecarComponentsNodeSelectorconstants entirely unless you explicitly need aliases for backward compatibility.Also applies to: 180-208, 333-360, 2490-2568
786-803: Clarify wording for node selector settings and fix truncated descriptionThe behavioral split between generic and CSI‑specific node‑selector/taint settings is good, but the descriptions have a couple of issues:
SettingDefinitionSystemManagedComponentsNodeSelectorhas:
except for .— looks like an incomplete sentence.SettingDefinitionSystemManagedComponentsNodeSelectorKubernetesCSIstarts with “set node selector for all Longhorn components”, which contradicts the subsequent explanation that it only applies to system‑managed Kubernetes CSI components.Suggest tightening the copy for clarity and correctness, e.g.:
@@ - SettingDefinitionSystemManagedComponentsNodeSelector = SettingDefinition{ - DisplayName: "System Managed Components Node Selector", - Description: "If you want to restrict Longhorn components to only run on particular set of nodes, you can set node selector for **all** Longhorn components except for . " + + SettingDefinitionSystemManagedComponentsNodeSelector = SettingDefinition{ + DisplayName: "System Managed Components Node Selector", + Description: "If you want to restrict Longhorn components to only run on a particular set of nodes, you can set a node selector for **all** Longhorn system managed components except for Kubernetes CSI sidecar components. " + @@ - SettingDefinitionSystemManagedComponentsNodeSelectorKubernetesCSI = SettingDefinition{ - DisplayName: "System Managed Components Node Selector for Kubernetes CSI components", - Description: "If you want to restrict Longhorn components to only run on particular set of nodes, you can set node selector for **all** Longhorn components. " + + SettingDefinitionSystemManagedComponentsNodeSelectorKubernetesCSI = SettingDefinition{ + DisplayName: "System Managed Components Node Selector for Kubernetes CSI components", + Description: "If you want to restrict Kubernetes CSI sidecar components to only run on a particular set of nodes, you can set a node selector for system managed Kubernetes CSI components here. " +The rest of each description (the ordered steps and examples) already clearly explains how the generic and CSI‑specific selectors should be combined.
Also applies to: 805-823, 824-841, 843-861
app/driver.go (1)
237-280: Sidecars now ignore global taint/nodeSelector; consider fallback for backward compatibilityThe new logic:
- Fetches and unmarshals
SettingNameTaintTolerationKubernetesCSI/SettingNameSystemManagedComponentsNodeSelectorKubernetesCSI.- Uses those values exclusively for the Attacher/Provisioner/Resizer/Snapshotter deployments.
- Continues to use the global
SettingNameTaintToleration/SettingNameSystemManagedComponentsNodeSelectorfor the plugin DaemonSet and forgetProcArg.This cleanly separates CSI sidecar placement from other components, which is the PR’s intent. However, it also changes behavior for existing clusters:
- Before this change, sidecars always honored the global taint/nodeSelector settings.
- After this change, if the CSI‑specific settings are left at their (empty) defaults, sidecars will have no tolerations/node selectors even when global values are set.
On an upgraded cluster that already relies on global taints/node selectors, this could:
- Make CSI sidecars unschedulable on tainted nodes, or
- Move them to a different subset of nodes than before, until admins explicitly configure the new settings.
Consider adding a fallback to preserve existing behavior, e.g.:
- If
SettingNameTaintTolerationKubernetesCSI.Value == "", default toSettingNameTaintToleration.Value.- If
SettingNameSystemManagedComponentsNodeSelectorKubernetesCSI.Value == "", default toSettingNameSystemManagedComponentsNodeSelector.Value.You can implement this directly here when computing
tolerationsKubernetesCSI/nodeSelectorKubernetesCSI, or inside the corresponding datastore helpers if you prefer a single source of truth.- tolerationSettingKubernetesCSI, err := lhClient.LonghornV1beta2().Settings(namespace).Get(context.TODO(), string(types.SettingNameTaintTolerationKubernetesCSI), metav1.GetOptions{}) + tolerationSettingKubernetesCSI, err := lhClient.LonghornV1beta2().Settings(namespace).Get(context.TODO(), string(types.SettingNameTaintTolerationKubernetesCSI), metav1.GetOptions{}) if err != nil { return err } - tolerationsKubernetesCSI, err := types.UnmarshalTolerations(tolerationSettingKubernetesCSI.Value) + tolerationValueCSI := tolerationSettingKubernetesCSI.Value + if tolerationValueCSI == "" { + // Fallback to global setting for backward compatibility + tolerationValueCSI = tolerationSetting.Value + } + tolerationsKubernetesCSI, err := types.UnmarshalTolerations(tolerationValueCSI) @@ - nodeSelectorSettingKubernetesCSI, err := lhClient.LonghornV1beta2().Settings(namespace).Get(context.TODO(), string(types.SettingNameSystemManagedComponentsNodeSelectorKubernetesCSI), metav1.GetOptions{}) + nodeSelectorSettingKubernetesCSI, err := lhClient.LonghornV1beta2().Settings(namespace).Get(context.TODO(), string(types.SettingNameSystemManagedComponentsNodeSelectorKubernetesCSI), metav1.GetOptions{}) if err != nil { return err } - nodeSelectorKubernetesCSI, err := types.UnmarshalNodeSelector(nodeSelectorSettingKubernetesCSI.Value) + nodeSelectorValueCSI := nodeSelectorSettingKubernetesCSI.Value + if nodeSelectorValueCSI == "" { + // Fallback to global setting for backward compatibility + nodeSelectorValueCSI = nodeSelectorSetting.Value + } + nodeSelectorKubernetesCSI, err := types.UnmarshalNodeSelector(nodeSelectorValueCSI)This keeps existing behavior by default while still allowing admins to override CSI sidecar placement independently.
Also applies to: 340-356, 360-360
🧹 Nitpick comments (5)
types/types.go (1)
248-253: CSI sidecar set definition looks good; minor naming/read‑only nitThe exported set of CSI sidecar names is correct and idiomatic (
map[string]struct{}as a set). To make intent slightly clearer, consider either renaming to something likeKubernetesCSISidecarSetor adding a brief comment that this map is read‑only and used purely for membership checks so callers don’t mutate it.datastore/longhorn.go (1)
4099-4121: CSI-specific setting getters mirror existing pattern and look correctBoth
GetSettingTaintTolerationKubernetesCSIandGetSettingSystemManagedComponentsNodeSelectorKubernetesCSIare consistent with the existing non-CSI getters: they useGetSettingWithAutoFillingROplusUnmarshalTolerations/UnmarshalNodeSelector, so empty values yield empty slice/map and invalid strings surface as errors. This is fine as-is; if more similar getters are added later, you might factor out small helpers to avoid duplication.controller/setting_controller.go (3)
470-533:updateTaintTolerationKubernetesCSIduplicates existing logic but scope is correctThe new handler cleanly reuses the existing toleration update flow while scoping updates to CSI Deployments via
collectRuntimeObjectsKubernetesCSI. Error handling and detached‑volume gating are consistent withupdateTaintToleration.One nit: the function comment still says “deletes all user‑deployed and system‑managed components”; in reality this only targets Kubernetes CSI system‑managed components. Consider tightening the comment to avoid confusion.
-// updateTaintTolerationKubernetesCSI deletes all user-deployed and system-managed components immediately with the updated taint toleration. +// updateTaintTolerationKubernetesCSI updates tolerations for Kubernetes CSI system-managed components.
1068-1128: Minor consistency nit inupdateNodeSelectorerror reportingLogic is unchanged except for the default branch error message, which still references
SettingNameSystemManagedComponentsNodeSelector. Since this function now coexists with the CSI‑specific variant, the message is fine but slightly generic.If you later share
getNotUpdatedNodeSelectorListbetween both settings (as you already do), consider making the error message setting‑agnostic there to avoid confusion.
535-576: Centralize CSI sidecar deploymentapplabel key constant to clarify the label/key contractThe code correctly separates CSI and non-CSI Deployments based on matching
dp.Labels["app"]againstKubernetesCSISidecarListkeys. However, this contract is implicit and distributed across two functions (collectRuntimeObjectsat line 563 andcollectRuntimeObjectsKubernetesCSIat line 586), with the key ("app") hardcoded in both checks.To improve maintainability and make the contract explicit, define the label key as a constant (e.g.,
CSIDeploymentAppLabelKey = "app") intypes/types.goalongsideKubernetesCSISidecarList, then reuse it in both filtering locations. This prevents accidental divergence if the label key ever changes, and clarifies the dependency for future maintainers.Additionally, consider adding a test case that verifies mock CSI Deployments with these specific app labels are correctly included/excluded.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/driver.go(2 hunks)controller/setting_controller.go(6 hunks)datastore/longhorn.go(1 hunks)types/setting.go(6 hunks)types/types.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
datastore/longhorn.go (1)
types/setting.go (2)
UnmarshalTolerations(1995-2012)UnmarshalNodeSelector(2055-2070)
controller/setting_controller.go (3)
types/setting.go (2)
UnmarshalTolerations(1995-2012)UnmarshalNodeSelector(2055-2070)util/util.go (1)
TolerationListToMap(613-621)types/types.go (3)
ErrorInvalidState(341-343)GetBaseLabelsForSystemManagedComponent(460-462)KubernetesCSISidecarList(248-253)
app/driver.go (2)
types/setting.go (2)
UnmarshalTolerations(1995-2012)UnmarshalNodeSelector(2055-2070)csi/deployment.go (4)
NewAttacherDeployment(53-91)NewProvisionerDeployment(109-150)NewResizerDeployment(168-216)NewSnapshotterDeployment(234-272)
⏰ 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: Summary
🔇 Additional comments (4)
controller/setting_controller.go (3)
279-287: Danger‑zone wiring for new CSI settings looks consistentNew Kubernetes CSI settings are added to
dangerSettingsRequiringAllVolumesDetachedand wired to dedicated handlers, mirroring the existing taint/node‑selector behavior (includingAreAllVolumesDetachedStatechecks). This is structurally sound and matches the intent of isolating CSI from generic settings.Also applies to: 288-305
666-686: Including CSI deployments in priority‑class updates is correctAggregating
collectRuntimeObjects()withcollectRuntimeObjectsKubernetesCSI()forupdatePriorityClassis the right fix after excluding CSI from the generic collector. The combined list ensures sidecars still receive PriorityClass updates alongside other components.
1130-1191:updateNodeSelectorKubernetesCSImirrors existing behavior appropriatelyThe CSI‑specific node‑selector update handler correctly:
- Collects only CSI deployments,
- Checks all volumes are detached,
- Updates DaemonSet/Deployment
NodeSelector, and- Deletes Pods when necessary.
The error message in the default branch correctly references
SettingNameSystemManagedComponentsNodeSelectorKubernetesCSI. Overall this is aligned with the non‑CSI variant.types/setting.go (1)
790-803: Validation wiring for new CSI settings is correctFor both CSI‑specific settings:
SettingNameTaintTolerationKubernetesCSI→ validated viaUnmarshalTolerationsSettingNameSystemManagedComponentsNodeSelectorKubernetesCSI→ validated viaUnmarshalNodeSelectorwhich mirrors the existing generic settings. Once the constant naming issue above is fixed, this validation path is appropriate and consistent.
Also applies to: 824-841, 2490-2568
derekbit
force-pushed
the
add_k_csi_nodeselector
branch
from
ac467da to
fc6c871
Compare
semenas
force-pushed
the
add_k_csi_nodeselector
branch
from
d777535 to
2bb85eb
Compare
derekbit
force-pushed
the
add_k_csi_nodeselector
branch
from
2bb85eb to
d834dd4
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
types/setting.go (1)
789-807: SystemManagedComponentsNodeSelector description has a broken sentenceThe
SettingDefinitionSystemManagedComponentsNodeSelector.Descriptionnow tries to explain the split between generic and CSI-sidecar node selectors, but the first sentence contains an incomplete fragment:“... set node selector for all Longhorn components except for .”
This is user-facing and confusing; it should explicitly mention “except for Kubernetes CSI sidecar components” (or similar) and read cleanly as a full sentence.
- Description: "If you want to restrict Longhorn components to only run on particular set of nodes, you can set node selector for **all** Longhorn components except for . " + + Description: "If you want to restrict Longhorn components to only run on a particular set of nodes, you can set node selectors for **all** Longhorn components except for Kubernetes CSI sidecar components. " +Also applies to: 808-826
🧹 Nitpick comments (2)
controller/setting_controller.go (2)
470-533: CSI-sidecar toleration update logic matches existing pattern; minor doc/duplication nitsThe implementation is essentially a scoped clone of
updateTaintToleration, limited tocollectRuntimeObjectsKubernetesCSI(), which is correct for targeting only CSI sidecars.Two small points:
- The comment still says “deletes all user-deployed and system-managed components”, but this function only touches the CSI sidecar deployments.
- The code is now duplicated between
updateTaintTolerationandupdateCSISidecarComponentTaintTolerationexcept for the setting name and collector; a shared helper taking(settingName, collectorFn)would reduce future drift.
1068-1128: Node-selector flows for generic vs CSI components are consistent; minor error-message nitThe new
updateCSISidecarComponentsNodeSelectormirrorsupdateNodeSelectorbut scopes tocollectRuntimeObjectsKubernetesCSI(), which is the right split given the two settings. Volume-detached gating and object handling (DaemonSet/Deployment/Pod) are consistent.Small nit:
getNotUpdatedNodeSelectorListstill usesSettingNameSystemManagedComponentsNodeSelectorin its default-case error message even when invoked from the CSI-specific updater. If that error ever triggers, it will point to the generic setting name regardless of which setting is being processed; not critical, but a bit confusing.Also applies to: 1130-1191
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/driver.go(2 hunks)controller/setting_controller.go(6 hunks)datastore/longhorn.go(1 hunks)types/setting.go(6 hunks)types/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- datastore/longhorn.go
🧰 Additional context used
🧬 Code graph analysis (2)
controller/setting_controller.go (3)
types/setting.go (4)
SettingNameCSISidecarComponentTaintToleration(89-89)SettingNameSystemManagedCSISidecarComponentsNodeSelector(90-90)UnmarshalTolerations(2014-2031)UnmarshalNodeSelector(2074-2089)util/util.go (1)
TolerationListToMap(613-621)types/types.go (3)
ErrorInvalidState(341-343)GetBaseLabelsForSystemManagedComponent(460-462)KubernetesCSISidecarList(248-253)
app/driver.go (1)
types/setting.go (4)
SettingNameCSISidecarComponentTaintToleration(89-89)UnmarshalTolerations(2014-2031)SettingNameSystemManagedCSISidecarComponentsNodeSelector(90-90)UnmarshalNodeSelector(2074-2089)
⏰ 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). (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (7)
controller/setting_controller.go (2)
279-286: Danger-zone wiring for new CSI settings is consistentIncluding
SettingNameCSISidecarComponentTaintTolerationandSettingNameSystemManagedCSISidecarComponentsNodeSelectorin the “all volumes detached” danger-zone list and branching to dedicated update handlers keeps their behavior aligned with the existing taint/node selector settings.Also applies to: 298-305
535-576: <!-- [scratchpad_start]
[task verification_1/1 complete]
[observations]
- csi/deployment_util.go:41 confirms app label set as: deploymentLabels["app"] = commonName
- NewAttacherDeployment passes types.CSIAttacherName to getCommonDeployment
- types/deploy.go values: CSIAttacherName="csi-attacher", CSIProvisionerName="csi-provisioner", CSIResizerName="csi-resizer", CSISnapshotterName="csi-snapshotter"
- types/types.go: CSISidecarPortNameAttacher="csi-attacher", CSISidecarPortNameProvisioner="csi-provisioner", CSISidecarPortNameResizer="csi-resizer", CSISidecarPortNameSnapshotter="csi-snapshotter"
- KubernetesCSISidecarList keys use CSISidecarPortName* constants
- All string values match exactly between CSIName and CSISidecarPortName constants
- setting_controller.go:563 correctly excludes CSI deployments from collectRuntimeObjects()
- setting_controller.go:586 correctly includes CSI deployments in collectRuntimeObjectsKubernetesCSI()
[analysis]
- Verification confirms the CSI sidecar deployments have app labels that exactly match KubernetesCSISidecarList keys
- Filtering logic properly classifies CSI vs non-CSI deployments
- No other deployments would accidentally match the list
- Original review comment's verification request is satisfied
[done]
✓ Confirmed CSI app labels match KubernetesCSISidecarList keys
✓ Verified filtering logic is correct
✓ Confirmed classification is properly localized
✓ No false positive risks identified
</scratchpad_end] -->Runtime object collection split between general and CSI scope is correct
Excluding CSI sidecar deployments from
collectRuntimeObjects()and reintroducing them viacollectRuntimeObjectsKubernetesCSI()gives you:
- Generic updates (taint toleration, generic node selector) that no longer touch CSI sidecars.
- PriorityClass updates that still see all managed objects by appending the CSI-specific slice.
This matches the new settings semantics, and the classification via
types.KubernetesCSISidecarList+"app"label keeps the logic localized. The app labels used by CSI sidecar deployments (csi-attacher, csi-provisioner, csi-resizer, csi-snapshotter) are correctly mapped as keys in the list, and no non-CSI deployments use these values.types/setting.go (3)
89-90: New CSI-sidecar setting names and definitions are wired correctlyThe new
SettingNameCSISidecarComponentTaintTolerationandSettingNameSystemManagedCSISidecarComponentsNodeSelectorare:
- Declared alongside the existing taint/node-selector settings.
- Included in
SettingNameListso they get CRs.- Added to
settingDefinitionswith correspondingSettingDefinition*entries, both in the DangerZone category.This ensures they participate fully in validation, default-setting handling, and danger-zone logic.
Also applies to: 181-209, 335-362
827-865: New CSI-sidecar-specific setting definitions are consistent with existing ones
SettingDefinitionCSISidecarComponentTaintTolerationandSettingDefinitionSystemManagedCSISidecarComponentsNodeSelector:
- Mirror the wording and behavior of the existing taint/node-selector definitions, but scoped to CSI sidecars.
- Are categorized as
SettingCategoryDangerZone, non-required, string-typed settings.- Clearly document example formats and the intended order of configuration steps between user-deployed, CSI sidecar, and other system-managed components.
This is aligned with how the controller uses them and with the split between generic and CSI-only behavior.
2579-2587: Validation for new CSI-sidecar settings correctly reuses existing parsersExtending
validateSettingStringwith:
case SettingNameCSISidecarComponentTaintToleration:→UnmarshalTolerationscase SettingNameSystemManagedCSISidecarComponentsNodeSelector:→UnmarshalNodeSelectorkeeps the syntax and validation rules identical to the existing taint and generic node-selector settings, which is what you want.
types/types.go (1)
248-253: KubernetesCSISidecarList implementation is correct and properly verifiedThe
KubernetesCSISidecarListmap is properly defined and serves its intended purpose. Verification confirms it is used incontroller/setting_controller.go(lines 563 and 586) to filter deployments based on thedp.Labels["app"]field—deployments are either excluded or included only if their app label matches one of these four CSI sidecar constants. The constants are consistently used as container port names in the deployment definitions. The code is correct as written.app/driver.go (1)
259-280: CSI sidecars now use dedicated placement settings; verify upgrade behavior is intentionalVerification confirms the behavioral concern: on upgrade, existing clusters will see CSI sidecar deployments ignore the global taint toleration and node selector settings if the new CSI-specific settings (
csi-sidecar-taint-tolerationandsystem-managed-csi-sidecar-components-node-selector) are not explicitly populated. The upgrade path does not automatically migrate values from global to CSI-specific settings, and no fallback logic exists in the deployment code.This is a breaking change for clusters that previously relied on global settings for CSI sidecar placement. Please confirm:
- Whether this behavioral change is intentional (i.e., CSI sidecars should start with empty placement unless explicitly configured), or
- Whether migration/fallback logic should be added to inherit from global settings when CSI-specific ones are empty, and
- Whether the release notes and upgrade documentation clearly document this change.
Also applies to: 340-355
|
@semenas However, we need to copy the valules of the settings Can you add a commit to handle it? |
derekbit
force-pushed
the
add_k_csi_nodeselector
branch
2 times, most recently
from
d7e9418 to
55e8f97
Compare
|
@semenas Will you update the PR for #4251 (comment)? |
Yes, I'll try to do it today, thank you |
done |
|
This pull request is now in conflict. Could you fix it @semenas? 🙏 |
upgrade/util/util.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // copy the valules of the settings taint-toleration and system-managed-components-node-selector |
There was a problem hiding this comment.
@semenas Can you check the implementation of the upgrade path https://github.com/longhorn/longhorn-manager/blob/master/upgrade/v19xto1100/upgrade.go? We read the CRs from cache instead.
There was a problem hiding this comment.
@derekbit Could you please check if that's the right way?
There was a problem hiding this comment.
@semenas Still have some issues. How about removing the upgrade path implementation commts? I can help implement this part.
controller/setting_controller.go
Outdated
| switch objType := obj.(type) { | ||
| case *appsv1.DaemonSet: | ||
| ds := obj.(*appsv1.DaemonSet) | ||
| sc.logger.Infof("Updating the node selector from %v to %v for %v", ds.Spec.Template.Spec.NodeSelector, newNodeSelector, ds.Name) | ||
| ds.Spec.Template.Spec.NodeSelector = newNodeSelector | ||
| if _, err := sc.ds.UpdateDaemonSet(ds); err != nil { |
There was a problem hiding this comment.
I think casting isn’t needed. The type switch already handles that. https://go.dev/ref/spec#Type_switches
| switch objType := obj.(type) { | |
| case *appsv1.DaemonSet: | |
| ds := obj.(*appsv1.DaemonSet) | |
| sc.logger.Infof("Updating the node selector from %v to %v for %v", ds.Spec.Template.Spec.NodeSelector, newNodeSelector, ds.Name) | |
| ds.Spec.Template.Spec.NodeSelector = newNodeSelector | |
| if _, err := sc.ds.UpdateDaemonSet(ds); err != nil { | |
| switch objTyped := obj.(type) { | |
| case *appsv1.DaemonSet: | |
| sc.logger.Infof("Updating the node selector from %v to %v for %v", objTyped.Spec.Template.Spec.NodeSelector, newNodeSelector, ds.Name) | |
| objTyped.Spec.Template.Spec.NodeSelector = newNodeSelector | |
| if _, err := sc.ds.UpdateDaemonSet(objTyped); err != nil { |
There was a problem hiding this comment.
We should refactor the other parts of the code as well. cc @derekbit
longhorn/longhorn#12316
| SettingNameTaintToleration = SettingName("taint-toleration") | ||
| SettingNameSystemManagedComponentsNodeSelector = SettingName("system-managed-components-node-selector") | ||
| SettingNameCSISidecarComponentTaintToleration = SettingName("csi-sidecar-taint-toleration") | ||
| SettingNameSystemManagedCSISidecarComponentsNodeSelector = SettingName("system-managed-csi-sidecar-components-node-selector") | ||
| SettingNameSystemManagedCSIComponentsResourceLimits = SettingName("system-managed-csi-components-resource-limits") |
There was a problem hiding this comment.
| SettingNameTaintToleration = SettingName("taint-toleration") | |
| SettingNameSystemManagedComponentsNodeSelector = SettingName("system-managed-components-node-selector") | |
| SettingNameCSISidecarComponentTaintToleration = SettingName("csi-sidecar-taint-toleration") | |
| SettingNameSystemManagedCSISidecarComponentsNodeSelector = SettingName("system-managed-csi-sidecar-components-node-selector") | |
| SettingNameSystemManagedCSIComponentsResourceLimits = SettingName("system-managed-csi-components-resource-limits") | |
| SettingNameTaintToleration = SettingName("taint-toleration") | |
| SettingNameCSISidecarComponentTaintToleration = SettingName("csi-sidecar-taint-toleration") | |
| SettingNameSystemManagedComponentsNodeSelector = SettingName("system-managed-components-node-selector") | |
| SettingNameSystemManagedCSISidecarComponentsNodeSelector = SettingName("system-managed-csi-sidecar-components-node-selector") | |
| SettingNameSystemManagedCSIComponentsResourceLimits = SettingName("system-managed-csi-components-resource-limits") |
| SettingNameTaintToleration, | ||
| SettingNameSystemManagedComponentsNodeSelector, | ||
| SettingNameCSISidecarComponentTaintToleration, | ||
| SettingNameSystemManagedCSISidecarComponentsNodeSelector, | ||
| SettingNameSystemManagedCSIComponentsResourceLimits, |
There was a problem hiding this comment.
| SettingNameTaintToleration, | |
| SettingNameSystemManagedComponentsNodeSelector, | |
| SettingNameCSISidecarComponentTaintToleration, | |
| SettingNameSystemManagedCSISidecarComponentsNodeSelector, | |
| SettingNameSystemManagedCSIComponentsResourceLimits, | |
| SettingNameTaintToleration, | |
| SettingNameCSISidecarComponentTaintToleration, | |
| SettingNameSystemManagedComponentsNodeSelector, | |
| SettingNameSystemManagedCSISidecarComponentsNodeSelector, | |
| SettingNameSystemManagedCSIComponentsResourceLimits, |
| SettingNameTaintToleration: SettingDefinitionTaintToleration, | ||
| SettingNameSystemManagedComponentsNodeSelector: SettingDefinitionSystemManagedComponentsNodeSelector, | ||
| SettingNameCSISidecarComponentTaintToleration: SettingDefinitionCSISidecarComponentTaintToleration, | ||
| SettingNameSystemManagedCSISidecarComponentsNodeSelector: SettingDefinitionSystemManagedCSISidecarComponentsNodeSelector, | ||
| SettingNameSystemManagedCSIComponentsResourceLimits: SettingDefinitionSystemManagedCSIComponentsResourceLimits, |
There was a problem hiding this comment.
| SettingNameTaintToleration: SettingDefinitionTaintToleration, | |
| SettingNameSystemManagedComponentsNodeSelector: SettingDefinitionSystemManagedComponentsNodeSelector, | |
| SettingNameCSISidecarComponentTaintToleration: SettingDefinitionCSISidecarComponentTaintToleration, | |
| SettingNameSystemManagedCSISidecarComponentsNodeSelector: SettingDefinitionSystemManagedCSISidecarComponentsNodeSelector, | |
| SettingNameSystemManagedCSIComponentsResourceLimits: SettingDefinitionSystemManagedCSIComponentsResourceLimits, | |
| SettingNameTaintToleration: SettingDefinitionTaintToleration, | |
| SettingNameCSISidecarComponentTaintToleration: SettingDefinitionCSISidecarComponentTaintToleration, | |
| SettingNameSystemManagedComponentsNodeSelector: SettingDefinitionSystemManagedComponentsNodeSelector, | |
| SettingNameSystemManagedCSISidecarComponentsNodeSelector: SettingDefinitionSystemManagedCSISidecarComponentsNodeSelector, | |
| SettingNameSystemManagedCSIComponentsResourceLimits: SettingDefinitionSystemManagedCSIComponentsResourceLimits, |
| "We recommend setting node selector during Longhorn deployment because the Longhorn system cannot be operated during the update. " + | ||
| "Multiple label key-value pairs are separated by semicolon. For example: \n\n" + | ||
| "* `label-key1=label-value1; label-key2=label-value2` \n\n" + | ||
| "Please see the documentation at https://longhorn.io for more detailed instructions about changing node selector", |
There was a problem hiding this comment.
Please see the documentation at https://longhorn.io/ for more detailed instructions about changing node selector
This line can probably be removed
types/types.go
Outdated
| var KubernetesCSISidecarList = map[string]struct{}{ | ||
| CSISidecarPortNameAttacher: {}, | ||
| CSISidecarPortNameProvisioner: {}, | ||
| CSISidecarPortNameResizer: {}, | ||
| CSISidecarPortNameSnapshotter: {}, | ||
| } |
There was a problem hiding this comment.
| var KubernetesCSISidecarList = map[string]struct{}{ | |
| CSISidecarPortNameAttacher: {}, | |
| CSISidecarPortNameProvisioner: {}, | |
| CSISidecarPortNameResizer: {}, | |
| CSISidecarPortNameSnapshotter: {}, | |
| } | |
| func IsKubernetesCSISidecar(appName string) bool { | |
| switch appName { | |
| case CSISidecarPortNameAttacher, | |
| CSISidecarPortNameProvisioner, | |
| CSISidecarPortNameResizer, | |
| CSISidecarPortNameSnapshotter: | |
| return true | |
| default: | |
| return false | |
| } | |
| } |
controller/setting_controller.go
Outdated
| if _, err := sc.ds.UpdateDaemonSet(ds); err != nil { | ||
| return err | ||
| } | ||
| case *appsv1.Deployment: |
There was a problem hiding this comment.
Is there a reason Longhorn needs to handle other object types like Pods or DaemonSets? The CSI sidecar components seem to be deployed only as DaemonSets.
There was a problem hiding this comment.
It looks like Pods and DaemonSets could be removed. These CSI sidecars handled as a Deployment.
|
@semenas Could you consolidate the commits in the PR and address @c3y1huang's comments? Thanks. |
|
Hello @semenas 
|
Signed-off-by: Alexander Semenov <sasforever@gmail.com>
Signed-off-by: Alexander Semenov <67096657+semenas@users.noreply.github.com>
semenas
force-pushed
the
add_k_csi_nodeselector
branch
from
c8ba3ef to
b81868e
Compare
derekbit
left a comment
derekbit
left a comment
There was a problem hiding this comment.
Some questions need to be clarified.
controller/setting_controller.go
Outdated
|
|
||
| detached, err := sc.ds.AreAllVolumesDetachedState() | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to check volume detachment for %v setting update", types.SettingNameCSISidecarComponentTaintToleration) | ||
| } | ||
| if !detached { | ||
| return &types.ErrorInvalidState{Reason: fmt.Sprintf("failed to apply %v setting to Longhorn components when there are attached volumes. It will be eventually applied", types.SettingNameCSISidecarComponentTaintToleration)} | ||
| } |
There was a problem hiding this comment.
Since the taint toleration only applies to CSI sidecar components, requiring all volumes to be detached does not seem necessary. WDYT? @c3y1huang @PhanLe1010 @shuo-wu
There was a problem hiding this comment.
Agree. There is no need to guarantee the detachment of all volumes
controller/setting_controller.go
Outdated
| detached, err := sc.ds.AreAllVolumesDetachedState() | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to check volume detachment for %v setting update", types.SettingNameSystemManagedCSISidecarComponentsNodeSelector) | ||
| } | ||
| if !detached { | ||
| return &types.ErrorInvalidState{Reason: fmt.Sprintf("failed to apply %v setting to Longhorn components when there are attached volumes. It will be eventually applied", types.SettingNameSystemManagedCSISidecarComponentsNodeSelector)} | ||
| } |
types/setting.go
Outdated
| Description: "If you want to restrict Longhorn components to only run on particular set of nodes, you can set node selector for **all** Longhorn components except for . " + | ||
| "Longhorn system contains user deployed components (e.g, Longhorn manager, Longhorn driver, Longhorn UI) and system managed components (e.g, instance manager, engine image, CSI driver, etc.) " + | ||
| "You must follow the below order when set the node selector:\n\n" + | ||
| "1. Set node selector for user deployed components in Helm chart or deployment YAML file depending on how you deployed Longhorn.\n\n" + | ||
| "2. Set node selector for system managed Kubernetes CSI components in system-managed-csi-sidecar-components-node-selector. \n\n" + | ||
| "3. Set node selector for other system managed components in here.\n\n" + | ||
| "All Longhorn volumes should be detached before modifying node selector settings. " + | ||
| "We recommend setting node selector during Longhorn deployment because the Longhorn system cannot be operated during the update. " + | ||
| "Multiple label key-value pairs are separated by semicolon. For example: \n\n" + | ||
| "* `label-key1=label-value1; label-key2=label-value2` \n\n", |
There was a problem hiding this comment.
Should update the description
Description: "This setting configures the node selector **only for system managed Kubernetes CSI sidecar components** (e.g., csi-attacher, csi-provisioner, csi-resizer, csi-snapshotter). " +
"It is used to restrict these CSI sidecar components to run on a specific set of nodes. " +
"Longhorn system contains user deployed components (e.g., Longhorn manager, Longhorn driver, Longhorn UI) and system managed components (e.g., instance manager, engine image, CSI driver, etc.). " +
"You must follow the order below when configuring node selectors:\n\n" +
"1. Configure node selectors for user deployed components via the Helm chart or deployment YAML, depending on how Longhorn was deployed.\n\n" +
"2. Configure node selectors for system managed Kubernetes CSI sidecar components using this setting.\n\n" +
"Multiple label key-value pairs can be specified and separated by semicolons. For example:\n\n" +
"* `label-key1=label-value1; label-key2=label-value2`\n\n",
cc @c3y1huang
types/setting.go
Outdated
|
|
||
| SettingDefinitionCSISidecarComponentTaintToleration = SettingDefinition{ | ||
| DisplayName: "Kubernetes Taint Toleration for Kubernetes CSI sidecar components", | ||
| Description: "If you want to dedicate nodes to just store Longhorn replicas and reject other general workloads, you can set tolerations for **all** Longhorn components and add taints to the nodes dedicated for storage. " + |
There was a problem hiding this comment.
The description is not correct. Should be
Description: "This setting configures taint tolerations **only for system managed Kubernetes CSI sidecar components** (e.g., csi-attacher, csi-provisioner, csi-resizer, csi-snapshotter). " +
"It is intended for environments where nodes are dedicated for Longhorn storage by applying taints. " +
"Multiple tolerations can be specified and separated by semicolons. For example:\n\n" +
"* `key1=value1:NoSchedule; key2:NoExecute:` this toleration tolerates everything because an empty key with operator `Exists` matches all keys, values, and effects.\n\n" +
"* `key1=value1:` this toleration has an empty effect and matches all effects with key `key1`.\n\n" +
"Because `kubernetes.io` is used as the key for Kubernetes default tolerations, it should not be used in the toleration settings.\n\n",
Correct?
cc @c3y1huang
|
@derekbit The naming convention for the settings is a little different. Should we make some changes? csi-sidecar-taint-toleration system-managed-csi-components-resource-limits |
I thought the question before. |
Mostly about these settings: system-managed-csi-sidecar-components-node-selector system-managed-csi-components-resource-limits (It's from the another pr in the master branch) |
shuo-wu
left a comment
shuo-wu
left a comment
There was a problem hiding this comment.
I am OK with this backend implementation in general.
But for the UI, I have a suggestion: Would we add special buttons like Apply the same value of setting "Kubernetes Taint Toleration" for the new settings?
controller/setting_controller.go
Outdated
| types.SettingNameCSISidecarComponentTaintToleration, | ||
| types.SettingNameSystemManagedCSISidecarComponentsNodeSelector, |
There was a problem hiding this comment.
As mentioned by Derek, CSI components restart won't affect the existing running volumes, hence there is no need to put these 2 settings in the danger zone.
There was a problem hiding this comment.
@derekbit I can remove these settings from the danger zone. WDYT?
|
@derekbit I've removed settings from the danger zone and made some fixes around descriptions and so on. |
Signed-off-by: Alexander Semenov <sasforever@gmail.com>
semenas
force-pushed
the
add_k_csi_nodeselector
branch
from
a4ecb19 to
46b32ae
Compare
|
This pull request is now in conflict. Could you fix it @semenas? 🙏 |
semenas
force-pushed
the
add_k_csi_nodeselector
branch
from
46b32ae to
772cff5
Compare
4 participants
Which issue(s) this PR fixes:
Issue # longhorn/longhorn#12079
What this PR does / why we need it:
Additional settings defaultSettings.taintTolerationKubernetesCSI and defaultSettings.systemManagedComponentsNodeSelectorKubernetesCSI provide more flexible options for pod placement of the CSI sidecars components (csi-attacher, csi-provisioner, csi-resizer, csi-snapshotter).
Currently, CSI sidecars components inherit the same node placement rules as all other system-managed components through defaultSettings.taintToleration and defaultSettings.systemManagedComponentsNodeSelector. These CSI components is not necessary to run on all nodes of cluster and in some cases it could be a requirement to use only specified nodes, e.g. control-plane. At other hand using available settings leads to other system-managed pods required for operation cannot start on all nodes.
Special notes for your reviewer:
This PR makes some minor fixes in the affected functions as well.
Additional documentation or context