Generic cloud-controller-manager config interface #2017
Conversation
ghost
assigned
jackfrancis
force-pushed
the
generic-ccm
branch
from
af2f68e to
0af1456
Compare
|
@JunSun17 Kindly requesting review. :) This is the final generic k8s runtime config to expose! |
JunSun17
left a comment
JunSun17
left a comment
There was a problem hiding this comment.
Looks good to me, just clarify the comment below and good to go.
|
|
||
| // Default cloud-controller-manager config | ||
| defaultCloudControllerManagerConfig := map[string]string{ | ||
| "--node-monitor-grace-period": DefaultKubernetesCtrlMgrNodeMonitorGracePeriod, |
There was a problem hiding this comment.
From the md file, the default is "--route-reconciliation-period", a typo?
There was a problem hiding this comment.
Good catch! Actually, the doc is correct, it's an error in the defaultCloudControllerManagerConfig object! (see the changes to parts/k8s/manifests/kubernetesmaster-cloud-controller-manager.yaml). I'll fix.
CecileRobertMichon
left a comment
CecileRobertMichon
left a comment
There was a problem hiding this comment.
lgtm, added a few comments
| } | ||
| return strings.TrimSuffix(buf.String(), ", ") | ||
| }, | ||
| "GetCloudControllerManagerConfigKeyVals": func(kc *api.KubernetesConfig) string { |
There was a problem hiding this comment.
Is it necessary to pass in the KubernetesConfig here? For distinct agent/master configs it needs to be passed in but since this looks like it's using the orchestratorProfile's kubernetesConfig here you could also use cs.Properties.OrchestratorProfile.KubernetesConfig since the getTemplateFuncMap gets passed in a ContainerService
There was a problem hiding this comment.
I'm actually planning to submit a follow-up PR that generalizes the getter function so that it works on map[string]strings generically.
| }, | ||
| "GetCloudControllerManagerConfigKeyVals": func(kc *api.KubernetesConfig) string { | ||
| cloudControllerManagerConfig := kc.CloudControllerManagerConfig | ||
| // Order by key for consistency |
There was a problem hiding this comment.
if this starts getting repeated in more configs (it's already in GetControllerManagerConfigKeyVals, GetKubeletConfigKeyVals and GetApiServerConfigKeyVals) maybe it's time to extract it to a function
There was a problem hiding this comment.
Oh, yes! What I said above.
| "orchestratorType": "Kubernetes", | ||
| "orchestratorRelease": "1.8", | ||
| "kubernetesConfig": { | ||
| "useCloudControllerManager": true |
There was a problem hiding this comment.
Might be beneficial to put a CloudControllerManagerConfig field here for reference
There was a problem hiding this comment.
I'm not sure I want to encourage someone to use this yet until we spend some time testing various custom configurations that actually work! :)
3 participants
What this PR does / why we need it: exposes a generic key/val interface for configuring cloud-controller-manager, using the configuration keys documented here:
https://kubernetes.io/docs/reference/generated/cloud-controller-manager/
API model usage patterns will look like this: