rework addon-resizer to use client-go #1859

piontec · 2019-04-04T12:52:55Z

The addon-resizer code doesn't modify attributes other than limits/requests, so it seems the bug is in libraries used. This is causing problems like #1858 and probably also #1799 and #1457 . To solve that, I upgraded libraries to more recent ones.

Changes include:

drop godep in favor of go mod - I was becoming sick trying to do that using godep
code is reworked to use client-go (version v8.0.0) and api libs (from 1.11 k8s release)
go version is updated to 1.12.1
Makefile builds docker image OK

Testing:

unit tests are passing
I manually verified that addon-scaler works OK running against an already provisioned cluster with kube-state-metrics pod.

piontec · 2019-04-04T12:57:20Z

Assuming the switch to go mod is OK, we can drop vendor/ in the repo and fetch dependencies on build time. Let me know if that's ok.

bskiba · 2019-04-05T11:12:53Z

In general, huge, huge thank you for taking the time and the effort!

Seeing core Kubernetes moving to go mod (https://groups.google.com/forum/#!topic/kubernetes-dev/ECejidBCELI) I think it's the right thing to move addon resizer too.

I have one request, can you split the go mod change from the client-go one? It will make it way easier for me to remove the code that actually changed.

piontec · 2019-04-05T11:20:11Z

Crap, I squashed my commits - I can't easily separate them now. But if we remove vendor/ and rely entirely on the new go mod, the changes should be much easier to review - the whole switch will be just in removing Godeps/ and adding go.mod. What do you think?

bskiba · 2019-04-05T11:43:06Z

There are several problems:

I need to make sure the dependencies haven't changed - we are not pulling in problematic licenses
I believe we need to have a copy of the license for all vendored dependencies

I can try to review this as a whole but I'll need time. I also need to consult someone around the license thing, I'll get back to you next week.
It's easy to omit things with such a huge change. Example: you have two different go versions specified, one in Makefile (1.12) and another in go.mod (1.11). Is that intentional?

piontec · 2019-04-05T12:07:53Z

Well, dependencies have changed, that's for sure. client-go wasn't even used before and it is now. The same with apimachinery. The problem with this project was that dependencies were so old and the project so small, that I actually removed all dependencies, started with empty go.mod and then I was rewriting the code to use new libraries, adding them to go.mod on the way.

As for version mismatch - my mistake, I forgot to switch everything to 1.12. Will fix in a minute.

bskiba · 2019-04-09T13:37:34Z

I talked a bit more with other sig-autoscaling members and it seems like we need a common answer for autoscaler components and go modules. It will take us a couple of weeks probably to flesh it out.
Until we do this I will not review this PR unfortunately, unless there is a way for you to make the desired change and use godeps for the time being.

piontec · 2019-04-10T10:35:34Z

OK, I'll try to rework using godeps. Do you have some hints about how to include client-go with it the easiest way?

piontec · 2019-04-11T08:10:50Z

@bskiba Done, it's using godep now. Can you also have a look at #1843, it seems to be stuck a little...

bskiba

Nits only, otherwise lgtm, thanks!

bskiba · 2019-04-16T09:46:31Z

addon-resizer/Makefile

@@ -65,9 +65,9 @@ container: .container-$(ARCH)
 	docker run --rm -it -v $(TEMP_DIR):$(TEMP_DIR):Z -v `pwd`:/go/src/k8s.io/autoscaler/addon-resizer/:Z \
        golang:${GOLANG_VERSION} \
        /bin/bash -c "\
-		go get github.com/tools/godep && \
+								go get github.com/tools/godep && \


Not sure where the whitespace changes come from.

bskiba · 2019-04-16T10:06:17Z

addon-resizer/nanny/kubernetes_client.go

-func (k *kubernetesClient) ContainerResources() (*apiv1.ResourceRequirements, error) {
-	pod, err := k.clientset.CoreClient.Pods(k.namespace).Get(k.pod)
+func (k *kubernetesClient) Stop() {
+	for i := 0; i < 3; i++ {


The i < 3 seems really error prone - if someone adds another lister, no way they will know thay have to increase it. Can you add a stopChan per lister instead?

bskiba · 2019-04-16T10:06:46Z

addon-resizer/nanny/kubernetes_client.go

-	wait "k8s.io/kubernetes/pkg/util/wait"
-	watch "k8s.io/kubernetes/pkg/watch"
+	apps "k8s.io/api/apps/v1"
+	api "k8s.io/api/core/v1"


nit: s/api/core/

bskiba · 2019-04-16T10:10:16Z

addon-resizer/main.go

+}
+
+// GetClientOutOfCluster returns a k8s clientset to the request from outside of cluster
+func GetClientOutOfCluster() kubernetes.Interface {


nit: GetClientOutOfClusterOrDie() (here and below)

piontec · 2019-04-17T14:08:13Z

@bskiba please check now

bskiba · 2019-04-17T14:34:48Z

/lgtm
/approve

k8s-ci-robot · 2019-04-17T14:35:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bskiba

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:

~~addon-resizer/OWNERS~~ [bskiba]

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

piontec · 2019-04-30T12:08:59Z

Hi @bskiba , do you have any time frame for releasing this?

diclophis · 2019-05-01T06:29:48Z

Also where can we find newer tagged images of this addon-resizer docker image?

bskiba · 2019-05-02T09:05:58Z

@piontec I'll try to release in the next two weeks.
@diclophis All images should be in k8s.gcr.io/addon-resizer, the newest is 2.1

wojtek-t · 2019-05-06T10:48:48Z

@bskiba - note that we're still using 1.8.* releases, as there were some issues with 2.* releases
I can't remember the details, but it's something to keep in mind.

bskiba · 2019-05-06T14:04:23Z

@wojtek-t Thanks! I wasn't planning on changing the default, and I think that there are folks successfully using the 2.X releases so it still makes sense to release. But it's good to note that maybe this fix should go to the 1.8 release as well. @piontec are you interested in porting to 1.8 or is the master branch satisfactory?

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 4, 2019

k8s-ci-robot requested review from bskiba and wojtek-t April 4, 2019 12:53

piontec changed the title ~~rework addon-resizer to use client-go (should solve #1799 #1858 and similar)~~ rework addon-resizer to use client-go Apr 4, 2019

piontec force-pushed the fix/addon-resizer-client-go branch from 3debf66 to 23534a8 Compare April 5, 2019 09:39

bskiba self-assigned this Apr 5, 2019

rework addon-resizer to use client-go (should solve #7199 and similar)

a61786a

piontec force-pushed the fix/addon-resizer-client-go branch from eedf74b to a61786a Compare April 11, 2019 07:59

bskiba reviewed Apr 16, 2019

View reviewed changes

small fixes and cleanup changes

afa1955

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

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

k8s-ci-robot merged commit 33d582f into kubernetes:master Apr 17, 2019

piontec deleted the fix/addon-resizer-client-go branch April 18, 2019 06:42

danielmellado mentioned this pull request Oct 24, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework addon-resizer to use client-go #1859

rework addon-resizer to use client-go #1859

piontec commented Apr 4, 2019

piontec commented Apr 4, 2019

bskiba commented Apr 5, 2019

piontec commented Apr 5, 2019

bskiba commented Apr 5, 2019

piontec commented Apr 5, 2019

bskiba commented Apr 9, 2019

piontec commented Apr 10, 2019

piontec commented Apr 11, 2019

bskiba left a comment

bskiba Apr 16, 2019

bskiba Apr 16, 2019

bskiba Apr 16, 2019

bskiba Apr 16, 2019

piontec commented Apr 17, 2019

bskiba commented Apr 17, 2019

k8s-ci-robot commented Apr 17, 2019

piontec commented Apr 30, 2019

diclophis commented May 1, 2019

bskiba commented May 2, 2019

wojtek-t commented May 6, 2019

bskiba commented May 6, 2019

rework addon-resizer to use client-go #1859

rework addon-resizer to use client-go #1859

Conversation

piontec commented Apr 4, 2019

piontec commented Apr 4, 2019

bskiba commented Apr 5, 2019

piontec commented Apr 5, 2019

bskiba commented Apr 5, 2019

piontec commented Apr 5, 2019

bskiba commented Apr 9, 2019

piontec commented Apr 10, 2019

piontec commented Apr 11, 2019

bskiba left a comment

Choose a reason for hiding this comment

bskiba Apr 16, 2019

Choose a reason for hiding this comment

bskiba Apr 16, 2019

Choose a reason for hiding this comment

bskiba Apr 16, 2019

Choose a reason for hiding this comment

bskiba Apr 16, 2019

Choose a reason for hiding this comment

piontec commented Apr 17, 2019

bskiba commented Apr 17, 2019

k8s-ci-robot commented Apr 17, 2019

piontec commented Apr 30, 2019

diclophis commented May 1, 2019

bskiba commented May 2, 2019

wojtek-t commented May 6, 2019

bskiba commented May 6, 2019