search

coreos / fleet

fleet ties together systemd and etcd into a distributed init system
Apache License 2.0
2.42k stars 302 forks source link

fleetd: process dependencies in [Install] section #1655

Closed dongsupark closed 7 years ago

dongsupark commented 8 years ago

Introduce a section [Install] in unit files, to allow WantedBy in the section to be able to enable the dependent units. This PR is just a rebased version of https://github.com/coreos/fleet/pull/1523, except for minor changes to fix build errors.

Resolves https://github.com/coreos/fleet/issues/1382 Supersedes https://github.com/coreos/fleet/pull/1523

/cc @kayrus

dongsupark commented 8 years ago

Hmm, interesting. TestInstallUnit fails only on semaphoreci, hanging forever on "fleetctl load". OTOH local test runs without any problem. Do not merge this PR until the test issue could get fixed.

kayrus commented 8 years ago

@dongsupark last one doesn't relate to install stuff.

dongsupark commented 8 years ago

@kayrus Right. I was just taking a try-and-error approach. But none of the reverted patches was a culprit. Though I think I just figured out how to reproduce the hanging problem on my local laptop, testing with CoreOS alpha. I'll let you know if I find more about this.

dongsupark commented 8 years ago

To sum up, this PR works fine on CoreOS alpha v1151.0.0 (with systemd v231), while it still fails with CoreOS beta(1122) & stable(1068) (with systemd v229). So the issue of failing functional test was clearly not a fleet issue. Once CoreOS stable got updated to 1151, I would merge this PR.

dongsupark commented 7 years ago

CoreOS stable was updated to 1185.3.0 with systemd v231. So finally this PR can be merged.

However it's still pending due to other test failures. It's blocked by https://github.com/coreos/fleet/pull/1700.

dongsupark commented 7 years ago

All right. As the blocker #1700 was merged, I'll also merge this PR.

jonboulle commented 7 years ago

Bit late to this. Code LGTM, but @dongsupark what do you think about making this configurable, since it can seem a bit unintuitive?

dongsupark commented 7 years ago

@jonboulle I agree that it's unintuitive, but I'm not sure about how to make it configurable for users. AFAICS users anyway need to write own unit files for dependencies, with either Wants or WantedBy. Can you please explain?