run kube controllers in a separate process #6735

deads2k · 2018-01-15T19:54:37Z

This adds a new exec start for our master-controllers service to launch the kube-controller-manager in the same unit. This is a compromise between the desired static pods and the current (and unmaintainable) two processes in a single process using separate leases. This keeps from requiring re-education on how to restart controllers for 3.9, while still allowing separate controller PIDs to go with the separate leases. We'll also get separate health and metrics, but we don't have those yet.

@smarterclayton @derekwaynecarr @eparis @liggitt as we've discussed separately. Also, are we ready for openshift start master to become "best effort" and spawn the kube controller managers for 3.9 or will we go for broke and remove it?
@sdodson This gives us something to talk about. How can I improve it?

Can't merge until openshift/origin#18100

smarterclayton · 2018-01-15T20:01:42Z

roles/openshift_master/templates/docker-cluster/atomic-openshift-master-controllers.service.j2

+  {% if openshift_cloudprovider_kind | default('') != '' -%} -v {{ openshift.common.config_base }}/cloudprovider:{{ openshift.common.config_base}}/cloudprovider {% endif -%} \
+  -v /etc/pki:/etc/pki:ro \
+  {% if l_bind_docker_reg_auth | default(False) %} -v {{ oreg_auth_credentials_path }}:/root/.docker:ro{% endif %}\
+  {{ osm_image }}:${IMAGE_VERSION} start master controllers \


Which part? {{ osm_image }} is part of the jinja template ${IMAGE_VERSION} is an environment variable set in EnvironmentFile

smarterclayton · 2018-01-15T20:02:12Z

roles/openshift_master/templates/native-cluster/atomic-openshift-master-controllers.service.j2

+  --leader-elect \
+  --leader-elect-retry-period=3s \
+  --leader-elect-resource-lock=configmaps \
+  --openshift-config=${CONFIG_FILE}


Interesting - I would have thought the other settings would come from openshift-config as well. Is that not the case?

Interesting - I would have thought the other settings would come from openshift-config as well. Is that not the case?

This file is needed for

recycler template which should be passed as a fully formed template file reference (flag available)

command line flag args which should be passed as options

I added comments here: https://github.com/openshift/origin/pull/18100/files#r161594020

I think we should try to follow kube when kube moves to its config, not double down on our own for those two settings. Then its a feature flag and we're only two patches away from running just like upstream (for good or for ill)

eparis · 2018-01-15T20:08:08Z

I assume this is
/hold
because we are past 3.9 feature freeze?

eparis · 2018-01-15T20:09:07Z

roles/openshift_master/templates/docker-cluster/atomic-openshift-master-controllers.service.j2

@@ -22,12 +22,39 @@ ExecStart=/usr/bin/docker run --rm --privileged --net=host \
  {% if l_bind_docker_reg_auth | default(False) %} -v {{ oreg_auth_credentials_path }}:/root/.docker:ro{% endif %}\
  {{ osm_image }}:${IMAGE_VERSION} start master controllers \
  --config=${CONFIG_FILE} $OPTIONS
+ExecStartPre=-/usr/bin/docker rm -f {{ openshift_service_type}}-master-kube-controllers


@mrunalp @runcom doesn't docker get really angry when you use rm -f ? Or is docker ok with this?

That was usually related to some devmapper stuff, I doubt we have that issue anymore but yeah, Docker at least had an issue with rm -f

eparis · 2018-01-15T20:11:50Z

roles/openshift_master/templates/native-cluster/atomic-openshift-master-controllers.service.j2

@@ -11,6 +11,21 @@ Type=notify
 EnvironmentFile=/etc/sysconfig/{{ openshift_service_type }}-master-controllers
 Environment=GOTRACEBACK=crash
 ExecStart=/usr/bin/openshift start master controllers --config=${CONFIG_FILE} $OPTIONS
+ExecStart=/usr/bin/hyperkube kube-controller-manager \
+  --controllers="*" --controllers=-ttl --controllers=-bootstrapsigner --controllers=-tokencleaner --controllers=-horizontalpodautoscaling --controllers=-serviceaccount-token \
+  --service-account-private-key-file=openshift.local.config/master/serviceaccounts.private.key \


Do things like openshift.local.config not need to be {{ openshift.local.config }} ? Throughout?

Do things like openshift.local.config not need to be {{ openshift.local.config }} ? Throughout?

I don't know. @sdodson ?

It should be --service-account-private-key-file={{ openshift_master_config_dir }}/serviceaccounts.private.key which would evaluate to /etc/origin/master/serviceaccounts.private.key

