Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make systemd service dependencies considerably more strict #1417

Merged
merged 1 commit into from
Apr 7, 2015

Conversation

sdodson
Copy link
Member

@sdodson sdodson commented Mar 23, 2015

Before/After enforces ordering and ordering only.
Wants triggers the named dependency to start but will not prevent the service from starting in the event that the dependency has failed. I've only used Requires in the case of the openshift-node->docker dependency because the other services could potentially be located on other hosts or may not even be used/installed like etcd so I believe Wants is more appropriate for those.

@sdodson
Copy link
Member Author

sdodson commented Mar 23, 2015

Similar changes for openshift-sdn in openshift/openshift-sdn#44

@sdodson sdodson changed the title Make service dependencies considerably more strict Make systemd service dependencies considerably more strict Mar 23, 2015
@sdodson
Copy link
Member Author

sdodson commented Mar 24, 2015

@brenton mind taking a look and giving this a merge?

@sdodson
Copy link
Member Author

sdodson commented Mar 27, 2015

@sosiouxme mind taking a look? This follows similar changes made in openshift-sdn. We're perhaps overly descriptive, including services that aren't directly connected to one another, but I was erring on the side of each unit file accurately describing where it falls into the complete system.

@@ -2,6 +2,11 @@
Description=OpenShift Master
Documentation=https://github.com/openshift/origin
After=network.target
After=etcd.service
Wants=etcd.service
Copy link
Contributor

Choose a reason for hiding this comment

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

Pardon my systemd unit file stupidity. What does this do if the etcd.service doesn't exist? (like in the case of use the embedded one)

Copy link
Member Author

Choose a reason for hiding this comment

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

It will log that they're not found and move on unless it's a Requires, in which case it would prevent the service from starting.

Copy link
Member Author

Choose a reason for hiding this comment

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

This does mean that if etcd were installed, but we're configured to use the embedded etcd, it would attempt to start etcd and we'd run into problems binding to the port assuming it's not reconfigured to use another port. But, uhhh, don't install etcd if you're not going to use it?

Copy link
Member

Choose a reason for hiding this comment

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

@brenton I'm only moderately up on unit file semantics but with systemd, load order and dependency are orthogonal concepts... See Requires/Wants vs Before/After at http://www.freedesktop.org/software/systemd/man/systemd.unit.html

I don't feel great about "don't install it if you don't want it" because it's nice to be able to just create an image with everything on it, then only activate the parts you want on a particular host. I guess you could just nuke stuff you don't want, but I wonder if there's a better way to indicate "Service A can do this or rely on Service B, but we don't want to do both"... going to think about it for a minute.

@sosiouxme
Copy link
Member

Discussed, to summarize:

  • If we just do After= everywhere, then assume the user (or installer) will systemctl enable whatever they actually want running, everything works out at boot time (as services pretty much are started by the same boot target).
  • The concern then is only when people are manually stopping/starting services, trying to make sure that if something is enabled and supposed to run before something else, it actually does, but not if it's not enabled. The [Install] section WantedBy seems to be ideal for this purpose if it works, but for some reason this is not recommended... see if/how it works.
  • Since I really don't think we generally want e.g. the master to start etcd unless etcd is enabled (least surprise principle), what we could do if the above is inadequate is have the install add e.g. Wants=etcd.service into /etc/systemd/system/openshift-master.d/etcd.conf (see Example 2. Overriding vendor settings) when it knows the master is depending on remote etcd. The top concern with this would be that the average sysadmin might not know to look for these overrides.

@sdodson
Copy link
Member Author

sdodson commented Mar 27, 2015

I've confirmed the second bullet point.

First, lets assume openshift*, docker, openvswitch, and etcd are installed on the host. All services are disabled and stopped. We're ignoring SDN related stack for now.

We want this to be a node and a node only, so we enable and start openshift-node. Assuming it's configured properly for a master everything is fine, no additional services are started.

Next, we want this to be an all in one with the master on the same host. We re-configure the node to use the local master (outside of the scope of the systemd service definitions) and we enable but do not start openshift-master service. Now whenever we start openshift-node it will start openshift-master due to this symlink created when systemctl enable openshift-master

[root@ose3-master ~]# systemctl enable openshift-master
ln -s '/usr/lib/systemd/system/openshift-master.service' '/etc/systemd/system/multi-user.target.wants/openshift-master.service'
ln -s '/usr/lib/systemd/system/openshift-master.service' '/etc/systemd/system/openshift-node.service.wants/openshift-master.service'

This happens because we've added WantedBy=openshift-node.service to the [Install] section of openshift-master.service

[Install]
WantedBy=multi-user.target
WantedBy=openshift-node.service

I believe bullet point two plus instructions or installer automation enabling etcd when it's intended to be used is sufficient. We'll still have After=etcd.service in the master to ensure ordering.

- After/Before enforce only ordering, they do not trigger dependencies
  to be started.
- To the [Install] section of openshift-master add WantedBy for
  openshift-sdn-master and openshift-node. This means that when you
  `systemctl enable openshift-master` it will cause starting those
  dependencies to also start openshift-master if it's not already
  running.
@sosiouxme
Copy link
Member

+1 :)

@sdodson
Copy link
Member Author

sdodson commented Apr 2, 2015

I think this should be ready for merge. @sosiouxme mind pressing the big button?

@sosiouxme
Copy link
Member

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1492/) (Image: devenv-fedora_1222)

@sosiouxme
Copy link
Member

@sdodson
Copy link
Member Author

sdodson commented Apr 7, 2015

[merge] again, i think I read somewhere that subsequent merge requests don't require ACL checks.

@sosiouxme
Copy link
Member

Alas poor https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1458/

Let's try another [merge] and hope for a better day.

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 32a35f5

openshift-bot pushed a commit that referenced this pull request Apr 7, 2015
@openshift-bot openshift-bot merged commit f175649 into openshift:master Apr 7, 2015
jpeeler added a commit to jpeeler/origin that referenced this pull request Oct 23, 2017
…service-catalog/' changes from aa27078754..dabde2eb85

dabde2eb85 origin build: add origin tooling
b70c076 Reorder class and plan creation; test plan conflict handling (openshift#1459)
4bea012 Use versioned client APIs (openshift#1458)
ff4af30 clean up logic for 410 gone deprovision poll (openshift#1452)
3fddf27 clean up logic and fix message for failed poll (openshift#1451)
40926cd Fix typo from openshift#1354 (openshift#1456)
ff86ef2 Delete removed serviceplans when they have no instances left (openshift#1444)
8411a16 tweak binding setAndUpdateOrphanMitigation function (openshift#1448)
ce28252 Combine apiserver and controller-manager into a single service-catalog image (openshift#1343)
7bbc8ee Check service class / plan before allowing provisioning or plan changes. (openshift#1439)
baf28de Create listers before adding event handlers in controller (openshift#1446)
294157d remove setServiceBindingCondition dependency on controller (openshift#1441)
118a0f7 Fix typo in validation (openshift#1447)
117bfbd clean up error logging (openshift#1443)
dff470f Move "External" around in some resource names/properties (openshift#1354)
0885edb Adding expectedGot function and using it. (openshift#1440)
a7d582e Pretty controller broker (openshift#1442)
c5edfaf Set apimachinery build variables with semver info (openshift#1429)
0e90d82 Add a pretty formatter for ClusterService[Class|Plan] (openshift#1408)
fb874df Remove deprecated basic auth config support (openshift#1431)
f4cd181 Migrate to metav1 methods for manipulating controllerRefs (openshift#1433)
96b286e Make service/plan reference fields on instance spec selectable (openshift#1422)
33f2b04 First example using the pretty context builder. (openshift#1403)
7852917 Stop using corev1.ObjectReference and corev1.LocalObjectReference (openshift#1417)
fcf9480 Add tests for plan updates (openshift#1412)
819332e Add root CAs (openshift#1419)
b49a76a Clean Makefile a little (openshift#1399)
d681da0 Use a separate etcd prefix for each integration test to keep tests isolated (openshift#1415)
314a622 Wire etcd prefix to storage and call complete with options (openshift#1394)
REVERT: aa27078754 origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: dabde2eb859b5e31e97c01a704561fc27e1848b2
jpeeler added a commit to jpeeler/origin that referenced this pull request Oct 24, 2017
…service-catalog/' changes from aa27078754..510060232e

510060232e origin build: add origin tooling
de45e94 v0.1.0 chart changes (openshift#1468)
0bb9982 Modify Makefile to only specify ldflags once (openshift#1471)
5d6afac Fixes openshift#735: Add repo-sync script for charts (openshift#1453)
630f13f fix lingering unversioned client API (openshift#1466)
6f49128 Fix several logging errors (openshift#1464)
2aece61 Delete removed serviceClasses when they have no instances left (openshift#1450)
179d302 Uncommenting UID field after updating to k8s 1.8 (openshift#1457)
b70c076 Reorder class and plan creation; test plan conflict handling (openshift#1459)
4bea012 Use versioned client APIs (openshift#1458)
ff4af30 clean up logic for 410 gone deprovision poll (openshift#1452)
3fddf27 clean up logic and fix message for failed poll (openshift#1451)
40926cd Fix typo from openshift#1354 (openshift#1456)
ff86ef2 Delete removed serviceplans when they have no instances left (openshift#1444)
8411a16 tweak binding setAndUpdateOrphanMitigation function (openshift#1448)
ce28252 Combine apiserver and controller-manager into a single service-catalog image (openshift#1343)
7bbc8ee Check service class / plan before allowing provisioning or plan changes. (openshift#1439)
baf28de Create listers before adding event handlers in controller (openshift#1446)
294157d remove setServiceBindingCondition dependency on controller (openshift#1441)
118a0f7 Fix typo in validation (openshift#1447)
117bfbd clean up error logging (openshift#1443)
dff470f Move "External" around in some resource names/properties (openshift#1354)
0885edb Adding expectedGot function and using it. (openshift#1440)
a7d582e Pretty controller broker (openshift#1442)
c5edfaf Set apimachinery build variables with semver info (openshift#1429)
0e90d82 Add a pretty formatter for ClusterService[Class|Plan] (openshift#1408)
fb874df Remove deprecated basic auth config support (openshift#1431)
f4cd181 Migrate to metav1 methods for manipulating controllerRefs (openshift#1433)
96b286e Make service/plan reference fields on instance spec selectable (openshift#1422)
33f2b04 First example using the pretty context builder. (openshift#1403)
7852917 Stop using corev1.ObjectReference and corev1.LocalObjectReference (openshift#1417)
fcf9480 Add tests for plan updates (openshift#1412)
819332e Add root CAs (openshift#1419)
b49a76a Clean Makefile a little (openshift#1399)
d681da0 Use a separate etcd prefix for each integration test to keep tests isolated (openshift#1415)
314a622 Wire etcd prefix to storage and call complete with options (openshift#1394)
REVERT: aa27078754 origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: 510060232e54eb64b294213bb5d7847e169a2fac
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Feb 1, 2018
…enshift#1417)

* Fix lint errors

* Stop using corev1.ObjectReference and corev1.LocalObjectReference
@sdodson sdodson deleted the service-dependencies branch May 11, 2018 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants