Vsphere enable autoscaling from/to zero #839
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
SamuelStuchly
force-pushed
the
vsphere-autoscale-from-zero
branch
from
4c80542
to
9bac00d
Compare
SamuelStuchly
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/retest |
| @@ -4,6 +4,7 @@ go 1.13 | |||
|
|
|||
| require ( | |||
| github.com/blang/semver v3.5.1+incompatible | |||
| github.com/go-logr/logr v0.3.0 | |||
There was a problem hiding this comment.
Do we really need this? Can we use klog as we do everywhere?
There was a problem hiding this comment.
explanation by @JoelSpeed: "This interface is what controller runtime uses, so if we just use klog directly, we lose the logging that controller runtime would emit, so it's better to use the logger interface so we are sharing what controller runtime is doing"
There was a problem hiding this comment.
This is already a dependency, though just be an indirect dependency up to this point, looks like we have already run make vendor on this PR right? As there are no changes to go.sum this shows it's not a new depdencency
There was a problem hiding this comment.
Yes, seems thats the case.
There was a problem hiding this comment.
i'm not following why we are creating a new machineset crd manifest and the machineset controller for vsphere. @SamuelStuchly could you share your thoughts?
nvm, i see what you have done in regards to the other PRs you linked
SamuelStuchly
force-pushed
the
vsphere-autoscale-from-zero
branch
from
9bac00d
to
7112e79
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
| @@ -4,6 +4,7 @@ go 1.13 | |||
|
|
|||
| require ( | |||
| github.com/blang/semver v3.5.1+incompatible | |||
| github.com/go-logr/logr v0.3.0 | |||
There was a problem hiding this comment.
This is already a dependency, though just be an indirect dependency up to this point, looks like we have already run make vendor on this PR right? As there are no changes to go.sum this shows it's not a new depdencency
| switch t := err.(type) { | ||
| case *mapierrors.MachineError: | ||
| if t.Reason == machinev1.InvalidConfigurationMachineError { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
If you use the errors package you can use errors.Is instead of this function, it detects wrapped errors and unwraps them until it finds the error you wanted (if it was ever in the chain)
There was a problem hiding this comment.
After further talk with @JoelSpeed we decided the change is not actually worth here.
|
[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 |
openshift-ci-robot
added
the
approved
|
/lgtm |
|
@SamuelStuchly: The following tests failed, say
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/test-infra repository. I understand the commands that are listed here. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
This PR adds a new controller to annotate machinesets with information required to enable autoscaling to/from zero based on providerSpec input based on openshift/enhancements#186.
Analogous to openshift/cluster-api-provider-aws#301, openshift/cluster-api-provider-azure#112 and openshift/cluster-api-provider-gcp#77.