Add typed errors, add errors_total metric #74
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
MaciekPytel
force-pushed
the
autoscaler_errors
branch
from
8ccf364
to
5dedc2a
Compare
| } | ||
| if len(readyNodes) == 0 { | ||
| glog.Error("No ready nodes in the cluster") | ||
| scaleDown.CleanUpUnneededNodes() | ||
| return | ||
| return nil |
There was a problem hiding this comment.
You're returning nil because it doesn't look like an error?
Then, should we turn glog.Error into glog.Warning?
There was a problem hiding this comment.
Yes, I don't think having an empty cluster is an autoscaler error, so I'm returning nil here. I will update log levels accordingly.
So far I've only did enough in core/ to get this PR to compile, so I could quickly get feedback on the idea of introducing typed errors. We have a lot of places where returning error signal failure of some function, but not an actual error condition that should be reported as such (e.g. not scaling up due to reaching max cluster size is expected autoscaler behavior). So I will have to make a lot of changes all over the codebase and I wanted to get other opinions before I do so.
| } | ||
| if len(allNodes) == 0 { | ||
| glog.Error("No nodes in the cluster") | ||
| scaleDown.CleanUpUnneededNodes() | ||
| return | ||
| return nil |
There was a problem hiding this comment.
| @@ -157,24 +158,24 @@ func (a *StaticAutoscaler) RunOnce(currentTime time.Time) { | |||
| fixedSomething, err := fixNodeGroupSize(autoscalingContext, time.Now()) | |||
| if err != nil { | |||
| glog.Warningf("Failed to fix node group sizes: %v", err) | |||
| return | |||
| return errors.ToAutoscalerError(errors.CloudProviderError, err) | |||
There was a problem hiding this comment.
If it is an error, should we log it as so? More concretely by turning glog.Warningf into glog.Errorf
MaciekPytel
changed the title
MaciekPytel
force-pushed
the
autoscaler_errors
branch
from
dba0721
to
ef58a9c
Compare
To keep reasonable commit size only top-level files use new errors. Will add them in other files in next commits.
Updated some of the functions called by scale up to return new errors as required.
MaciekPytel
force-pushed
the
autoscaler_errors
branch
from
ef58a9c
to
58cdfa1
Compare
|
Since there is quite a bit of changes in various places in this PR, this is how I'm testing it:
|
|
LGTM |
Ref: #59