Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

STOR-2141: add MaxAllowedBlockVolumesPerNode field to VSphereCSIDriverConfigSpec #2190

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

STOR-2141: add MaxAllowedBlockVolumesPerNode field to VSphereCSIDriverConfigSpec #2190

wants to merge 1 commit into from

Conversation

RomanBednar
Copy link
Contributor

No description provided.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 3, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 3, 2025 •
edited by openshift-ci bot
Loading

@RomanBednar: This pull request references STOR-2141 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 epic to target the "4.19.0" version, but no target version was set.

In 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.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 3, 2025

Hello @RomanBednar! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@openshift-ci openshift-ci bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 3, 2025
@openshift-ci openshift-ci bot requested review from deads2k and sjenning February 3, 2025 17:02
@RomanBednar RomanBednar mentioned this pull request Feb 3, 2025
Open
everettraven
everettraven reviewed Feb 3, 2025
View reviewed changes
everettraven
everettraven approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. Tagging in @JoelSpeed for a round of review/approval.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 5, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: everettraven, RomanBednar
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@RomanBednar
Copy link
Contributor Author

The error in verify-crd-schema job does not sound related to this PR: ConditionsMustHaveProperSSATags: crd/clustercsidrivers.operator.openshift.io version/v1 field/^.status.condition must define valid condition properties: observedGeneration attribute is missing - is that a good candidate for /override?

JoelSpeed
JoelSpeed reviewed Feb 6, 2025
View reviewed changes
@RomanBednar RomanBednar force-pushed the STOR-2141 branch 2 times, most recently from 78a4793 to 17d87dc Compare February 12, 2025 15:12
@RomanBednar
Copy link
Contributor Author

ping @JoelSpeed - to review the new version.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 20, 2025

@RomanBednar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-serial e269021 link true /test e2e-aws-serial
ci/prow/e2e-aws-serial-techpreview e269021 link true /test e2e-aws-serial-techpreview
ci/prow/verify-crd-schema e269021 link true /test verify-crd-schema

Full PR test history. Your PR dashboard.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

JoelSpeed
JoelSpeed reviewed Feb 24, 2025
View reviewed changes
operator/v1/types_csi_cluster_driver.go
@@ -369,6 +369,21 @@ type VSphereCSIDriverConfigSpec struct {
// +openshift:enable:FeatureGate=VSphereDriverConfiguration
// +optional
GranularMaxSnapshotsPerBlockVolumeInVVOL *uint32 `json:"granularMaxSnapshotsPerBlockVolumeInVVOL,omitempty"`

// maxAllowedBlockVolumesPerNode is an optional configuration parameter that allows setting custom value for limit of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// maxAllowedBlockVolumesPerNode is an optional configuration parameter that allows setting custom value for limit of
// maxAllowedBlockVolumesPerNode is an optional configuration parameter that allows setting a custom value for the limit of

operator/v1/types_csi_cluster_driver.go
@@ -369,6 +369,21 @@ type VSphereCSIDriverConfigSpec struct {
// +openshift:enable:FeatureGate=VSphereDriverConfiguration
// +optional
GranularMaxSnapshotsPerBlockVolumeInVVOL *uint32 `json:"granularMaxSnapshotsPerBlockVolumeInVVOL,omitempty"`

// maxAllowedBlockVolumesPerNode is an optional configuration parameter that allows setting custom value for limit of
// PersistentVolumes attached to a node. In vSphere version 7 this limit was set to 59 by default, however in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// PersistentVolumes attached to a node. In vSphere version 7 this limit was set to 59 by default, however in
// the number of PersistentVolumes attached to a node.
// In vSphere version 7 this limit was set to 59 by default, however in

operator/v1/types_csi_cluster_driver.go
// +kubebuilder:validation:Maximum=255
// +openshift:enable:FeatureGate=VSphereConfigurableMaxAllowedBlockVolumesPerNode
// +optional
MaxAllowedBlockVolumesPerNode *int32 `json:"maxAllowedBlockVolumesPerNode,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have omitempty on this optional integer, and 0 is not a valid value, so you don't need this to be a pointer

...lustercsidrivers.operator.openshift.io/VSphereConfigurableMaxAllowedBlockVolumesPerNode.yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a test for a negative value? Or 0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnufied gnufied gnufied left review comments

@JoelSpeed JoelSpeed JoelSpeed left review comments

@everettraven everettraven everettraven approved these changes

@deads2k deads2k Awaiting requested review from deads2k

@sjenning sjenning Awaiting requested review from sjenning

Assignees
No one assigned
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RomanBednar @openshift-ci-robot @gnufied @JoelSpeed @everettraven