Code review reminder: pkg/update fix and sysrepo config resiliency

[Tim Foster]

Hi there,

Just a quick reminder that I've got two small fixes out for code review
at the moment, and was wondering if anyone had time to take a look?

The webrevs are:

https://cr.opensolaris.org/action/browse/pkg/timf/sysrepo-resilient-config/sysrepo-resilient-config-webrev/

https://cr.opensolaris.org/action/browse/pkg/timf/add-cronjob/add_cronjob-webrev

The original mails to pkg-discuss were:

Subject: Code review: making pkg.sysrepo more resilient in the face of
inaccessible file repos
(sent 02/26)

Subject: Code review: add_cronjob failing after unclean shutdown
(sent 03/14)

cheers,
tim

[Erik Trauschke]

Post by Tim Foster
Hi there,
Just a quick reminder that I've got two small fixes out for code review
at the moment, and was wondering if anyone had time to take a look?
https://cr.opensolaris.org/action/browse/pkg/timf/sysrepo-resilient-config/sysrepo-resilient-config-webrev/

lgtm

Post by Tim Foster
https://cr.opensolaris.org/action/browse/pkg/timf/add-cronjob/add_cronjob-webrev

154: nit: /contains/contain/

you're testing only for the existence of ${CMD} in the whole crontab.
How likely is it that this test might find something the user put in
there (or asked differently, how unique is ${CMD})?

Erik

[Tim Foster]

Post by Erik Trauschke

Post by Tim Foster
https://cr.opensolaris.org/action/browse/pkg/timf/sysrepo-resilient-config/sysrepo-resilient-config-webrev/

lgtm

Thanks for taking a look!

Post by Erik Trauschke

Post by Tim Foster
https://cr.opensolaris.org/action/browse/pkg/timf/add-cronjob/add_cronjob-webrev

154: nit: /contains/contain/

Yep, I'll fix that.

Post by Erik Trauschke
you're testing only for the existence of ${CMD} in the whole crontab.
How likely is it that this test might find something the user put in
there (or asked differently, how unique is ${CMD})?

Sure - I tried to address that in the comment at the top of
add_cronjob(). In this case, $CMD is unique in that it contains the
full SMF FMRI of the service we're looking to add/remove, eg.

"/usr/sbin/svcadm refresh svc:/application/pkg/mirror:default"

It specifically doesn't include the rest of the cron entry (namely the
cron schedule) because that can change as the user modifies SMF properties.

For the consumers of this function, pkg/update and pkg/mirror, this is
enough I think. Yes, the pkg/update service would potentially clash if
we had more than one instance of the service running, since the command
it uses is:

"/usr/lib/update-manager/update-refresh.sh"

but that doesn't seem likely (and I hope we get to nuke pkg/update real
soon now anyway - its use of root's crontab is disturbing..)


Thanks for looking at these webrevs, much appreciated!

cheers,
tim

[Tim Foster]

Hi all,

Sorry for the follow up on this, there was some conversation about this
service this morning (and the breakage in build 17 that this bug fixes)

Post by Erik Trauschke

Post by Tim Foster
https://cr.opensolaris.org/action/browse/pkg/timf/add-cronjob/add_cronjob-webrev

154: nit: /contains/contain/
you're testing only for the existence of ${CMD} in the whole crontab.
How likely is it that this test might find something the user put in
there (or asked differently, how unique is ${CMD})?

in particular, how it should deal with users that have commented out the
crontab entry the service creates. I've a one-line change here that
specifically looks for uncommented entries when deciding to add a new
entry that I'd like to include with this putback:

---
diff --git a/src/svc/pkg5_include.sh b/src/svc/pkg5_include.sh
--- a/src/svc/pkg5_include.sh
+++ b/src/svc/pkg5_include.sh
@@ -152,7 +152,7 @@
$CRONTAB -l > $current_crontab
EXIT=0
# if the crontab doesn't already contain our command, add it
- $GREP -q " $CMD"$ $current_crontab
+ $GREP -q "^[0-9, \*]+ $CMD"$ $current_crontab
if [ $? -ne 0 ]; then
$GREP -v " ${CMD}"$ $current_crontab > $new_crontab
echo "$SCHEDULE $CMD" >> $new_crontab

---

the upshot is, if you've manually commented out the crontab entry, then
by enabling the service, it will always add it back in.

Without this change, if you enable the service, then comment-out the
crontab entry, and stop/start the service, the service will appear to be
online without the cronjob ever actually firing, which seems like a
worse user-experience to me.

I know this risks confusing the user ("Hey, I commented out that cron
entry, how come it keeps coming back!?") but ultimately, I hope that
some work elsewhere will help us from needing to deal with cron at all,
so this will be a short-lived annoyance at worst.

cheers,
tim

[Edward Pilatowicz]

Post by Tim Foster
Hi there,
Just a quick reminder that I've got two small fixes out for code
review at the moment, and was wondering if anyone had time to take a
look?
https://cr.opensolaris.org/action/browse/pkg/timf/sysrepo-resilient-config/sysrepo-resilient-config-webrev/

lgtm.
thanks for fixing this.
ed