dbus: add support for new ListUnits* methods #150

kayrus · 2016-04-21T15:14:56Z

…d support)

is a part of this change: endocode/systemd@5b4452a
and this pull request: coreos/fleet#1564

jonboulle · 2016-04-26T12:27:40Z

dbus/methods.go

+// and hence there might be more unit names loaded than actual units behind them.
+func (c *Conn) ListUnitsFiltered(states []string, patterns []string) ([]UnitStatus, error) {
+	result := make([][]interface{}, 0)
+	err := c.sysobj.Call("org.freedesktop.systemd1.Manager.ListUnitsFiltered", 0, states, patterns).Store(&result)


is this documented somewhere? seems they've stopped updating the API page :-( https://www.freedesktop.org/wiki/Software/systemd/dbus/

No, it is not, see systemd/systemd#1042
Also this ListUnitsFiltered implementation is customized. I'm working on systemd patch as I mentioned in PR description.

kayrus · 2016-04-29T14:08:28Z

@jonboulle updated this PR according to systemd/systemd#3142

kayrus · 2016-05-11T15:25:43Z

@jonboulle Updated this PR according to systemd/systemd#3182

kayrus · 2016-05-12T17:18:24Z

@jonboulle both new methods were merged in systemd.

jonboulle · 2016-05-18T13:59:46Z

dbus/methods.go

+// units. Input array should contain exact unit names, but not patterns.
+func (c *Conn) ListUnitsByNames(units []string) ([]UnitStatus, error) {
+	result := make([][]interface{}, 0)
+	err := c.sysobj.Call("org.freedesktop.systemd1.Manager.ListUnitsByNames", 0, units).Store(&result)


This function is 99% the same as ListUnits, so can you split out the common code, something like

type storeFunc func(retvalues ...interface{}) error

func (c *Conn) listUnitsInternal(f storeFunc) ([]UnitStatus, error) { result := make([][]interface{}, 0) err := f(&result) if err != nil { ... // rest of current func here return status, nil }

func (c *Conn) ListUnitsByNames(units []string) ([]UnitStatus, error) { return c.listUnitsInternal( c.sysobj.Call("org.freedesktop.systemd1.Manager.ListUnitsByNames", 0, units).Store ) }

func (c *Conn) ListUnitsByPatterns(states []string, patterns []string) ([]UnitStatus, error) { return c.listUnitsInternal( c.sysobj.Call("org.freedesktop.systemd1.Manager.ListUnitsByPatterns", 0, states, patterns).Store ) }

func (c *Conn) ListUnits(units []string) ([]UnitStatus, error) { return c.listUnitsInternal( c.sysobj.Call("org.freedesktop.systemd1.Manager.ListUnits", 0).Store ) }

jonboulle · 2016-05-18T14:24:24Z

LGTM in principle, but:

style nit (for common code)
any chance of some some kind of testing? I realise it'll be difficult because this hasn't made it into a systemd release yet I guess..?

kayrus · 2016-05-24T12:12:09Z

@jonboulle fixed PR following your suggestions. I'll provide benchmark results inside fleet PR: coreos/fleet#1564

lucab · 2016-05-26T14:43:38Z

Regarding previous review, code has been refactored but it is still not covered by any tests (is fleet blocked by this? Can you add them here or later?). Nothing else to add from my side.

kayrus · 2016-05-26T15:20:24Z

@lucab does switching from ListUnits to ListUnitsByNames in methods_test.go sufficient?

lucab · 2016-05-30T22:07:46Z

@kayrus I'm not sure if it is doable, but I would try to pick some well-known unit name/pattern (ie. existing on most systems) and try to test if the filtered listing succeeds and the result is a subset of the full set you get via ListUnits.

kayrus · 2016-05-31T14:33:30Z

@lucab done

implemented new dbus methods: * ListUnitsFiltered * ListUnitsByPatterns * ListUnitsByNames * ListUnitFilesByPatterns refactored legacy ListUnits and ListUnitFiles functions

jonboulle · 2016-05-31T15:26:06Z

dbus/methods_test.go

+		}
+	}
+
+	if unit == nil {


Why should it be found in the list? this is counterintuitive the way it's written

@jonboulle take a look on this comment https://github.com/coreos/go-systemd/pull/150/files#diff-b5b8384be79564335a3b50bcfed63e91R293
systemd should return unit states for non-existing units.

Oh... confusing

On Tue, May 31, 2016 at 5:34 PM, kayrus [email protected] wrote:

In dbus/methods_test.go
#150 (comment):

t.Fatalf("%s unit not found in list", target1)

}

if unit.ActiveState != "active" {

t.Fatalf("%s unit should be active but it is %s", target1, unit.ActiveState)

}

unit = nil

for _, u := range units {

if u.Name == target2 {

unit = &u

break

}

}

if unit == nil {

@jonboulle https://github.com/jonboulle take a look on this comment
https://github.com/coreos/go-systemd/pull/150/files#diff-b5b8384be79564335a3b50bcfed63e91R293
systemd should return unit states for non-existing units.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/coreos/go-systemd/pull/150/files/124d391a9338d39c19cd55de93b70a4e7ed649f4#r65206518,
or mute the thread
https://github.com/notifications/unsubscribe/ACewNzMPezWMqZoW3zMavEVcKRxk5tcbks5qHFUSgaJpZM4IMyF5
.

@jonboulle yep I confirm it's a bit confusing, but that's how systemd works actually please check this comment systemd/systemd#3182 (comment) so basically the implementation doesn't bother it's just gives you back what you have requested setting up the right state of course.

Now this test can be updated to be more readable:
just create a target1 and target2 object which has {unit_name, activestate, a bolean found or not} , do the whole check inside the same loop, and check after the loop if the units have been found or not. The doc of ListUnitsByNames() clearly states that it may return results for even non-existent units which is good.

kayrus · 2016-06-02T10:07:49Z

@lucab ping

tixxdz · 2016-06-02T14:46:05Z

dbus/methods_test.go

+			break
+		}
+	}
+


same thing with previous test ? have one loop maybe ?

tixxdz · 2016-06-02T15:03:47Z

@jonboulle

any chance of some some kind of testing? I realise it'll be difficult because this hasn't made it into a systemd release yet I guess..?

Not sure if it covers what you have in mind, but that change at systemd level was covered inside systemctl by a fallback to the old method if the new provided method is not available. I think @kayrus did the right thing the check also fallbacks in fleet here:
https://github.com/coreos/fleet/pull/1564/files#diff-561bbb797eb2f941c1384271efe28c0bR208

However to not have test failures in go-systemd, so inside newly added tests of go-systemd when you call the new methods, check the error code and if the method is not available do not fatal ? warning or something ? not sure if this make sense, it depends on how often go-systemd is updated, in all case systemd was released with these new changes.

Thanks!

tixxdz · 2016-06-02T15:08:00Z

lgtm overall, just small code cleanups, thanks!

lucab · 2016-06-02T19:24:08Z

@kayrus sorry for the delay. Except for @tixxdz comments above, I think it should be ready to go.

* Added getUnitStatus and getUnitFile functions for test * Added t.Skip for methods which are not available * Increased verbosity to show skipped tests

kayrus · 2016-06-07T07:57:39Z

@lucab @jonboulle done

lucab · 2016-06-07T09:24:15Z

LGTM

kayrus · 2016-06-07T14:36:37Z

@lucab will you make some kind of tag?

lucab · 2016-06-07T14:54:01Z

@kayrus yes, v9 is pending due to a regression in sdjournal.

kayrus · 2016-06-07T15:17:04Z

@lucab please ping me when you'l fix that.

lucab · 2016-06-07T16:12:05Z

Tagged v9 - https://github.com/coreos/go-systemd/releases/tag/v9

kayrus mentioned this pull request Apr 21, 2016

fleetd: get filtered list of units instead of all units coreos/fleet#1564

Merged

jonboulle reviewed Apr 26, 2016
View reviewed changes

kayrus force-pushed the kayrus/ListUnitsFiltered branch 2 times, most recently from ff35f30 to 74acfae Compare April 29, 2016 14:04

kayrus changed the title ~~[WIP] dbus: get filtered list of units (implemented ListUnitsFiltered metho…~~ dbus: get filtered list of units (implemented ListUnitsFiltered metho… Apr 29, 2016

kayrus mentioned this pull request May 3, 2016

[WIP] dbus: implemented GetUnitsStates method #155

Closed

jonboulle added the dependency/external label May 4, 2016

kayrus force-pushed the kayrus/ListUnitsFiltered branch from 5f70197 to c280fd6 Compare May 11, 2016 15:54

jonboulle reviewed May 18, 2016
View reviewed changes

jonboulle added needs/rework and removed dependency/external labels May 18, 2016

kayrus force-pushed the kayrus/ListUnitsFiltered branch 2 times, most recently from ac30e13 to 4207225 Compare May 24, 2016 11:49

kayrus changed the title ~~dbus: get filtered list of units (implemented ListUnitsFiltered metho…~~ dbus: added new dbus methods May 24, 2016

kayrus force-pushed the kayrus/ListUnitsFiltered branch 3 times, most recently from 80168f1 to 6c718b3 Compare May 24, 2016 12:49

jonboulle added needs/review and removed needs/rework labels May 26, 2016

jonboulle assigned lucab May 26, 2016

kayrus force-pushed the kayrus/ListUnitsFiltered branch 2 times, most recently from e523618 to d3fb628 Compare May 31, 2016 12:40

kayrus force-pushed the kayrus/ListUnitsFiltered branch 2 times, most recently from 1107669 to b9d28d2 Compare May 31, 2016 14:33

dbus: added new dbus methods

124d391

implemented new dbus methods: * ListUnitsFiltered * ListUnitsByPatterns * ListUnitsByNames * ListUnitFilesByPatterns refactored legacy ListUnits and ListUnitFiles functions

kayrus force-pushed the kayrus/ListUnitsFiltered branch from b9d28d2 to 124d391 Compare May 31, 2016 14:40

jonboulle reviewed May 31, 2016
View reviewed changes

tixxdz reviewed Jun 2, 2016
View reviewed changes

dbus/methods_test.go

break

}

}

Copy link

tixxdz Jun 2, 2016 •

edited

Loading

Choose a reason for hiding this comment

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

same thing with previous test ? have one loop maybe ?

lucab added needs/rework and removed needs/review labels Jun 3, 2016

test: optimized dbus tests

c498a32

* Added getUnitStatus and getUnitFile functions for test * Added t.Skip for methods which are not available * Increased verbosity to show skipped tests

lucab added needs/review and removed needs/rework labels Jun 7, 2016

lucab added reviewed/lgtm and removed needs/review labels Jun 7, 2016

lucab changed the title ~~dbus: added new dbus methods~~ dbus: add support for new ListUnits* methods Jun 7, 2016

lucab merged commit f193a6f into coreos:master Jun 7, 2016

lucab added this to the v9 milestone Jun 7, 2016

dbus: add support for new ListUnits* methods #150

dbus: add support for new ListUnits* methods #150

Uh oh!

Conversation

kayrus commented Apr 21, 2016

Uh oh!

jonboulle Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

kayrus Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

kayrus commented Apr 29, 2016

Uh oh!

kayrus commented May 11, 2016

Uh oh!

kayrus commented May 12, 2016

Uh oh!

jonboulle May 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonboulle commented May 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kayrus commented May 24, 2016

Uh oh!

lucab commented May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kayrus commented May 26, 2016

Uh oh!

lucab commented May 30, 2016

Uh oh!

kayrus commented May 31, 2016

Uh oh!

jonboulle May 31, 2016

Choose a reason for hiding this comment

Uh oh!

kayrus May 31, 2016

Choose a reason for hiding this comment

Uh oh!

jonboulle May 31, 2016

Choose a reason for hiding this comment

Uh oh!

tixxdz Jun 2, 2016

Choose a reason for hiding this comment

Uh oh!

kayrus commented Jun 2, 2016

Uh oh!

tixxdz Jun 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tixxdz commented Jun 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tixxdz commented Jun 2, 2016

Uh oh!

lucab commented Jun 2, 2016

Uh oh!

kayrus commented Jun 7, 2016

Uh oh!

lucab commented Jun 7, 2016

Uh oh!

kayrus commented Jun 7, 2016

Uh oh!

lucab commented Jun 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kayrus commented Jun 7, 2016

Uh oh!

lucab commented Jun 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jonboulle May 18, 2016 •

edited

Loading

jonboulle commented May 18, 2016 •

edited

Loading

lucab commented May 26, 2016 •

edited

Loading

tixxdz Jun 2, 2016 •

edited

Loading

tixxdz commented Jun 2, 2016 •

edited

Loading

lucab commented Jun 7, 2016 •

edited

Loading