Priority expander relies now on listers instead of direct watch api #1843

piontec · 2019-03-29T11:10:36Z

This is a rework of this PR: #1801
The changes are as follows:

it uses lister now
unittests rely on already existing tooling
app is never crashed, just error is logged

piontec · 2019-03-29T11:11:27Z

piontec · 2019-03-29T11:14:04Z

cluster-autoscaler/expander/factory/expander_factory.go

+
+func getPriorityExpandersConfigMapLister(kubeClient kube_client.Interface, stopChannel <-chan struct{},
+	namespace string) v1lister.ConfigMapLister {
+	// FIXME: how to be sure the reflector completed at least one run and switch to the line below?


@MaciekPytel I have a problem here: how to make sure the reflector completed at least 1 sync loop before I access the lister? I want to validate the configmap before starting.
If this can't be done easily, I probably have to give up on startup time validation and assume reflector will be ready when first call for Best comes. But even that smells like race condition.

CA loop generally takes a few seconds before it gets to expander and other listers must have already synced for a scale-up to have any chance of happening. Since you have a fallback strategy and the race is quite unlikely I'm ok to merge this without logic to explicitly make sure lister has initialized.
If you think you really need to make sure: I think the LastSyncResourceVersion() you do below is the best way unfortunately (at least the best way I know of). I don't like creating lister in here though. This file is really not the place for all those client-go internals. If you really want to sync, I'd rather you created your own configmap lister interface in utils/kubernetes/listers.go (like we do for pod listers) and expose a HasSynced() method.

piontec · 2019-03-29T11:14:47Z

cluster-autoscaler/expander/factory/expander_factory.go

-			return nil, errors.ToAutoscalerError(errors.InternalError, err)
-		}
-		return priority.NewStrategy(initialPriorities, priorityChangesChan, autoscalingKubeClients.LogRecorder)
+		// TODO: how to get proper termination info? It seems other listers do the same here


Other listers seems to be doing the same - please make sure it's ok to leave the termination channel like that

It's fine. Currently you can't change expander at runtime and you can't stop autoscaler main loop without killing the process, so we don't have any need to ever stop listers.

piontec · 2019-03-29T11:16:51Z

And one important question here: is there any comprehensive doc arund here about how to use the client-go lib properly? Either I can't find it or it's just a bunch of blog entries of kind "do this and that". The API is pretty big, I was running around different packages like crazy trying to figure out what's what and how to use it.

MaciekPytel · 2019-04-03T12:47:19Z

is there any comprehensive doc arund here about how to use the client-go lib properly?

I'm not aware of any such doc. I'm not an expert on client-go though, it may be worth asking directly on that repo or in kubernetes slack sig-apimachinery channel. As far as I know Kubernetes documentation is mostly limited to how to use it. Documentation for developing is minimal and the entry point for contributing is quite high.

MaciekPytel · 2019-04-03T12:59:52Z

cluster-autoscaler/expander/factory/expander_factory.go

-			return nil, errors.ToAutoscalerError(errors.InternalError, err)
-		}
-		return priority.NewStrategy(initialPriorities, priorityChangesChan, autoscalingKubeClients.LogRecorder)
+		// TODO: how to get proper termination info? It seems other listers do the same here


It's fine. Currently you can't change expander at runtime and you can't stop autoscaler main loop without killing the process, so we don't have any need to ever stop listers.

MaciekPytel · 2019-04-03T15:14:07Z

cluster-autoscaler/expander/factory/expander_factory.go

+
+func getPriorityExpandersConfigMapLister(kubeClient kube_client.Interface, stopChannel <-chan struct{},
+	namespace string) v1lister.ConfigMapLister {
+	// FIXME: how to be sure the reflector completed at least one run and switch to the line below?


CA loop generally takes a few seconds before it gets to expander and other listers must have already synced for a scale-up to have any chance of happening. Since you have a fallback strategy and the race is quite unlikely I'm ok to merge this without logic to explicitly make sure lister has initialized.
If you think you really need to make sure: I think the LastSyncResourceVersion() you do below is the best way unfortunately (at least the best way I know of). I don't like creating lister in here though. This file is really not the place for all those client-go internals. If you really want to sync, I'd rather you created your own configmap lister interface in utils/kubernetes/listers.go (like we do for pod listers) and expose a HasSynced() method.

piontec · 2019-04-04T06:20:03Z

OK, I didn't want to leave the code there, just make sure first how to solve this. In this case I'm in favor of not running configmap validation at startup and leave that until the first BestOption() call happens - then any potential problems will go to logs and CM's events. Let me know if you're OK with that and I can quickly change it.

piontec · 2019-04-08T06:54:30Z

ping @MaciekPytel

MaciekPytel · 2019-04-11T14:38:30Z

Sorry for delay. I'm ok with what you proposed in #1843 (comment).

piontec · 2019-04-12T07:30:24Z

@MaciekPytel Done, on-init config check is removed. Please review again.

MaciekPytel

One comment, otherwise lgtm (though the nature of comment suggests you haven't actually run the code, if that's true please test before merging).

MaciekPytel · 2019-04-12T09:23:49Z

cluster-autoscaler/utils/kubernetes/listers.go

+// NewConfigMapListerForNamespace builds a configmap lister for the passed namespace (including all).
+func NewConfigMapListerForNamespace(kubeClient client.Interface, stopchannel <-chan struct{},
+	namespace string) v1lister.ConfigMapLister {
+	listWatcher := cache.NewListWatchFromClient(kubeClient.AppsV1().RESTClient(), "configmaps", namespace, fields.Everything())


Configmap is in core/v1, not apps/v1. Does that work for you? Seems like it shouldn't.

Yeah, my bad, I ran tests actually before removing the test code here. I was sure it was exactly the same as in Listers - it wasn't. Fixing, and I'll re-run tests once more. Thanks for catching!

@MaciekPytel please check now

MaciekPytel · 2019-04-16T12:55:28Z

/lgtm
/approve

k8s-ci-robot · 2019-04-16T12:55:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MaciekPytel

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~~ [MaciekPytel]

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

Cherry pick #1801 #1843 #1889 #1920 to 1.14: Priority Expander

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 29, 2019

k8s-ci-robot requested review from aleksandra-malinowska and piosz March 29, 2019 11:10

k8s-ci-robot assigned MaciekPytel Mar 29, 2019

piontec commented Mar 29, 2019

View reviewed changes

piontec force-pushed the feature/priority_expander_remake_squashed branch from 142e057 to 1516d42 Compare March 29, 2019 12:37

bskiba added the area/cluster-autoscaler label Apr 1, 2019

MaciekPytel requested changes Apr 3, 2019

View reviewed changes

piontec mentioned this pull request Apr 11, 2019

rework addon-resizer to use client-go #1859

Merged

piontec force-pushed the feature/priority_expander_remake_squashed branch from 1516d42 to 17e4c89 Compare April 12, 2019 07:29

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 12, 2019

MaciekPytel reviewed Apr 12, 2019

View reviewed changes

MaciekPytel mentioned this pull request Apr 12, 2019

Disable failing priority expander tests #1889

Merged

piontec force-pushed the feature/priority_expander_remake_squashed branch from 17e4c89 to 119dc75 Compare April 15, 2019 09:20

rely on listers instead of watch api

856e58d

piontec force-pushed the feature/priority_expander_remake_squashed branch from 119dc75 to 856e58d Compare April 16, 2019 11:48

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2019

k8s-ci-robot merged commit 6675f95 into kubernetes:master Apr 16, 2019

piontec deleted the feature/priority_expander_remake_squashed branch April 17, 2019 10:44

piontec mentioned this pull request Apr 18, 2019

priority expander based on client-go for cluster autoscaler is missing permissions #1920

Merged

danielmellado mentioned this pull request Oct 24, 2019

Rebase to upstream/cluster-autoscaler-release-1.16 openshift/kubernetes-autoscaler#119

Closed

Jeffwan mentioned this pull request Nov 27, 2019

Cherry pick #1801 #1843 #1889 #1920 to 1.14: Priority Expander #2578

Merged

k8s-ci-robot added a commit that referenced this pull request Nov 28, 2019

Merge pull request #2578 from Jeffwan/1.14

9430261

Cherry pick #1801 #1843 #1889 #1920 to 1.14: Priority Expander

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Priority expander relies now on listers instead of direct watch api #1843

Priority expander relies now on listers instead of direct watch api #1843

piontec commented Mar 29, 2019

piontec commented Mar 29, 2019

piontec Mar 29, 2019

MaciekPytel Apr 3, 2019

piontec Mar 29, 2019

MaciekPytel Apr 3, 2019

piontec commented Mar 29, 2019

MaciekPytel commented Apr 3, 2019

MaciekPytel Apr 3, 2019

MaciekPytel Apr 3, 2019

piontec commented Apr 4, 2019

piontec commented Apr 8, 2019

MaciekPytel commented Apr 11, 2019

piontec commented Apr 12, 2019

MaciekPytel left a comment

MaciekPytel Apr 12, 2019

piontec Apr 12, 2019

piontec Apr 15, 2019

MaciekPytel commented Apr 16, 2019

k8s-ci-robot commented Apr 16, 2019

Priority expander relies now on listers instead of direct watch api #1843

Priority expander relies now on listers instead of direct watch api #1843

Conversation

piontec commented Mar 29, 2019

piontec commented Mar 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piontec commented Mar 29, 2019

MaciekPytel commented Apr 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piontec commented Apr 4, 2019

piontec commented Apr 8, 2019

MaciekPytel commented Apr 11, 2019

piontec commented Apr 12, 2019

MaciekPytel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaciekPytel commented Apr 16, 2019

k8s-ci-robot commented Apr 16, 2019