Add loop visualization and parallel execution support into rcorder

[lytboris_gmail.com]

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.

[bcr]

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

123

I think the "in" here is superfluous.

197–198

A line break is needed after a sentence stop.

203

Another line break is needed here.

211

Sentence stop = line break.

214

... and another one here.

224

s/provirer/provider/

[lytboris_gmail.com]

Updated man page as per comments from @bcr.

[bcr]

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

[melifaro]

rcorder.c

643

Typo?

652

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

723

IIRC style(9) requires space after while

725

Could you please use strlen here as well?

739

It’s not bufsize anymore

746

You need to decrease bufsize with each invocation

785

Please do not use static variables.

988

style(9) empty line if no vars

1049

Why not typecast to const filenode *?

1057

Worth declaring in header

1071

why +1?

[0mp]

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

45

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

118

It would be great if we could keep the list sorted

.It Fl g
...
.It Fl k
...
.It Fl p
...
.It Fl s

[lytboris_gmail.com]

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]

Makefile

11 ↗

(On Diff #73472)

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

rcorder.8

208

s/Dq Li/Ql/

211

s/Dq Li/Ql/

218

s/Dq Li/Ql/

223

s/Dq Li/Ql/

225

s/Dq Li/Ql/

[0mp]

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

[melifaro]

LGTM, please see some comments inline.

rcorder.c

702

does it print or return the loop string?

1010

typo?

[lytboris_gmail.com]

Altered Copyright header, s/gen_/generate_/

[rpokala]

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.

[avg]

In D25389#560485, @rpokala 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.

[rpokala]

rcorder.8

110

I think this reads slightly better with a comma:

Generate ordering suitable for parallel startup, placing files that can be

[rpokala]

In D25389#560500, @avg wrote:

In D25389#560485, @rpokala 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! :-)

[avg]

rcorder.8

111

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.

[0mp]

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

[lytboris_gmail.com]

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

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

739

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.

[lytboris_gmail.com]

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?

[melifaro]

LGTM.

[marck]

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

thanks!

[lytboris_gmail.com]

Can anyone merge it into head please? :)

[lytboris_gmail.com]

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

[unitrunker_gmail.com]

Here is /etc/rc patch to take advantage of new rcorder -p option:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=249192