csi: adjust CSI pods CPU requests on IBM Z#646
Open
malayparida2000 wants to merge 1 commit into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: malayparida2000 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 |
malayparida2000
force-pushed
the
ibm_z_csi_adjustment
branch
from
a4d9813 to
2057580
Compare
Contributor
Author
|
Got it tested on a IBM Z cluster, works fine, The csi pods restarted to take the values |
malayparida2000
force-pushed
the
ibm_z_csi_adjustment
branch
from
2057580 to
d6f6e48
Compare
Contributor
Author
|
leelavg
reviewed
Jul 1, 2026
| } | ||
| templates.CSIOperatorConfigSpec.DeepCopyInto(&csiOperatorConfig.Spec) | ||
| driverSpecDefaults := csiOperatorConfig.Spec.DriverSpecDefaults | ||
| if goruntime.GOARCH == ibmZCpuArch { |
Contributor
There was a problem hiding this comment.
why not goruntime.Is390x == 1 rather than using names?
Contributor
Author
There was a problem hiding this comment.
I could not find any variable or func of the type goruntime.Is390x / goruntime.IsS390x
Only GOARCH & GOOS are availble https://pkg.go.dev/runtime#pkg-constants
| templates.CSIOperatorConfigSpec.DeepCopyInto(&csiOperatorConfig.Spec) | ||
| driverSpecDefaults := csiOperatorConfig.Spec.DriverSpecDefaults | ||
| if goruntime.GOARCH == ibmZCpuArch { | ||
| adjustControllerPluginCpuResourcesForIbmZ(&driverSpecDefaults.ControllerPlugin.Resources, ibmZCpuAdjustFactor) |
Contributor
There was a problem hiding this comment.
Suggested change
| adjustControllerPluginCpuResourcesForIbmZ(&driverSpecDefaults.ControllerPlugin.Resources, ibmZCpuAdjustFactor) | |
| adjustControllerPluginCpuRequestsForIbmZ(&driverSpecDefaults.ControllerPlugin.Resources, ibmZCpuAdjustFactor) |
Contributor
Author
There was a problem hiding this comment.
I intentionally did not mention Request in the name, so that in future if required the same func can be expanded to adjust limits as well
| driverSpecDefaults := csiOperatorConfig.Spec.DriverSpecDefaults | ||
| if goruntime.GOARCH == ibmZCpuArch { | ||
| adjustControllerPluginCpuResourcesForIbmZ(&driverSpecDefaults.ControllerPlugin.Resources, ibmZCpuAdjustFactor) | ||
| adjustNodePluginCpuResourcesForIbmZ(&driverSpecDefaults.NodePlugin.Resources, ibmZCpuAdjustFactor) |
Contributor
There was a problem hiding this comment.
Suggested change
| adjustNodePluginCpuResourcesForIbmZ(&driverSpecDefaults.NodePlugin.Resources, ibmZCpuAdjustFactor) | |
| adjustNodePluginCpuRequestsForIbmZ(&driverSpecDefaults.NodePlugin.Resources, ibmZCpuAdjustFactor) |
Contributor
Author
There was a problem hiding this comment.
I intentionally did not mention Request in the name, so that in future if required the same func can be expanded to adjust limits as well
Comment on lines
+1429
to
+1447
| func adjustControllerPluginCpuResourcesForIbmZ(resources *csiopv1.ControllerPluginResourcesSpec, adjustFactor float64) { | ||
| adjustCpuResourcesForIbmZ(resources.Attacher, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Snapshotter, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Resizer, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Provisioner, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.OMapGenerator, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Liveness, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Addons, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.LogRotator, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Plugin, adjustFactor) | ||
| } | ||
|
|
||
| func adjustNodePluginCpuResourcesForIbmZ(resources *csiopv1.NodePluginResourcesSpec, adjustFactor float64) { | ||
| adjustCpuResourcesForIbmZ(resources.Registrar, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Liveness, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Addons, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.LogRotator, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Plugin, adjustFactor) | ||
| } |
Contributor
There was a problem hiding this comment.
if the adjust factor is same why not go over all the resources and set the values rather than picking each resource/sidecar because it creates a requirement to update things in two places when there is a new container. tnf also should've been like that.
Contributor
Author
There was a problem hiding this comment.
You are right, going over the list would be a better approach so that we cover any new containers if they come up in future
| } | ||
|
|
||
| func AdjustCPU(cpuQty resource.Quantity, adjustFactor float64) resource.Quantity { | ||
| str := strconv.FormatInt(int64(float64(cpuQty.MilliValue())*adjustFactor), 10) + "m" |
Contributor
There was a problem hiding this comment.
too many conversions i guess, took a quick look at the lib, could you try resource.NewMilliQuantity(cpuQty.MilliValue() / 2, resource.DecimalSI)?
Contributor
There was a problem hiding this comment.
why do you still require the conversions? you changed the value to milli and adjust factor leaves a reminder between 0 to that num (2 = 1, 9 = 8) and float to int anyways would curtail it making it redundant, am I missing something?
Contributor
Author
There was a problem hiding this comment.
@leelavg Did not get you, Are you asking about the float64 conv or the int64 conv
Madhu-1
reviewed
Jul 2, 2026
| adjustCpuResourcesForIbmZ(resources.Resizer, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Provisioner, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.OMapGenerator, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Liveness, adjustFactor) |
Member
There was a problem hiding this comment.
we dont have this anymore should be removed
Contributor
Author
There was a problem hiding this comment.
Member
There was a problem hiding this comment.
its not used in downstream we can remove it
Contributor
Author
There was a problem hiding this comment.
Now switched to iterate over the struct
|
|
||
| func adjustNodePluginCpuResourcesForIbmZ(resources *csiopv1.NodePluginResourcesSpec, adjustFactor float64) { | ||
| adjustCpuResourcesForIbmZ(resources.Registrar, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Liveness, adjustFactor) |
Contributor
Author
There was a problem hiding this comment.
| func adjustControllerPluginCpuResourcesForIbmZ(resources *csiopv1.ControllerPluginResourcesSpec, adjustFactor float64) { | ||
| adjustCpuResourcesForIbmZ(resources.Attacher, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Snapshotter, adjustFactor) | ||
| adjustCpuResourcesForIbmZ(resources.Resizer, adjustFactor) |
Member
There was a problem hiding this comment.
i think there is two more containers missing or two more related to odf snapshotter and snapshotter metadata
Contributor
Author
There was a problem hiding this comment.
Member
There was a problem hiding this comment.
odf-snapshotter uses the internal snapshotter and the metadata sidecar doesnt have the resource and limits set.
Halve default CSI controller and node plugin CPU requests when the operator runs on s390x, matching the IBM Z resource adjustment in ocs-operator. The reason for this is individual CPU cores on IBM Z are more than 2X powerful than the ones on other archs. Signed-off-by: Malay Kumar Parida <mparida@redhat.com>
malayparida2000
force-pushed
the
ibm_z_csi_adjustment
branch
from
d6f6e48 to
10ec298
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Halve default CSI controller and node plugin CPU requests when the operator runs on s390x, matching the IBM Z resource adjustment in ocs-operator. The reason for this is individual CPU cores on IBM Z are more than 2X powerful than the ones on other archs.
Ref-https://redhat.atlassian.net/browse/RHSTOR-9444