Add unremovable_nodes_count metric

[evgenii-petrov-arrival]

I want to set up alerting for cases where nodes are unremovable due to user errors, like lack of "safe to deschedule" annotation, and so this Pull Requests adds a Gauge sliced by fmt.Sprintf("%s", simulator.UnremovableReason).

I wasn't sure if there is any concurrent access involved, so added a mutex for the counter map, please let me know if it is unnecessary.

I also wasn't sure how to test this, any suggestions are welcome.

[k8s-ci-robot]

Welcome @evgenii-petrov-arrival!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[mwielgus]

A related test is failing:

--- FAIL: TestFindUnneededNodes (0.02s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x3b7791f]

Please fix your code.

[evgenii-petrov-arrival]

Please fix your code.

Done.

Sorry for not running the gofmt/golint/tests before create a pull request, I haven't figured out how vendoring is structured in the repo and was depending on CI to catch any errors while I'm figuring that out.

[evgenii-petrov-arrival]

@mwielgus , as far as I can tell, there is no way for me to mark your request for changes as addressed, so just in case I'll duplicate a request for re-review as a comment.

I think I've addressed your request for changes and the tests are passing now.

[MaciekPytel]

It seems to me that this is duplicating the logic for gathering unready reasons that we already have for ScaleDownStatusProcessor. I think implementing this as a processor should be preferred to avoid duplication, keep the aggregation logic encapsulated and avoid bloating scale-down logic even more.

[evgenii-petrov-arrival]

It seems to me that this is duplicating the logic for gathering unready reasons that we already have for ScaleDownStatusProcessor.

I've looked at ScaleDownStatusProcessor after your comment (wasn't aware of it before), and I don't think that adding a processor here would reduce complexity. Currently NoOpScaleDownStatusProcessor is the default ScaleDownStatusProcessor. And changing from NoOp to SomeOp seems to be a much larger change than adding a metric next to another similar metric.

That said, moving both metrics.UpdateUnneededNodesCount and metrics.UpdateUnremovableNodesCount to ScaleDownStatusProcessor seems like a good idea, but that refactoring is a much larger change than this one, and I think shouldn't block addition of this feature.

[evgenii-petrov-arrival]

@mwielgus , is there something I can do to advance this pull request through the review?

[towca]

Adding a processor only for updating one metric does seem like a bit of an overkill to me. This PR isn't actually duplicating any existing logic, since we don't pivot the unremovableNodeReasons map into per-reason counters anywhere. That said, I see two major problems with the proposed approach.

First of all, unremovable reasons are also set in TryToScaleDown(), which is called after UpdateUnneededNodes(). If you set the metric at the end of UpdateUnneededNodes(), you'll miss all of the reasons found in TryToScaleDown(). I think a good place to update this metric is just after TryToScaleDown() is called in RunOnce().

Secondly, I think you're missing the fact that the unremovableNodeReasons map is cleared every loop, at the beginning of the scale-down logic in RunOnce() - in scaleDown.CleanUp(). So you don't need to keep track of how many reasons were added in the current loop in another structure. I'd recommend adding a getUnremovableNodeReasonCounters() map[simulator.UnremovableReason]int method to ScaleDown, which would go through all nodes in unremovableNodeReasons and count how many times each reason occurs. Then you can just pass its result to the function which updates the metric.

[evgenii-petrov-arrival]

Thanks @towca , both very good points. I think I've addressed them know, please take a look.

[towca]

Sorry for missing your reply, thanks for addressing my comments!

/lgtm

[mwielgus]

Please squash commits to just 1-2 and we are good to merge.

[evgenii-petrov-arrival]

Squashed to one commit, please take another look.

[towca]

/lgtm

[mwielgus]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evgenii-petrov-arrival, mwielgus

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:

• cluster-autoscaler/OWNERS [mwielgus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment