Updates for new k-c-m flag #1465

chrislovecnm · 2017-01-13T03:21:14Z

New flag for kubernetes controller manager that reduces the number of API calls that k-c-m makes. In 1.4.7 and 1.5.x a new feature was added to k-c-m. That feature can now spam AWS APIs if not tuned correctly.

This change is

k8s-ci-robot · 2017-01-16T22:09:29Z

Jenkins Kubernetes AWS e2e failed for commit 34bc061. Full PR test history. cc @chrislovecnm

The magic incantation to run this job again is @k8s-bot aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

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. I understand the commands that are listed here.

chrislovecnm · 2017-01-16T22:13:23Z

I am not able to get the yaml updating on the cluster spec of a running server:

kops get cluster --name my.cluster --full -o yaml

  kubeControllerManager:
    allocateNodeCIDRs: true
    attachDetachReconcileSyncPeriod: 1m0s
    cloudProvider: aws
    clusterCIDR: 100.96.0.0/11
    clusterName: liea-ginny.dev.datapipe.io
    configureCloudRoutes: false
    image: gcr.io/google_containers/kube-controller-manager:v1.5.2
    leaderElection:
      leaderElect: true
    logLevel: 2
    master: 127.0.0.1:8080
    master: 127.0.0.1:8080
    config: /etc/kubernetes/manifests
    enableDebuggingHandlers: true
    hostnameOverride: '@aws'
    logLevel: 2
    networkPluginMTU: null
    networkPluginName: cni
    nonMasqueradeCIDR: 100.64.0.0/10

Ideas?

krisnova · 2017-01-17T00:59:11Z

pkg/model/components/kubecontrollermanager.go

+		return fmt.Errorf("Unable to parse kubernetesVersion v1.4.8")
+	}
+
+	k8sv152, _ := kops.ParseKubernetesVersion("v1.5.2")


Assuming this returns an error

k8sv152, _

Shouldn't we grab it?

already did in an update ... will push

justinsb

A bunch of style nits. Will now look at why the flag isn't working...

justinsb · 2017-01-17T00:39:17Z

pkg/model/components/kubecontrollermanager_test.go

+	}
+
+	if spec.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration != time.Minute {
+		t.Fatalf("k-c-m builder should be set to 1m - %s", spec.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration.String())


s/k-c-m- builder/AttachDetachReconcileSyncPeriod/g

justinsb · 2017-01-17T00:39:42Z

pkg/model/components/kubecontrollermanager_test.go

+	err := kcm.BuildOptions(&spec)
+
+	if err != nil {
+		t.Fatalf("k-c-m builder errors: %v", err)


s/k-c-m builder errors/unexpected error from BuildOptions/g

justinsb · 2017-01-17T00:40:20Z

pkg/model/components/kubecontrollermanager_test.go

+	}
+
+	if spec.KubeControllerManager.AttachDetachReconcileSyncPeriod != nil {
+		t.Fatalf("k-c-m builder cannot be set for k8s %s", spec.KubernetesVersion)


"AttachDetachReconcileSyncPeriod should not be set for old kubernetes version %s"

justinsb · 2017-01-17T00:40:32Z

pkg/model/components/kubecontrollermanager_test.go

+	err := kcm.BuildOptions(&spec)
+
+	if err != nil {
+		t.Fatalf("k-c-m builder errors: %v", err)


"unexpected error from BuildOptions"

justinsb · 2017-01-17T00:44:33Z

pkg/model/components/kubecontrollermanager_test.go

+	UpdatePolicy      string
+}
+
+func buildCluster(clusterArgs interface{}) *api.Cluster {


Why interface{}?

Just code I grabbed from elsewhere, will clean-up the tests some

justinsb · 2017-01-17T00:54:28Z

pkg/model/components/kubecontrollermanager.go

+		return fmt.Errorf("Unable to parse kubernetesVersion v1.4.8")
+	}
+
+	k8sv152, _ := kops.ParseKubernetesVersion("v1.5.2")


You need to assign err here

have in an update

justinsb · 2017-01-17T00:59:54Z

pkg/model/components/kubecontrollermanager.go

+
+	kubernetesVersion, err := b.Context.KubernetesVersion()
+	if err != nil {
+		return fmt.Errorf("Unable to parse kubernetesVersion")


You don't need to add to the error - the error that comes back from KubernetesVersion is already pretty good I think. But you shouldn't swallow the error i.e. do fmt.Errorf("additional info: %v", err)

justinsb · 2017-01-17T01:00:42Z

pkg/model/components/kubecontrollermanager.go

+		glog.Info("k-c-m default-attach-detach-reconcile-sync-period correct version found")
+		// If not set ... or set to 0s ... which is stupid
+		if options.KubeControllerManager.AttachDetachReconcileSyncPeriod == nil ||
+			options.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration.String() == "0s" {


I think you can do == 0 rather than calling .String() == "0s"

That did not work when I tested it :(

justinsb · 2017-01-17T01:01:13Z

pkg/model/components/kubecontrollermanager.go

+		if options.KubeControllerManager.AttachDetachReconcileSyncPeriod == nil ||
+			options.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration.String() == "0s" {
+
+			glog.V(8).Info("k-c-m default-attach-detach-reconcile-sync-period flag is set to default")


"AttachDetachReconcileSyncPeriod is not set; will set to default %v", defaultAttachDetachReconcileSyncPeriod

justinsb · 2017-01-17T01:02:25Z

pkg/model/components/kubecontrollermanager.go

+		} else if options.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration < defaultAttachDetachReconcileSyncPeriod &&
+			options.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration > time.Second {
+
+			glog.Info("k-c-m default-attach-detach-reconcile-sync-period flag is set lower than recommended")


Either use the flag name attach-detach-reconcile-sync-period or the field AttachDetachReconcileSyncPeriod. And probably best to spell out kube-controller-manager, particularly as this is at level Info

justinsb · 2017-01-17T01:09:10Z

You want a test like this also...


func TestBuildFlags(t *testing.T) {
	kcm := &api.KubeControllerManagerConfig{
		AttachDetachReconcileSyncPeriod: &metav1.Duration{ Duration: time.Minute },
	}
	actual, err := nodeup.BuildFlags(kcm)
	if err != nil {
		t.Fatalf("error from BuildFlags: %v", err)
	}
	expected := []string{ "--attach-detach-reconcile-sync-period=60s"}
	if !reflect.DeepEqual(actual, expected) {
		t.Fatalf("unexpected flags.  actual=%v expected=%v", actual, expected)
	}
}

Which will fail because BuildFlags of value type not handled: v1.Duration .AttachDetachReconcileSyncPeriod={1m0s}

justinsb · 2017-01-17T01:11:36Z

And to fix that you need this in build_flags.go

		case metav1.Duration:
			vString := v.Duration.String()
			flag = fmt.Sprintf("--%s=%s", flagName, vString)

justinsb · 2017-01-17T01:17:26Z

Also glog.Infof is really annoying after a kops edit cluster. I think we want at least v(2)

But then it works for me:

  kubeControllerManager:
    allocateNodeCIDRs: true
    attachDetachReconcileSyncPeriod: 1m0s

I haven't tested it all the way through, but I think that should be it....

justinsb · 2017-01-17T01:41:13Z

Correct version of test. Note also the awkward v=0 arg, but that's a separate issue...


func TestBuildFlags(t *testing.T) {
	kcm := &api.KubeControllerManagerConfig{
		AttachDetachReconcileSyncPeriod: &metav1.Duration{Duration: time.Minute},
	}
	actual, err := nodeup.BuildFlags(kcm)
	if err != nil {
		t.Fatalf("error from BuildFlags: %v", err)
	}
	expected := "--attach-detach-reconcile-sync-period=1m0s --v=0"
	if actual != expected {
		t.Fatalf("unexpected flags.  actual=%q expected=%q", actual, expected)
	}
}

chrislovecnm · 2017-01-17T06:09:51Z

@justinsb thanks for the catch ... was driving me nuts.

chrislovecnm · 2017-01-17T06:17:27Z

@justinsb not sure where you want me to put the test, because you seemed to changed:

actual, err := nodeup.BuildFlags(kcm)

to

actual, err := nodeup.buildFlags(kcm)

You changed buildFlags to public.

chrislovecnm · 2017-01-18T19:10:33Z

@justinsb / @kris-nova need another review. Will rebase after e2e

…-detach-reconcile-sync-period

chrislovecnm · 2017-01-18T20:36:40Z

#1528 filed this issue for documenting changes like this.

justinsb

A few more tweaks needed I think

justinsb · 2017-01-18T20:39:14Z

pkg/model/components/kubecontrollermanager.go

+	}
+
+	k8sv148, err := kops.ParseKubernetesVersion("v1.4.8")
+


Simpler is better I think.

justinsb · 2017-01-18T20:39:27Z

pkg/model/components/kubecontrollermanager.go

+		return fmt.Errorf("Unable to parse kubernetesVersion %s", err)
+	}
+
+	k8sv152, _ := kops.ParseKubernetesVersion("v1.5.2")


You need to map the error here

justinsb · 2017-01-18T20:41:07Z

pkg/model/components/kubecontrollermanager.go

+		if options.KubeControllerManager.AttachDetachReconcileSyncPeriod == nil ||
+			options.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration.String() == "0s" {
+
+			glog.V(8).Info("AttachDetachReconcileSyncPeriod is not set; will set to default %v", defaultAttachDetachReconcileSyncPeriod)


Infof. I prefer always to use Infof, because then you never make this mistake, though I think go vet or go lint doesn't like it

justinsb · 2017-01-18T20:43:16Z

pkg/model/components/kubecontrollermanager.go

+
+			glog.Info("KubeControllerManager AttachDetachReconcileSyncPeriod is set lower than recommended")
+
+			// If less than 1sec you get an error.  Controller no worky .. it goes boom.


Is there a k8s issue?

Misworded .. let me reword

justinsb · 2017-01-18T20:45:59Z

pkg/model/components/kubecontrollermanager.go

+		}
+	} else {
+
+		glog.Info("not setting AttachDetachReconcileSyncPeriod, k8s version is too low")


V(2) or higher

justinsb · 2017-01-18T20:47:57Z

pkg/model/components/kubecontrollermanager_test.go

+	}
+
+	if spec.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration != time.Minute {
+		t.Fatalf("k-c-m builder should be set to 1m - %s", spec.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration.String())


AttachDetachReconcileSyncPeriod should be set...

justinsb · 2017-01-18T20:48:16Z

upup/pkg/fi/cloudup/populatecluster_test.go

+	}
+
+	if full.Spec.KubeControllerManager.AttachDetachReconcileSyncPeriod == nil {
+		t.Fatalf("Attache Detach not set correctly")


AttachDetachReconcileSyncPeriod

justinsb · 2017-01-18T20:48:23Z

upup/pkg/fi/cloudup/populatecluster_test.go

+	}
+
+	if full.Spec.KubeControllerManager.AttachDetachReconcileSyncPeriod != nil {
+		t.Fatalf("Attach Detach is not supported in 1.4.7")


AttachDetachReconcileSyncPeriod

justinsb · 2017-01-18T20:49:20Z

pkg/model/components/kubecontrollermanager.go

+
+			// If less than 1sec you get an error.  Controller no worky .. it goes boom.
+		} else if options.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration < time.Second {
+			return fmt.Errorf("Unable to set AttachDetachReconcileSyncPeriod than 1 second")


Let's make it more obviously a validation error: "AttachDetachReconcileSyncPeriod cannot be set to less than 1 second"

justinsb · 2017-01-18T20:49:57Z

pkg/model/components/kubecontrollermanager.go

+		} else if options.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration < defaultAttachDetachReconcileSyncPeriod &&
+			options.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration > time.Second {
+
+			glog.Info("KubeControllerManager AttachDetachReconcileSyncPeriod is set lower than recommended")


Infof(...than recommend: %v", options.KubeControllerManager.AttachDetachReconcileSyncPeriod.Duration)

chrislovecnm · 2017-01-18T20:59:32Z

@justinsb

Simpler is better I think.

Huh??

justinsb

LGTM

chrislovecnm added critical-path P1 WIP: Work In Progress labels Jan 13, 2017

chrislovecnm requested a review from justinsb January 13, 2017 03:21

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

chrislovecnm force-pushed the k-c-m-attachedetachflag branch from 8c84e52 to 60bd9e7 Compare January 16, 2017 20:11

chrislovecnm assigned justinsb and krisnova Jan 16, 2017

krisnova reviewed Jan 17, 2017

View reviewed changes

justinsb reviewed Jan 17, 2017

View reviewed changes

chrislovecnm force-pushed the k-c-m-attachedetachflag branch from 34bc061 to d980445 Compare January 17, 2017 06:18

chrislovecnm changed the title ~~[WIP] Updates for new k-c-m flag~~ Updates for new k-c-m flag Jan 18, 2017

chrislovecnm force-pushed the k-c-m-attachedetachflag branch from f1ed339 to b647126 Compare January 18, 2017 20:04

Updates to add new flag used by Kubernetes Controller manager: attach…

3cabfb2

…-detach-reconcile-sync-period

chrislovecnm force-pushed the k-c-m-attachedetachflag branch from b647126 to 3cabfb2 Compare January 18, 2017 20:35

justinsb reviewed Jan 18, 2017

View reviewed changes

pr review updates

48a4cd1

justinsb approved these changes Jan 18, 2017

View reviewed changes

Tweaking function comments

2f86c3a

chrislovecnm removed the WIP: Work In Progress label Jan 18, 2017

justinsb merged commit a77c1ed into kubernetes:master Jan 19, 2017

andersosthus mentioned this pull request May 5, 2020

Add support for disable-attach-detach-reconcile-sync flag in kube-controller-manager #9067

Closed


		glog.Info("KubeControllerManager AttachDetachReconcileSyncPeriod is set lower than recommended")

		// If less than 1sec you get an error. Controller no worky .. it goes boom.

Updates for new k-c-m flag #1465

Updates for new k-c-m flag #1465

Conversation

chrislovecnm commented Jan 13, 2017 • edited by k8s-reviewable Loading

k8s-ci-robot commented Jan 16, 2017

chrislovecnm commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Jan 17, 2017

justinsb commented Jan 17, 2017

justinsb commented Jan 17, 2017

justinsb commented Jan 17, 2017

chrislovecnm commented Jan 17, 2017

chrislovecnm commented Jan 17, 2017

chrislovecnm commented Jan 18, 2017

chrislovecnm commented Jan 18, 2017

justinsb 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

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

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

chrislovecnm commented Jan 18, 2017

justinsb left a comment

Choose a reason for hiding this comment

chrislovecnm commented Jan 13, 2017 •

edited by k8s-reviewable

Loading