OSASINFRA-3179: openstack failureDomain: Add RootVolume type#1496
Conversation
|
Hello @pierreprinetti! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/S
|
Nice addition, thanks. Could you add a test to the testsuite among rootVolume.availabilityZone maybe? Thanks |
pierreprinetti
force-pushed
the
openstack_volume_type
branch
from
502c252 to
1cb3e26
Compare
openshift-ci
bot
added
size/L
|
/lgtm Thanks Pierre for this addition. I liked the validations & the tests. |
| // +kubebuilder:validation:MaxLength=255 | ||
| // +kubebuilder:validation:Pattern=`^[^ ]*$` | ||
| // +optional | ||
| VolumeType string `json:"volumeType,omitempty"` |
There was a problem hiding this comment.
This corresponds exactly with our machinev1alpha1 API:
https://github.com/openshift/api/blob/master/machine/v1alpha1/types_openstack.go#L358
pierreprinetti
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@pierreprinetti: This pull request references OSASINFRA-3179 which is a valid jira issue. 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 kubernetes/test-infra repository. |
|
/assign JoelSpeed |
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Couple of documentation nits, but otherwise LGTM
| // The maximum length of a volume type name is 255 as per the OpenStack limit. | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=255 | ||
| // +kubebuilder:validation:Pattern=`^[^ ]*$` |
There was a problem hiding this comment.
Can you explain this validation in prose within the godoc please?
Can you possibly also provide examples of valid types that a user might put in there?
There was a problem hiding this comment.
Volume types with a space are apparently legit however the handling of those starting with a space appears to be buggy (actually it was me being buggy). I am updating the regex accordingly.
EDIT: I am removing the white space restrictions, as they are allowed in any position within the volume type name.
pierreprinetti
force-pushed
the
openstack_volume_type
branch
from
1cb3e26 to
c3af31f
Compare
pierreprinetti
force-pushed
the
openstack_volume_type
branch
2 times, most recently
from
bb0f158 to
f5676c6
Compare
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
The volumeType is as of OpenStack v17 the canonical way to distribute storage in separate failure domains. This change introduces the ability to set a variable volume type for the Control plane instances.
pierreprinetti
force-pushed
the
openstack_volume_type
branch
from
f5676c6 to
5ca5168
Compare
|
@pierreprinetti: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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/test-infra repository. I understand the commands that are listed here. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=255 | ||
| // +optional | ||
| VolumeType string `json:"volumeType,omitempty"` |
There was a problem hiding this comment.
Are there definitely no character limitations on this? Are there any examples of valid volume types we can include in the godoc to give the user an idea of a correct value?
There was a problem hiding this comment.
There are no documented character limitations. Volume types are typically created by the OpenStack infrastructure administrators, and we are a tenant payload. This means that in practice, OpenShift administrators put here ready-made volume types that their OpenStack administrators make available to them.
For instance, as an admin of my cloud I just did this:
$ openstack volume type create '432,+?^%$_ ù\f ~`helluva name'
+-------------+--------------------------------------+
| Field | Value |
+-------------+--------------------------------------+
| description | None |
| id | ab3c8647-1bca-4ab8-89dd-f1ce1453ec3e |
| is_public | True |
| name | 432,+?^%$_ ù\f ~`helluva name |
+-------------+--------------------------------------+There was a problem hiding this comment.
Ok, in that case I guess there's not a lot we can do
|
Are we expecting any further changes? Has this been tested at all to give us confidence in the API? I have no further feedback given we've gone through and established there's no patterns etc that can be applied |
|
I don't expect any further changes on this. As for the pattern, I have slavishly followed the OpenStack ProviderSpec nomenclature. Tests will happen in the CPMS PR. Do you want to defer merging until that PR is ready? |
|
No, I think in this case it's fine, I know that PR is pretty much ready to go |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: EmilienM, JoelSpeed, pierreprinetti The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
5 participants
The volumeType is as of OpenStack v17 the canonical way to distribute storage in separate failure domains. This change introduces the ability to set a variable volume type for the Control plane instances.
/cc mandre emilienm