Page MenuHomeFreeBSD

Add loop visualization and parallel execution support into rcorder
ClosedPublic

Authored by lytboris_gmail.com on Jun 21 2020, 3:00 PM.

Details

Summary

Requirement violations in rc files are not that rare than it would be expected: REQUIRE and BEFORE keywords allow an easy to make but hard to debug requirement loop. For now rcorder emits an error when it faces the last chain link of this loop but unable to track it as a whole.
This patch introduces two features to make loop eliminating process a lot easier:

  • in normal mode rcorder now prints full loop, not just the last link into stderr;
  • the same loops can be observed by generating GraphViz-compatible execution tree. Nodes on this graph represents providers, dependencies are drawn as edges. BEFORE requirement is drawn as a dashed line and all nodes that were making issues while dependency resolution are clearly marked.

Another feature of this Revision is support for grouping different rc scripts by the moment when all requirements needed for theirs startup are met. This patch does not include corresponding support for the feature in /etc/rc and it will go separately.

Test Plan

The best way to see the changes in action is to make an artificial requirement loop in additional rc file, e.g. add a following requirements:

# PROVIDE: dummytest
# BEFORE: LOGIN
# REQUIRE: sshd

as sshd requires LOGIN this would create a dependency loop that would affect a lot of other scripts. Tracking down the most frequent loop members (see stderr for a number) will lead to the troublemaker.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Thank you for your change, looks like a useful feature.
I've made some comments on the man page. You can also use textproc/igor and "mandoc -Tlint" for further checks.