openshift-ci-robot · 2018-01-15T20:50:00Z

@deads2k: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/tox	`4ea7a9a`	link	`/test tox`
ci/openshift-jenkins/logging	`4ea7a9a`	link	`/test logging`
ci/openshift-jenkins/install	`4ea7a9a`	link	`/test install`
ci/openshift-jenkins/containerized	`4ea7a9a`	link	`/test containerized`
ci/openshift-jenkins/extended_conformance_install_crio	`4ea7a9a`	link	`/test crio`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open 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.

sdodson · 2018-01-16T13:17:13Z

roles/openshift_master/templates/docker-cluster/atomic-openshift-master-controllers.service.j2

+  -v /etc/pki:/etc/pki:ro \
+  {% if l_bind_docker_reg_auth | default(False) %} -v {{ oreg_auth_credentials_path }}:/root/.docker:ro{% endif %}\
+  {{ osm_image }}:${IMAGE_VERSION} start master controllers \
+  --controllers="*" --controllers=-ttl --controllers=-bootstrapsigner --controllers=-tokencleaner --controllers=-horizontalpodautoscaling --controllers=-serviceaccount-token \


If an admin would ever want to modify this we should use an environment variable to define these so they can just edit /etc/sysconfig/origin-master and restart the service.

sdodson · 2018-01-16T13:20:11Z

roles/openshift_master/templates/docker-cluster/atomic-openshift-master-controllers.service.j2

 LimitNOFILE=131072
 LimitCORE=infinity
 WorkingDirectory={{ r_openshift_master_data_dir }}
 SyslogIdentifier={{ openshift_service_type }}-master-controllers
+SyslogIdentifier={{ openshift_service_type }}-master-kube-controllers


This will either blast both syslog identifiers with logs from both processes or it just won't work, need to check the docs.

sdodson · 2018-01-16T13:22:01Z

@smarterclayton @eparis how do you feel about multiple execs in one unit versus composing dependencies in multiple unit files? Will we ever wish to restart one of these processes independently we can't do that with only one unit file.

deads2k · 2018-01-16T13:25:13Z

@smarterclayton @eparis how do you feel about multiple execs in one unit versus composing dependencies in multiple unit files? Will we ever wish to restart one of these processes independently we can't do that with only one unit file.

I don't think we need independent restart capability and since this won't last for very long (probably just one release), I'd rather avoid re-education costs associated with multiple units.

sdodson · 2018-01-16T14:30:09Z

Right, I don't want to change the UX except for debugging scenarios where they'd want to restart a specific set of controllers. I'm fine with this approach as is if this is short lived and of course works. Unless someone says this is a 3.9 blocker I don't think CL team will have time to test this.

deads2k · 2018-01-16T17:47:39Z

For 3.9, we will deal with the "which controllers are running in this process" problem instead of trying to use this to run in separate processes.

run kube controllers in a separate process

4ea7a9a

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 15, 2018

smarterclayton reviewed Jan 15, 2018

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2018

eparis reviewed Jan 15, 2018

View reviewed changes

sdodson reviewed Jan 16, 2018

View reviewed changes

deads2k closed this Jan 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

run kube controllers in a separate process #6735

run kube controllers in a separate process #6735

deads2k commented Jan 15, 2018

smarterclayton Jan 15, 2018

sdodson Jan 16, 2018

smarterclayton Jan 15, 2018

deads2k Jan 15, 2018

eparis commented Jan 15, 2018

eparis Jan 15, 2018

runcom Jan 15, 2018

eparis Jan 15, 2018

deads2k Jan 15, 2018

sdodson Jan 16, 2018

openshift-ci-robot commented Jan 15, 2018 •

edited

Loading

sdodson Jan 16, 2018

sdodson Jan 16, 2018

sdodson commented Jan 16, 2018 •

edited

Loading

deads2k commented Jan 16, 2018

sdodson commented Jan 16, 2018

deads2k commented Jan 16, 2018

run kube controllers in a separate process #6735

run kube controllers in a separate process #6735

Conversation

deads2k commented Jan 15, 2018

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

eparis commented Jan 15, 2018

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

openshift-ci-robot commented Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdodson commented Jan 16, 2018 • edited Loading

deads2k commented Jan 16, 2018

sdodson commented Jan 16, 2018

deads2k commented Jan 16, 2018

openshift-ci-robot commented Jan 15, 2018 •

edited

Loading

sdodson commented Jan 16, 2018 •

edited

Loading