CORS-3741: [Nutanix] allow multi-subnets in Machine providerSpec and failureDomain configuration #2077
Conversation
|
Hello @yanhua121! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/L
yanhua121
force-pushed
the
nutanix-multi-subnets
branch
2 times, most recently
from
371b5d3
to
66fbeb9
Compare
|
/retest |
|
/assign @JoelSpeed |
There was a problem hiding this comment.
This change should be introduced behind a feature gate, please take a look at our docs, https://github.com/openshift/enhancements/blob/master/dev-guide/featuresets.md
yanhua121
changed the title
|
@yanhua121: An error was encountered searching for bug CORS-3741 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://issues.redhat.com/rest/api/2/issue/CORS-3741": GET https://issues.redhat.com/rest/api/2/issue/CORS-3741 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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. |
|
@yanhua121: An error was encountered searching for bug CORS-3741 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://issues.redhat.com/rest/api/2/issue/CORS-3741": GET https://issues.redhat.com/rest/api/2/issue/CORS-3741 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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. |
yanhua121
force-pushed
the
nutanix-multi-subnets
branch
from
66fbeb9
to
3086007
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yanhua121 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 |
yanhua121
force-pushed
the
nutanix-multi-subnets
branch
from
3086007
to
39ad5d1
Compare
config/v1/types_infrastructure.go
Outdated
| // +listMapKey=type | ||
| // +kubebuilder:validation:MaxItems=32 | ||
| // +listType=atomic | ||
| // +kubeubilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x == y))",message="each subnet must be unique" |
There was a problem hiding this comment.
Typo in this one sorry
| // +kubeubilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x == y))",message="each subnet must be unique" | |
| // +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x == y))",message="each subnet must be unique" |
There was a problem hiding this comment.
Actually, adding the suggested validation rule failed the ci/prow/verify-crd-schema test. When I tried to apply the "infrastructure" crd to my local OCP cluster, I got the same error:
The CustomResourceDefinition "infrastructures.config.openshift.io" is invalid:
spec.validation.openAPIV3Schema.properties[spec].properties[platformSpec].properties[nutanix].properties[failureDomains].items.properties[subnets].x-kubernetes-validations[0].rule: Forbidden: estimated rule cost exceeds budget by factor of more than 100x (try simplifying the rule, or adding maxItems, maxProperties, and maxLength where arrays, maps, and strings are declared) ......
So I removed this validation rule.
There was a problem hiding this comment.
@JoelSpeed Can you take another look?
There was a problem hiding this comment.
Is it because the parents slice, FailureDomains within the NutanixPlatformSpec does not have a maxItems. Do you know how many possible values we might expect for a list of FailureDomains? We could potentially impose a limit retrospectively thanks to ratcheting validation
There was a problem hiding this comment.
With the below extra rule, the ci/prow/verify-crd-schema and all the e2e test runs, and my local test failed with the above mentioned error: "infrastructures.config.openshift.io" is invalid
Without this rule (the current code change), all the test passed, as well as my local test.
// +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x == y))",message="each subnet must be unique"
There was a problem hiding this comment.
@JoelSpeed Can you see if the PR is approvable? I hope it can merge today so as to unblock the other 3 dependent PRs review process.
There was a problem hiding this comment.
The NutanixMachineProviderConfig can optionally refer to one FailureDomain by name. The FailureDomain configuration is in the Infrastructure CR.
And this field is being added to the infrastructure CR, as I mentioned, you need to limit the field FailureDomains slice that this field exists within, so that you can add the rule back
@JoelSpeed Can you see if the PR is approvable? I hope it can merge today so as to unblock the other 3 dependent PRs review process.
I previously requested this feature to be put behind a feature gate, I don't know if you saw that?
There was a problem hiding this comment.
@JoelSpeed Done the changes suggested, and the test ci/prow/verify-crd-schema passed. Please take another look.
There was a problem hiding this comment.
Added the feature gate as required.
yanhua121
force-pushed
the
nutanix-multi-subnets
branch
4 times, most recently
from
f5ec960
to
8e91447
Compare
|
@yanhua121: all tests passed! 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. |
|
New feature must now be implemented behind featuregates, especially this close to branching. Also, validation ratcheting is a new feature, so we will need to test the ratcheting feature for the new failure domains limit, for now, lets see if we can ask QE to create a cluster, add 257 failure domains, and then upgrade to this new schema and observe what happens |
yanhua121
force-pushed
the
nutanix-multi-subnets
branch
from
8e91447
to
d5da1a4
Compare
openshift-ci
bot
added
size/XXL
| @@ -1741,6 +1741,7 @@ type NutanixPlatformSpec struct { | |||
| // prism element clusters to improve fault tolerance of the cluster. | |||
| // +listType=map | |||
| // +listMapKey=name | |||
| // +openshift:validation:FeatureGateAwareMaxItems:featureGate=NutanixMultiSubnets,maxItems=32 | |||
There was a problem hiding this comment.
Did you intend for this to be 32? Previously we discussed 256, I'm happy with 32 if you are though
CORS-3741
Nutanix: allow multi-subnets in Machine providerSpec and failureDomain configuration