rcorder.8
117 ↗(On Diff #73434)

I think the "in" here is superfluous.

178 ↗(On Diff #73434)

A line break is needed after a sentence stop.

184 ↗(On Diff #73434)

Another line break is needed here.

192 ↗(On Diff #73434)

Sentence stop = line break.

195 ↗(On Diff #73434)

... and another one here.

205 ↗(On Diff #73434)

s/provirer/provider/

Updated man page as per comments from @bcr.

Great, thanks for those quick fixes. OK form manpages.

rcorder.c
640 ↗(On Diff #73439)

Typo?

649 ↗(On Diff #73439)

This 3-line pattern gets repeated 3 times. Mind moving to an inline func?

720 ↗(On Diff #73439)

IIRC style(9) requires space after while

722 ↗(On Diff #73439)

Could you please use strlen here as well?

736 ↗(On Diff #73439)

It’s not bufsize anymore

743 ↗(On Diff #73439)

You need to decrease bufsize with each invocation

767 ↗(On Diff #73439)

Please do not use static variables.

981 ↗(On Diff #73439)

style(9) empty line if no vars

1042 ↗(On Diff #73439)

Why not typecast to const filenode *?

1050 ↗(On Diff #73439)

Worth declaring in header

1063 ↗(On Diff #73439)

why +1?

0mp requested changes to this revision.Jun 22 2020, 10:05 AM

BTW, I've just cleaned up the manual page a little bit in 362491. Would you mind rebasing ot include those changes?

The only major change there which might be interesting to you is that it uses Ql instead of Dq Li as Li has been deprecated recently.

Cheers!

rcorder.8
44 ↗(On Diff #73439)

Usually, flags without arguments are listed using one Fl macro before any other flags and arguments:

.Op Fl gp
.Op Fl k Ar keep
.Op Fl s Ar skip
112 ↗(On Diff #73439)

It would be great if we could keep the list sorted

.It Fl g
...
.It Fl k
...
.It Fl p
...
.It Fl s
This revision now requires changes to proceed.Jun 22 2020, 10:05 AM
lytboris_gmail.com marked 17 inline comments as done.

Addressed comments by @melifaro:

  • static requirements stack is converted into an additional argument for do_file() and satisfy_req()
  • sequence_cmp() cleanup
  • converted FAKE_PROV_NAME check with an inline function
  • code face lifting to meet style(9)

Manpage is rebased and updated also. Thanks, @0mp.

0mp requested changes to this revision.Jun 22 2020, 6:59 PM
0mp added inline comments.
Makefile
11 ↗(On Diff #73472)

I guess that -O0 and -g could be dropped, right?

rcorder.8
208 ↗(On Diff #73472)

s/Dq Li/Ql/

211 ↗(On Diff #73472)

s/Dq Li/Ql/

218 ↗(On Diff #73472)

s/Dq Li/Ql/

223 ↗(On Diff #73472)

s/Dq Li/Ql/

225 ↗(On Diff #73472)

s/Dq Li/Ql/

This revision now requires changes to proceed.Jun 22 2020, 6:59 PM

the manpage seems fine from my perspective. Thanks a lot!

This revision is now accepted and ready to land.Jun 22 2020, 7:27 PM

LGTM, please see some comments inline.

rcorder.c
702 ↗(On Diff #73473)

does it print or return the loop string?

1010 ↗(On Diff #73473)

typo?

This revision now requires review to proceed.Jun 22 2020, 7:56 PM
lytboris_gmail.com marked 2 inline comments as done.

Altered Copyright header, s/gen_/generate_/

I'm a little late to the party, but thanks for the GraphViz! IIRC, I wrote a little shell script several years ago to pipe through to do the same thing, but I never committed it.

I'm a little late to the party, but thanks for the GraphViz! IIRC, I wrote a little shell script several years ago to pipe through to do the same thing, but I never committed it.

I can see sbin/rcorder/rcorder-visualize.sh committed by @vangyzen more than 3 years ago.
According to the commit message, it was written by Joerg Sonnenberger for NetBSD.

rcorder.8
110 ↗(On Diff #73479)

I think this reads slightly better with a comma:

Generate ordering suitable for parallel startup, placing files that can be
In D25389#560500, @avg wrote:

I'm a little late to the party, but thanks for the GraphViz! IIRC, I wrote a little shell script several years ago to pipe through to do the same thing, but I never committed it.

I can see sbin/rcorder/rcorder-visualize.sh committed by @vangyzen more than 3 years ago.
According to the commit message, it was written by Joerg Sonnenberger for NetBSD.

Ah! Great minds think alike! :-)

rcorder.8
111 ↗(On Diff #73479)

This comment is not about the man page change, but about the actual option.
It's better than nothing, of course.
But I think that for the parallel startup some sort of "interactive" mode would be more useful.
That is, initially rcorder prints all entries that have no requirements and then reads its stdin.
Once one of the earlier entries is echoed back, rcorder prints a list of newly unlocked scripts and goes back to reading again.
And so on until the whole list is printed.

This way the rc infrastructure would be able to start new script(s) as soon as a requirement is satisfied.

Of course, this can be done as a future improvement. If needed.

In D25389#562391, @0mp wrote:

BTW, this DR might be interesting to you: https://reviews.freebsd.org/D3715

Thanks for this link. rc-side implementation issues are exactly the reasons I was not going to include actual parallel startup code into this DR. The good point is even though -p flag can not be used right now, we still benefit from sequence numbers tracking (generations in D3715 terminology) and perform qsort based on them. This shuffles a bit the startup order allowing daemons that are not so lucky with their's names to be run as soon as they could be. Thus, -p key is used just for putting a space instead of a \n.

rcorder.8
111 ↗(On Diff #73479)

This interactive mode can be easily added as generate_ordering() now emits scripts exactly in the way you want them to to appear.
Not sure if it would be better than using wait(1) in rc.

rcorder.c
736 ↗(On Diff #73439)

It is still bufsize as long as I use strlcat(3):

strlcat() appends string src to the end of dst.  It will append at most
dstsize - strlen(dst) - 1 characters.

I was looking into order change and have found very disturbing thing: with this new code securelevel is being started the last. That would not be an issue unless this comment in LOGIN:

# PROVIDE: LOGIN
# REQUIRE: DAEMON

#       This is a dummy dependency to ensure user services such as xdm,
#       inetd, cron and kerberos are started after everything else, in case
#       the administrator has increased the system security level and
#       wants to delay user logins until the system is (almost) fully
#       operational.

securelevel:

# PROVIDE: securelevel
# REQUIRE: adjkerntz ipfw ipfilter pf

Effectively there is no explicit requirement that securelevel should be run before cron, sshd, etc. Thus new code pushes it to the tail.
Am I missing something?

This revision is now accepted and ready to land.Jun 27 2020, 5:54 PM

avg, melifaro, could some of you commit it please?

thanks!

Can anyone merge it into head please? :)

Commit message could be:
A few features are added into rcorder:

  1. Dependency loop logging is enhanced: full chain is being printed instead of the last link competing the loop
  2. -g option is added to generate dependency graph suitable for GraphViz visualization. Loops and other graph generation issues are highlighted automatically.
  3. -p option is added that enables grouping items that can be processed in parallel
This revision was automatically updated to reflect the committed changes.