-
Notifications
You must be signed in to change notification settings - Fork 202
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.
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
Bug 1882723: vSphere: Create warning on invalid specs and remove defaults #732
Bug 1882723: vSphere: Create warning on invalid specs and remove defaults #732
Conversation
@alexander-demichev: This pull request references Bugzilla bug 1882723, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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 kubernetes/test-infra repository. |
if providerSpec.NumCPUs == 0 { | ||
errs = append(errs, field.Invalid(field.NewPath("providerSpec", "numCPUs"), providerSpec.NumCPUs, "numCPUs must be greater than 0")) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC there's something in the controller that caters for this scenario, can we check that before we add this one?
We want to verify that placing a 0 value in this field will result in a failed machine, have you tried this for each of the three errors you're adding?
@@ -980,14 +980,24 @@ func validateVSphere(m *Machine, clusterID string) (bool, []string, utilerrors.A | |||
|
|||
errs = append(errs, validateVSphereNetwork(providerSpec.Network, field.NewPath("providerSpec", "network"))...) | |||
|
|||
if providerSpec.NumCPUs < minVSphereCPU { | |||
warnings = append(warnings, fmt.Sprintf("providerSpec.numCPUs: %d is less than the minimum value (%d): the minimum value will be used instead", providerSpec.NumCPUs, minVSphereCPU)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing the minimum value will be used instead
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely forgot about this behavior.
5209c5c
to
c975e1b
Compare
@alexander-demichev: This pull request references Bugzilla bug 1882723, which is valid. 3 validation(s) were run on this bug
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 kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried this out on a real cluster, what happens if you set CPU 0?
@@ -980,14 +980,14 @@ func validateVSphere(m *Machine, clusterID string) (bool, []string, utilerrors.A | |||
|
|||
errs = append(errs, validateVSphereNetwork(providerSpec.Network, field.NewPath("providerSpec", "network"))...) | |||
|
|||
if providerSpec.NumCPUs < minVSphereCPU { | |||
warnings = append(warnings, fmt.Sprintf("providerSpec.numCPUs: %d is less than the minimum value (%d): the minimum value will be used instead", providerSpec.NumCPUs, minVSphereCPU)) | |||
if providerSpec.NumCPUs < minVSphereCPU || providerSpec.NumCPUs == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we actually need the or statement here, 0 < minVSphereCPU is true anyway. Similar goes for the cases below
c975e1b
to
d5f1c9d
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
@@ -981,13 +981,13 @@ func validateVSphere(m *Machine, clusterID string) (bool, []string, utilerrors.A | |||
errs = append(errs, validateVSphereNetwork(providerSpec.Network, field.NewPath("providerSpec", "network"))...) | |||
|
|||
if providerSpec.NumCPUs < minVSphereCPU { | |||
warnings = append(warnings, fmt.Sprintf("providerSpec.numCPUs: %d is less than the minimum value (%d): the minimum value will be used instead", providerSpec.NumCPUs, minVSphereCPU)) | |||
warnings = append(warnings, fmt.Sprintf("providerSpec.numCPUs: %d is missing or less than the minimum value (%d): the minimum value will be used instead", providerSpec.NumCPUs, minVSphereCPU)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we actually setting the minimum value? Below it looks like that logic was removed from the reconciler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic was removed from the webhook, but is being done in the reconcile loop currently
machine-api-operator/pkg/controller/vsphere/reconciler.go
Lines 550 to 561 in 44f29cc
numCPUs := s.providerSpec.NumCPUs | |
if numCPUs < minCPU { | |
numCPUs = minCPU | |
} | |
numCoresPerSocket := s.providerSpec.NumCoresPerSocket | |
if numCoresPerSocket == 0 { | |
numCoresPerSocket = numCPUs | |
} | |
memMiB := s.providerSpec.MemoryMiB | |
if memMiB == 0 { | |
memMiB = minMemMB | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Mike's comment came in while i was reviewing, removing my lgtm to ensure we don't merge before it's answered. |
d5f1c9d
to
8a8cf6e
Compare
@elmiko @michaelgugino Can I get another review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@alexander-demichev: All pull requests linked via external trackers have merged: Bugzilla bug 1882723 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
If the user forgets to specify CPU, memory, or disk size we default it to 0 and the reconciler then defaults it to a minimal value.
This PR: