fleetd: process dependencies in [Install] section #1523

kayrus · 2016-03-30T14:47:06Z

resolves #1382

kayrus · 2016-03-30T15:13:23Z

TODO:

write functional test [done]
write unit test [won't implement]

antrik · 2016-03-30T15:44:18Z

systemd/manager.go

@@ -106,6 +106,13 @@ func (m *systemdUnitManager) Load(name string, u unit.UnitFile) error {
 	if err != nil {
 		return err
 	}
+	if u.HasReverseDependencies() {


I think the idea was to try always enabling units on load? This kind of special-casing seems fragile, unnecessary, and confusing...

It is not posible to enable unit when it doesn't have Install section

I see. That being so, how about just making it conditional on the presence of this section? It seems a much clearer and less error-prone distinciton...

What if this section is empty or it has invalid data?

Well, you tell me ;-) I'm assuming if it's empty, systemd will enable it just fine? And if it's invalid, it will surely bomb -- which I'd say is the right thing to do...

kayrus · 2016-03-30T17:18:49Z

Just tested this functionality and noticed few issues. Here is the input data:

wants@.service:

[Service]
ExecStart=/bin/bash -c "while true; do echo wants@%i.service unit file; sleep 1; done"

wanted@.service:

[Unit]
Before=wants@%i.service
BindsTo=wants@%i.service

[Service]
ExecStart=/bin/bash -c "while true; do echo wanted@%i.service unit file; sleep 1; done"

[Install]                                                                                                               
WantedBy=wants@%i.service                                                                                               

[X-Fleet]                                                                                                               
MachineOf=wants@%i.service

Issue 1

fleectl submit wanted@.service wants@.service
fleetctl start wants@1.service

In this case fleetd doesn't process WantedBy of the wanted@.service template. And if we would like to start wanted@1.service automatically, we have to load it first, i.e.:

fleectl submit wanted@.service wants@.service
fleetctl load wanted@1.service
fleetctl start wants@1.service

And here comes Issue 2:

Within this situation fleetd doesn't monitor the wanted@1.service status and it still has loaded state:

fleetctl list-unit-files
UNIT                    HASH    DSTATE          STATE           TARGET
wanted@.service         2797f9a inactive        inactive        -
wanted@1.service        2797f9a loaded          loaded          6ed03b01.../coreos1
wants@.service          038ee83 inactive        inactive        -
wants@1.service         038ee83 launched        launched        6ed03b01.../coreos1

But systemd status is correct:

systemctl status wanted@1.service                                                                      
● wanted@1.service                                                                                                      
   Loaded: loaded (/run/fleet/units/wanted@1.service; enabled-runtime; vendor preset: disabled)                         
   Active: active (running) since Wed 2016-03-30 16:52:54 UTC; 11min ago                                                
 Main PID: 17765 (bash)                                                                                                 
   CGroup: /system.slice/system-wanted.slice/wanted@1.service                                                           
           ├─17765 /bin/bash -c while true; do echo wanted@1.service unit file; sleep 1; done                           
           └─19108 sleep 1                                                                                              

Mar 30 17:03:50 coreos1 bash[17765]: wanted@1.service unit file                                                         
Mar 30 17:03:51 coreos1 bash[17765]: wanted@1.service unit file                                                         
Mar 30 17:03:52 coreos1 bash[17765]: wanted@1.service unit file
Mar 30 17:03:53 coreos1 bash[17765]: wanted@1.service unit file
Mar 30 17:03:54 coreos1 bash[17765]: wanted@1.service unit file
Mar 30 17:03:55 coreos1 bash[17765]: wanted@1.service unit file
Mar 30 17:03:56 coreos1 bash[17765]: wanted@1.service unit file
Mar 30 17:03:57 coreos1 bash[17765]: wanted@1.service unit file
Mar 30 17:03:58 coreos1 bash[17765]: wanted@1.service unit file
Mar 30 17:03:59 coreos1 bash[17765]: wanted@1.service unit file

systemctl list-dependencies --reverse wants@1.service
wants@1.service
● └─wanted@1.service

Should fleet monitor unit status on the systemd side? And what should it do when it will find that desired status doesn't correspond to the real state? Should fleetd change unit's state by itself (not by systemd)?

The answer to the last question could be as follows:

When wants@.service unit's state is being changed, fleetd should get all existing units, parse them, and figure out whether there is an additional dependency. If there is WantedBy and unit is not loaded - just ignore that. If there is RequiredBy and unit is not loaded - we have to load that unit first, and then change wants@.service unit's state.

Looks like this issue has lots pitfalls.

UPD: some fleet specific options (dependencies) were already discussed in #464

kayrus · 2016-03-30T17:23:05Z

@jonboulle comments?

antrik · 2016-03-31T14:51:44Z

@kayrus I don't think your Issue 1 is actually a problem -- as far as I can see, it's perfectly in line with the conversation in #1382 .

As for Issue 2... This does sound like a problem -- but isn't it just the same when using Wants dependencies instead, which apparently already worked in the existing code base?...

kayrus · 2016-04-04T19:37:20Z

@joshix @robszumski could you review documentation part?

jonboulle · 2016-04-05T14:45:27Z

Documentation/unit-files-and-scheduling.md

+MachineOf=my.service
+```
+
+Fleet will load and automatically enable unit above because it contains `[Install]` section. Systemd will create a `my.service.wants/my_discovery.service` symlink to the `my_discovery.service` unit file. And when you start `my.service`, systemd will start `my_discovery.service` independently on `my_discovery.service` desired state defined in fleet. This situation may casue confusion between `fleetctl list-units` and `systemd list-units` outputs. It is recommended to use `fleetctl status my_discovery.service` to get real unit status (`fleetctl status` just passes arguments to `systemctl status` command).


Can you elaborate on "this may cause confusion between fleetctl list-units and systemctl list-units"?

It worries me and I'm starting to wonder if this is a bad idea.

I think we need to specifically define what "enable" means for the user, ie. starts on boot

It doesn't suite fleet behavior. Because fleet stores unit files inside tmpfs and they will not be booted automatically using only systemd routines.

@jonboulle I agree that this is a problem -- but AIUI it's actually not a new problem: according to the discussion in #1382 , systemd was already handling dependencies expressed by "Wants" relationships even without enabling; it's just "WantedBy" that changes... So unless I'm missing something, enabling units doesn't fundamentally change the situation -- it only makes it manifest in another case. (Which was somewhat inconsistent before.)

AIUI, the issue should be mostly mitigated once fleet implements dependencies such as X-Requires etc. -- though I guess it might be worthwhile to look into the inconsistency problem regardless. But I really think this is a separate issue.

kayrus · 2016-04-07T14:47:47Z

Updated tests. Now semaphoreci works.
I've removed failed golang 1.4.3 test in #1535

antrik · 2016-04-07T16:31:54Z

functional/install_test.go

+
+	// Verify that discovery.service unit is stopped
+	timeout, err := util.WaitForState(
+		func() bool { return checkInactiveState() },


No need to wrap it in closure like that -- you can just pass checkInactiveState directly.

joshix · 2016-04-08T06:06:39Z

Documentation/unit-files-and-scheduling.md

@@ -57,6 +57,35 @@ To use instance units, simply create a unit file whose name matches the `<name>@

 When working with instance units, it is strongly recommended that all units be _entirely homogenous_. This means that any unit created as, say, `foo@1.service`, should be created only from the unit named `foo@.service`. This homogeneity will be enforced by the fleet API in future.

+## Definition of the Install Section
+
+Unit files which have `[Install]` section will be automatically enabled by fleet. This means that states of such unit files may not be tracked by fleet. For example you have loaded `my.service` unit file:


s/which have/which have an/
s/states/the states/
s/may not/cannot/
"For example, assume we have loaded this my.service unit file:"

joshix · 2016-04-12T00:14:23Z

Documentation/unit-files-and-scheduling.md

+ExecStart=/bin/bash -c "while true; do echo my.service unit file; sleep 1; done"
+```
+
+and then loaded additional [sidekick][sidekick] discovery unit `my_discovery.service`:


s/additional/an additional/

joshix · 2016-04-12T09:14:36Z

docs LGTM for purpose

antrik · 2016-04-19T11:53:11Z

functional/install_test.go

+		if stdout, stderr, err := cluster.Fleetctl(m0, "--strict-host-key-checking=false", "ssh", machine, "uptime"); err != nil {
+			t.Fatalf("Unable to SSH into fleet machine: \nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err)
+		}
+	}


Why do you need to check this here? We have separate tests for this?...

antrik · 2016-04-19T11:55:31Z

functional/install_test.go

+			return true
+		} else {
+			return false
+		}


You don't need an if here: the comparison already produces a bool...

kayrus · 2016-04-19T17:03:08Z

@antrik updated.

antrik · 2016-04-19T17:25:34Z

Looks good to me now :-)

jonboulle · 2016-05-18T13:24:58Z

Punting this to the next milestone as @kayrus thinks it should be reconsidered (?) but is afk

dongsupark · 2016-07-27T09:17:23Z

This PR looks good to me.

Besides I had to think about a couple of open questions:

Unit states from "fleetctl list-units" do not match with those from "systemctl list-units"
** : That's not a new issue, as addressed by Unit does not automatically start Required units #464. In the future we might need to implement X-Requires to avoid the inconsistency.
We have not merged this one, in order to avoid potential conflicts with other PRs like unit state caches (now obsolete) or gRPC (still pending). Though as far as I can tell, gRPC PR doesn't seem to have anything to do with the Install section.

So I think I'll merge this one, probably some time in August.

dongsupark · 2016-08-08T13:47:10Z

Closed via #1655

kayrus self-assigned this Mar 30, 2016

kayrus added this to the v0.13.0 milestone Mar 30, 2016

kayrus force-pushed the kayrus/install_section branch from 7aaf1db to 4aec26f Compare March 30, 2016 15:16

antrik reviewed Mar 30, 2016
View reviewed changes

kayrus force-pushed the kayrus/install_section branch 2 times, most recently from 64ffe99 to 6753795 Compare April 4, 2016 19:35

jonboulle reviewed Apr 5, 2016
View reviewed changes

kayrus force-pushed the kayrus/install_section branch 2 times, most recently from 0d4e50e to 8ee465e Compare April 7, 2016 14:24

kayrus changed the title ~~[WIP] fleetd: process dependencies in [Install] section~~ fleetd: process dependencies in [Install] section Apr 7, 2016

antrik reviewed Apr 7, 2016
View reviewed changes

kayrus force-pushed the kayrus/install_section branch from 1e4142b to 291afb0 Compare April 7, 2016 17:15

joshix reviewed Apr 8, 2016
View reviewed changes

kayrus force-pushed the kayrus/install_section branch from 291afb0 to b8bab3f Compare April 8, 2016 13:49

kayrus mentioned this pull request Apr 8, 2016

*: fleet removes empty sections from unit files #1539

Open

joshix reviewed Apr 12, 2016
View reviewed changes

kayrus force-pushed the kayrus/install_section branch from b8bab3f to 427913c Compare April 12, 2016 08:22

kayrus added the priority/P2 label Apr 18, 2016

tixxdz added the needs review label Apr 19, 2016

antrik reviewed Apr 19, 2016
View reviewed changes

kayrus force-pushed the kayrus/install_section branch from 427913c to 1d563e5 Compare April 19, 2016 17:02

fleetd: process dependencies in [Install] section

ccfe955

kayrus force-pushed the kayrus/install_section branch from 1d563e5 to ccfe955 Compare April 19, 2016 17:23

jonboulle modified the milestones: v1.0.0, v0.13.0 May 18, 2016

dongsupark force-pushed the master branch from 0338168 to 56b539f Compare May 23, 2016 17:57

dongsupark force-pushed the master branch 2 times, most recently from 14580b0 to 63b20dc Compare June 22, 2016 10:22

dongsupark force-pushed the master branch 3 times, most recently from 23d4b13 to 6fb1256 Compare July 1, 2016 11:07

dongsupark force-pushed the master branch 2 times, most recently from 54409ab to a1a21b8 Compare July 8, 2016 11:38

dongsupark added reviewed/lgtm and removed needs review labels Jul 27, 2016

dongsupark mentioned this pull request Aug 8, 2016

fleetd: process dependencies in [Install] section #1655

Merged

dongsupark closed this Aug 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fleetd: process dependencies in [Install] section #1523

fleetd: process dependencies in [Install] section #1523

kayrus commented Mar 30, 2016

kayrus commented Mar 30, 2016 •

edited by tixxdz

Loading

antrik Mar 30, 2016

kayrus Mar 30, 2016

antrik Mar 30, 2016 via email

kayrus Mar 30, 2016

antrik Mar 30, 2016 via email

kayrus commented Mar 30, 2016

kayrus commented Mar 30, 2016

antrik commented Mar 31, 2016

kayrus commented Apr 4, 2016

jonboulle Apr 5, 2016

jonboulle Apr 5, 2016

robszumski Apr 5, 2016

kayrus Apr 5, 2016

antrik Apr 6, 2016

kayrus commented Apr 7, 2016

antrik Apr 7, 2016

joshix Apr 8, 2016

joshix Apr 12, 2016

joshix commented Apr 12, 2016

antrik Apr 19, 2016

antrik Apr 19, 2016

kayrus commented Apr 19, 2016

antrik commented Apr 19, 2016

jonboulle commented May 18, 2016

dongsupark commented Jul 27, 2016

dongsupark commented Aug 8, 2016

fleetd: process dependencies in [Install] section #1523

fleetd: process dependencies in [Install] section #1523

Conversation

kayrus commented Mar 30, 2016

kayrus commented Mar 30, 2016 • edited by tixxdz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antrik Mar 30, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antrik Mar 30, 2016 via email

Choose a reason for hiding this comment

kayrus commented Mar 30, 2016

kayrus commented Mar 30, 2016

antrik commented Mar 31, 2016

kayrus commented Apr 4, 2016

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

kayrus commented Apr 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshix commented Apr 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kayrus commented Apr 19, 2016

antrik commented Apr 19, 2016

jonboulle commented May 18, 2016

dongsupark commented Jul 27, 2016

dongsupark commented Aug 8, 2016

kayrus commented Mar 30, 2016 •

edited by tixxdz

Loading