Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Generic controller-manager config #1960

Merged
merged 7 commits into from
Dec 22, 2017
Merged

Generic controller-manager config #1960

merged 7 commits into from
Dec 22, 2017

Conversation

jackfrancis
Copy link
Member

@jackfrancis jackfrancis commented Dec 20, 2017 •
edited
Loading

What this PR does / why we need it: exposes a generic key/val interface for configuring controller-manager, using the configuration keys documented here:

https://kubernetes.io/docs/reference/generated/kube-controller-manager/

API model usage patterns will look like this:

"kubernetesConfig": {
    <...>
    "controllerManagerConfig": {
          "--node-monitor-grace-period": "40s",
          "--pod-eviction-timeout": "5m0s",
          "--route-reconciliation-period": "10s"
    }
    <...>
}

Generic controller-manager config

@jackfrancis jackfrancis self-assigned this Dec 20, 2017
@ghost ghost added the in progress label Dec 20, 2017
@jackfrancis jackfrancis force-pushed the generic-controller-manager branch from c2717ce to e0e53e2 Compare December 20, 2017 23:17
@jackfrancis jackfrancis changed the title WIP: generic controller-manager config Generic controller-manager config Dec 21, 2017
@jackfrancis
Copy link
Member Author

@CecileRobertMichon FYI this PR also separates out kubelet config default handling into a separate file.

@jackfrancis jackfrancis force-pushed the generic-controller-manager branch 2 times, most recently from 2e471d5 to 003487f Compare December 21, 2017 22:45
@jackfrancis jackfrancis force-pushed the generic-controller-manager branch from 003487f to 9a00460 Compare December 22, 2017 17:17
JunSun17
JunSun17 previously approved these changes Dec 22, 2017
View reviewed changes
Copy link
Collaborator

@JunSun17 JunSun17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, some minor questions below and ship it after that.

pkg/acsengine/engine.go
@@ -862,6 +859,28 @@ func (t *TemplateGenerator) getTemplateFuncMap(cs *api.ContainerService) templat
}
return buf.String()
},
"GetControllerManagerConfigKeyVals": func(kc *api.KubernetesConfig) string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see kc is used here. It is ok if you plan to use it in the future and want to place it here for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, will address in a follow-up commit.

pkg/acsengine/defaults-controller-manager.go
"github.com/Azure/acs-engine/pkg/api"
)

func setControllerManagerConfig(cs *api.ContainerService) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since o.KubernetesConfig.ControllerManagerConfig is now exposed to user, it looks we allow user to define other configs beyond the list below (default + static + rbac) but in https://kubernetes.io/docs/reference/generated/kube-controller-manager/ . Do we want to support this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what this statement in the doc update supports:

We consider kubeletConfig and controllerManagerConfig to be generic conveniences that add power/flexibility to cluster deployments. Their usage comes with no operational guarantees! They are manual tuning features that enable low-level configuration of a kubernetes cluster.

pkg/api/vlabs/validate.go Outdated
if a.CtrlMgrNodeMonitorGracePeriod == "" {
return fmt.Errorf("--node-status-update-frequency was set to '%s' but OrchestratorProfile.KubernetesConfig.CtrlMgrNodeMonitorGracePeriod was not set", val)
if a.ControllerManagerConfig != nil {
if a.ControllerManagerConfig["--node-monitor-grace-period"] == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this check and some other checks below since we provide a default value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you're right, addressing in a follow-up commit.

@jackfrancis
1 fix and 2 cleanups
6f52628 
- actually using passed-in *api.KubernetesConfig reference in GetControllerManagerConfigKeyVals()
- removing unnecessary validations for both controller manager and kubelet
@jackfrancis jackfrancis merged commit 39081b8 into Azure:master Dec 22, 2017
@ghost ghost removed the in progress label Dec 22, 2017
@jackfrancis jackfrancis deleted the generic-controller-manager branch December 22, 2017 22:24
@jackfrancis jackfrancis mentioned this pull request Jan 8, 2018
Merged
@jcharlytown jcharlytown mentioned this pull request Feb 13, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JunSun17 JunSun17 JunSun17 left review comments

Assignees

@jackfrancis jackfrancis

Labels
orchestrator/k8s
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jackfrancis @JunSun17