Allow to balance nodes using specified labels #3839

rtnpro · 2021-01-21T15:26:15Z

k8s-ci-robot · 2021-01-21T15:26:24Z

Welcome @rtnpro!

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. 😃

k8s-ci-robot · 2021-01-21T15:26:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rtnpro
To complete the pull request process, please assign aleksandra-malinowska after the PR has been reviewed.
You can assign the PR to them by writing /assign @aleksandra-malinowska in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

cluster-autoscaler/OWNERS

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

bpineau · 2021-01-25T19:52:15Z

cluster-autoscaler/processors/nodegroupset/compare_nodegroups.go

@@ -160,5 +167,10 @@ func IsCloudProviderNodeInfoSimilar(n1, n2 *schedulerframework.NodeInfo, ignored
 		return false
 	}

+	fmt.Println(includedLabels, len(includedLabels))
+	if len(includedLabels) > 0 && !compareLabels(nodes, includedLabels) {


Wasn't the #3615 intent to shortcut the full labels comparison (the if !compareLabels(nodes, ignoredLabels) above) with a comparison over a smaller set of includedLabels?

You are correct @bpineau . I will fix the PR

rtnpro · 2021-02-01T13:02:12Z

cluster-autoscaler/processors/nodegroupset/compare_nodegroups.go

+func CreateGenericNodeInfoLabelComparator(labels []string) NodeInfoComparator {
+	return func(n1, n2 *schedulerframework.NodeInfo) bool {
+		includedLabels := make(map[string]bool)
+		for _, l := range labels {
+			includedLabels[l] = true
+		}
+		if !compareLabels([]*schedulerframework.NodeInfo{n1, n2}, includedLabels, make(map[string]bool)) {
+			return false
+		}
+		return IsCloudProviderNodeInfoSimilar(n1, n2, make(map[string]bool))
+	}
+}
+


As suggested, I created a new comparator to accept labels instead of extraIgnoredLabels, and compare nodes based on the specified labels. This --balancing-labels arg here bypasses the --balancing-ignored-labels flag. This also ignores the cloud provider-specific basic ignored flags. This calls for some discussion.

Rather than creating a new comparator, I'd personally prefer to edit the signature of the current comparator creators to accept an extra argument for labels in addition to extraIgnoredLabels, and compare the nodes for matching labels specified in labels if labels is not empty. If check fails, then we return, else we proceed to call IsCloudProviderNodeInfoSimilar, which takes care of ignored labels.

@MaciekPytel please share your thoughts and review here.

I think the second implementation idea doesn't change current behavior at all. Currently every label that is not on ignored-labels list will cause the node groups to compare as different. Checking --balancing-labels first doesn't change the ultimate result, it just fails the comparison slightly earlier.

My understanding of #3615 is that specifying --balancing-labels makes comparison treat any label not on the list as ignored-label (current implementation treats any label that is not on ignored list as "balancing labels"). I think that:

Specifying both --balancing-labels and --balancing-ignored-labels makes no sense. What would be the behavior for any label not on either list?

(if you agree with this logic) we should add validation in main.go that prevents setting both flags at the same time.

Ignoring cloudprovider labels seems irrelevant when using --balancing-labels (since we would be ignoring all labels not on the list).

On the implementation side - calling IsCloudProviderNodeInfoSimilar as-is will not work as far as I can see, since it already has the label check embedded. I think the easiest way is to extract the resources check to a separate function and call that new function if --balancing-labels are specified, but there are plenty of other ways to refactor and I don't feel particularly strongly about which is the right one.

👍 to

Making --balancing-labels and --balancing-ignored-labels mutually exclusive

Ignoring cloudprovider labels irrelevant when using --balancing-labels

Yes, we'll need to refactor a underlying implementation of IsCloudProviderNodeInfoSimilar as you said. Since this is my first issue on CA, I was a bit hesitant to do much refactoring. Thanks for asserting my thoughts.

rtnpro · 2021-02-16T15:11:39Z

@MaciekPytel I pushed some changes to this PR today, as per your last review comments: fd5434f

Need a review here. Thanks

rtnpro · 2021-04-12T15:13:08Z

@MaciekPytel could I get a review here?

fejta-bot · 2021-07-22T17:54:34Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

k8s-triage-robot · 2021-08-21T18:25:39Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2021-09-20T19:00:42Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2021-09-20T19:00:59Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 21, 2021

k8s-ci-robot requested review from aleksandra-malinowska and towca January 21, 2021 15:26

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 21, 2021

bpineau reviewed Jan 25, 2021

View reviewed changes

rtnpro force-pushed the balancing-node-explicit-labels branch from a76493f to 87986aa Compare January 31, 2021 11:23

Create a new comparator to balance nodes based on specified labels.

548a061

rtnpro force-pushed the balancing-node-explicit-labels branch from 87986aa to 548a061 Compare February 1, 2021 12:44

rtnpro commented Feb 1, 2021

View reviewed changes

Fix CreateGenericNodeInfoLabelComparator and added tests.

fd5434f

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 16, 2021

Fixed formatting.

76f9572

jbartosik added the area/cluster-autoscaler label Apr 23, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 22, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 21, 2021

k8s-ci-robot closed this Sep 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to balance nodes using specified labels #3839

Allow to balance nodes using specified labels #3839

rtnpro commented Jan 21, 2021

k8s-ci-robot commented Jan 21, 2021

k8s-ci-robot commented Jan 21, 2021

bpineau Jan 25, 2021

rtnpro Jan 31, 2021

rtnpro Feb 1, 2021

MaciekPytel Feb 1, 2021

rtnpro Feb 9, 2021

rtnpro commented Feb 16, 2021

rtnpro commented Apr 12, 2021

fejta-bot commented Jul 22, 2021

k8s-triage-robot commented Aug 21, 2021

k8s-triage-robot commented Sep 20, 2021

k8s-ci-robot commented Sep 20, 2021

Allow to balance nodes using specified labels #3839

Allow to balance nodes using specified labels #3839

Conversation

rtnpro commented Jan 21, 2021

k8s-ci-robot commented Jan 21, 2021

k8s-ci-robot commented Jan 21, 2021

bpineau Jan 25, 2021

Choose a reason for hiding this comment

rtnpro Jan 31, 2021

Choose a reason for hiding this comment

rtnpro Feb 1, 2021

Choose a reason for hiding this comment

MaciekPytel Feb 1, 2021

Choose a reason for hiding this comment

rtnpro Feb 9, 2021

Choose a reason for hiding this comment

rtnpro commented Feb 16, 2021

rtnpro commented Apr 12, 2021

fejta-bot commented Jul 22, 2021

k8s-triage-robot commented Aug 21, 2021

k8s-triage-robot commented Sep 20, 2021

k8s-ci-robot commented Sep 20, 2021