Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6720005:
enter_chroot: introduce a sync process that synchronizes given files between chroot and host (Closed)
|
Created:
9 years, 8 months ago by zbehan Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Visibility:
Public. |
Descriptionenter_chroot: introduce a sync process that synchronizes given files between chroot and host
Currently used for resolv.conf and hosts, because these files can
change during the lifetime of a chroot, for example on computers
with more dynamic network (laptops).
While this creates a persistent process in the background for the
sole purpose of syncing files, the performance impact is negligible.
BUG=12316
TEST=below
1) enter_chroot once+quit, many times + quit, verify correct exit
behaviour
2) enter_chroot, modify host resolv.conf, see chroot being updated
Change-Id: I26573570c027acc2c214a00838a6f982a7585b13
R=robotboy@chromium.org,dparker@chromium.org,sosa@chromium.org
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=e28239d
Patch Set 1 #
Messages
Total messages: 10 (0 generated)
How much impact will this have with multiple chroots open in different environments? I typically have ~6 chroots open at a given point on my machine (although most are idle) - will the impact still be low? (it looks like it at a glance but wanted to make sure. especially if this is being written mainly for the laptop use case which usually has less CPU available than a workstation) Maybe an option to adjust the poll period? For files like this which shouldn't change often 10 seconds is probably quicker than needed. On Sat, Apr 2, 2011 at 1:33 AM, <zbehan@chromium.org> wrote: > Reviewers: robotboy, Dave Parker, sosa, > > Description: > enter_chroot: introduce a sync process that synchronizes given files > between > chroot and host > > Currently used for resolv.conf and hosts, because these files can > change during the lifetime of a chroot, for example on computers > with more dynamic network (laptops). > > While this creates a persistent process in the background for the > sole purpose of syncing files, the performance impact is negligible. > > BUG=12316 > TEST=below > 1) enter_chroot once+quit, many times + quit, verify correct exit > behaviour > 2) enter_chroot, modify host resolv.conf, see chroot being updated > > Change-Id: I26573570c027acc2c214a00838a6f982a7585b13 > > R=robotboy@chromium.org,dparker@chromium.org,sosa@chromium.org > > > Please review this at http://codereview.chromium.org/6720005/ > > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master > > Affected files: > M enter_chroot.sh > > > Index: enter_chroot.sh > diff --git a/enter_chroot.sh b/enter_chroot.sh > index > bca9c68866b365e58899af344589004058e88264..5c7f07b52a14043bf0d174966fed85f3a69b2651 > 100755 > --- a/enter_chroot.sh > +++ b/enter_chroot.sh > @@ -129,6 +129,7 @@ SAVED_AUTOMOUNT_PREF_FILE="/tmp/.automount_pref" > sudo chmod 0777 "$FLAGS_chroot/var/lock" > > LOCKFILE="$FLAGS_chroot/var/lock/enter_chroot" > +SYNCERPIDFILE="${FLAGS_chroot}/var/tmp/enter_chroot_sync.pid" > > > function ensure_mounted { > @@ -153,12 +154,46 @@ function ensure_mounted { > fi > } > > +function env_sync_proc { > + # This function runs and performs periodic updates to the chroot env, if > + # necessary. > + > + local poll_interval=10 > + local sync_files="etc/resolv.conf etc/hosts" > + > + # Make sure the synced files are writable by normal user, so that we > + # don't have to sudo inside the loop. > + for file in ${sync_files}; do > + sudo chown ${USER} ${FLAGS_chroot}/${file} 1>&2 > + done > + > + while true; do > + # Sync files > + for file in ${sync_files}; do > + if ! cmp /${file} ${FLAGS_chroot}/${file} &> /dev/null; then > + cp -f /${file} ${FLAGS_chroot}/${file} > + fi > + done > + > + sleep ${poll_interval} > + done > +} > + > function setup_env { > # Validate sudo timestamp before entering the critical section so that we > # don't stall for a password while we have the lockfile. > # Don't use sudo -v since that has issues on machines w/ no password. > sudo echo "" > /dev/null > > + # Syncer proc, but only the first time > + if ! [ -f "${SYNCERPIDFILE}" ] || \ > + ! [ -d /proc/$(cat "${SYNCERPIDFILE}") ]; then > + debug "Starting sync process" > + env_sync_proc & > + echo $! > "${SYNCERPIDFILE}" > + disown $! > + fi > + > ( > flock 200 > echo $$ >> "$LOCKFILE" > @@ -283,6 +318,9 @@ function teardown_env { > debug "At least one other pid is running in the chroot, so not" > debug "tearing down env." > else > + debug "Stopping syncer process" > + kill $(cat "${SYNCERPIDFILE}") > + > MOUNTED_PATH=$(readlink -f "$FLAGS_chroot") > debug "Unmounting chroot environment." > # sort the list of mounts in reverse order, to ensure umount of > > >
The performance impact is really unmeasurable. Most of the time is spent comparing the files, which are in the vincity of 100 bytes in size and kept in caches because they're constantly accessed by most network-related processes. In the rare cases where host files have changed, another unmeasurable overhead is added to copy the files. Honestly, if there should be any concerns, i wouldn't expect them to be performance related. Adding an option for the poll interval, well, could be, but I don't want to overcomplicate this. Less user visible options = good, IMO. On Sat, Apr 2, 2011 at 11:01 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > How much impact will this have with multiple chroots open in different > environments? I typically have ~6 chroots open at a given point on my > machine (although most are idle) - will the impact still be low? (it looks > like it at a glance but wanted to make sure. especially if this is being > written mainly for the laptop use case which usually has less CPU available > than a workstation) > > Maybe an option to adjust the poll period? For files like this which > shouldn't change often 10 seconds is probably quicker than needed. > > > On Sat, Apr 2, 2011 at 1:33 AM, <zbehan@chromium.org> wrote: > >> Reviewers: robotboy, Dave Parker, sosa, >> >> Description: >> enter_chroot: introduce a sync process that synchronizes given files >> between >> chroot and host >> >> Currently used for resolv.conf and hosts, because these files can >> change during the lifetime of a chroot, for example on computers >> with more dynamic network (laptops). >> >> While this creates a persistent process in the background for the >> sole purpose of syncing files, the performance impact is negligible. >> >> BUG=12316 >> TEST=below >> 1) enter_chroot once+quit, many times + quit, verify correct exit >> behaviour >> 2) enter_chroot, modify host resolv.conf, see chroot being updated >> >> Change-Id: I26573570c027acc2c214a00838a6f982a7585b13 >> >> R=robotboy@chromium.org,dparker@chromium.org,sosa@chromium.org >> >> >> Please review this at http://codereview.chromium.org/6720005/ >> >> SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master >> >> Affected files: >> M enter_chroot.sh >> >> >> Index: enter_chroot.sh >> diff --git a/enter_chroot.sh b/enter_chroot.sh >> index >> bca9c68866b365e58899af344589004058e88264..5c7f07b52a14043bf0d174966fed85f3a69b2651 >> 100755 >> --- a/enter_chroot.sh >> +++ b/enter_chroot.sh >> @@ -129,6 +129,7 @@ SAVED_AUTOMOUNT_PREF_FILE="/tmp/.automount_pref" >> sudo chmod 0777 "$FLAGS_chroot/var/lock" >> >> LOCKFILE="$FLAGS_chroot/var/lock/enter_chroot" >> +SYNCERPIDFILE="${FLAGS_chroot}/var/tmp/enter_chroot_sync.pid" >> >> >> function ensure_mounted { >> @@ -153,12 +154,46 @@ function ensure_mounted { >> fi >> } >> >> +function env_sync_proc { >> + # This function runs and performs periodic updates to the chroot env, >> if >> + # necessary. >> + >> + local poll_interval=10 >> + local sync_files="etc/resolv.conf etc/hosts" >> + >> + # Make sure the synced files are writable by normal user, so that we >> + # don't have to sudo inside the loop. >> + for file in ${sync_files}; do >> + sudo chown ${USER} ${FLAGS_chroot}/${file} 1>&2 >> + done >> + >> + while true; do >> + # Sync files >> + for file in ${sync_files}; do >> + if ! cmp /${file} ${FLAGS_chroot}/${file} &> /dev/null; then >> + cp -f /${file} ${FLAGS_chroot}/${file} >> + fi >> + done >> + >> + sleep ${poll_interval} >> + done >> +} >> + >> function setup_env { >> # Validate sudo timestamp before entering the critical section so that >> we >> # don't stall for a password while we have the lockfile. >> # Don't use sudo -v since that has issues on machines w/ no password. >> sudo echo "" > /dev/null >> >> + # Syncer proc, but only the first time >> + if ! [ -f "${SYNCERPIDFILE}" ] || \ >> + ! [ -d /proc/$(cat "${SYNCERPIDFILE}") ]; then >> + debug "Starting sync process" >> + env_sync_proc & >> + echo $! > "${SYNCERPIDFILE}" >> + disown $! >> + fi >> + >> ( >> flock 200 >> echo $$ >> "$LOCKFILE" >> @@ -283,6 +318,9 @@ function teardown_env { >> debug "At least one other pid is running in the chroot, so not" >> debug "tearing down env." >> else >> + debug "Stopping syncer process" >> + kill $(cat "${SYNCERPIDFILE}") >> + >> MOUNTED_PATH=$(readlink -f "$FLAGS_chroot") >> debug "Unmounting chroot environment." >> # sort the list of mounts in reverse order, to ensure umount of >> >> >> >
Ok good points. Just was a bit wary of how this might scale. I t hink personally I'd still prefer an option (or at least a const/define type setting at the top) but no actual objections on the CL as is. On Sat, Apr 2, 2011 at 6:50 PM, Zdenek Behan <zbehan@chromium.org> wrote: > The performance impact is really unmeasurable. Most of the time is spent > comparing the files, which are in the vincity of 100 bytes in size and kept > in caches because they're constantly accessed by most network-related > processes. In the rare cases where host files have changed, another > unmeasurable overhead is added to copy the files. > > Honestly, if there should be any concerns, i wouldn't expect them to be > performance related. > > Adding an option for the poll interval, well, could be, but I don't want to > overcomplicate this. Less user visible options = good, IMO. > > On Sat, Apr 2, 2011 at 11:01 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > >> How much impact will this have with multiple chroots open in different >> environments? I typically have ~6 chroots open at a given point on my >> machine (although most are idle) - will the impact still be low? (it looks >> like it at a glance but wanted to make sure. especially if this is being >> written mainly for the laptop use case which usually has less CPU available >> than a workstation) >> >> Maybe an option to adjust the poll period? For files like this which >> shouldn't change often 10 seconds is probably quicker than needed. >> >> >> On Sat, Apr 2, 2011 at 1:33 AM, <zbehan@chromium.org> wrote: >> >>> Reviewers: robotboy, Dave Parker, sosa, >>> >>> Description: >>> enter_chroot: introduce a sync process that synchronizes given files >>> between >>> chroot and host >>> >>> Currently used for resolv.conf and hosts, because these files can >>> change during the lifetime of a chroot, for example on computers >>> with more dynamic network (laptops). >>> >>> While this creates a persistent process in the background for the >>> sole purpose of syncing files, the performance impact is negligible. >>> >>> BUG=12316 >>> TEST=below >>> 1) enter_chroot once+quit, many times + quit, verify correct exit >>> behaviour >>> 2) enter_chroot, modify host resolv.conf, see chroot being updated >>> >>> Change-Id: I26573570c027acc2c214a00838a6f982a7585b13 >>> >>> R=robotboy@chromium.org,dparker@chromium.org,sosa@chromium.org >>> >>> >>> Please review this at http://codereview.chromium.org/6720005/ >>> >>> SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master >>> >>> Affected files: >>> M enter_chroot.sh >>> >>> >>> Index: enter_chroot.sh >>> diff --git a/enter_chroot.sh b/enter_chroot.sh >>> index >>> bca9c68866b365e58899af344589004058e88264..5c7f07b52a14043bf0d174966fed85f3a69b2651 >>> 100755 >>> --- a/enter_chroot.sh >>> +++ b/enter_chroot.sh >>> @@ -129,6 +129,7 @@ SAVED_AUTOMOUNT_PREF_FILE="/tmp/.automount_pref" >>> sudo chmod 0777 "$FLAGS_chroot/var/lock" >>> >>> LOCKFILE="$FLAGS_chroot/var/lock/enter_chroot" >>> +SYNCERPIDFILE="${FLAGS_chroot}/var/tmp/enter_chroot_sync.pid" >>> >>> >>> function ensure_mounted { >>> @@ -153,12 +154,46 @@ function ensure_mounted { >>> fi >>> } >>> >>> +function env_sync_proc { >>> + # This function runs and performs periodic updates to the chroot env, >>> if >>> + # necessary. >>> + >>> + local poll_interval=10 >>> + local sync_files="etc/resolv.conf etc/hosts" >>> + >>> + # Make sure the synced files are writable by normal user, so that we >>> + # don't have to sudo inside the loop. >>> + for file in ${sync_files}; do >>> + sudo chown ${USER} ${FLAGS_chroot}/${file} 1>&2 >>> + done >>> + >>> + while true; do >>> + # Sync files >>> + for file in ${sync_files}; do >>> + if ! cmp /${file} ${FLAGS_chroot}/${file} &> /dev/null; then >>> + cp -f /${file} ${FLAGS_chroot}/${file} >>> + fi >>> + done >>> + >>> + sleep ${poll_interval} >>> + done >>> +} >>> + >>> function setup_env { >>> # Validate sudo timestamp before entering the critical section so that >>> we >>> # don't stall for a password while we have the lockfile. >>> # Don't use sudo -v since that has issues on machines w/ no password. >>> sudo echo "" > /dev/null >>> >>> + # Syncer proc, but only the first time >>> + if ! [ -f "${SYNCERPIDFILE}" ] || \ >>> + ! [ -d /proc/$(cat "${SYNCERPIDFILE}") ]; then >>> + debug "Starting sync process" >>> + env_sync_proc & >>> + echo $! > "${SYNCERPIDFILE}" >>> + disown $! >>> + fi >>> + >>> ( >>> flock 200 >>> echo $$ >> "$LOCKFILE" >>> @@ -283,6 +318,9 @@ function teardown_env { >>> debug "At least one other pid is running in the chroot, so not" >>> debug "tearing down env." >>> else >>> + debug "Stopping syncer process" >>> + kill $(cat "${SYNCERPIDFILE}") >>> + >>> MOUNTED_PATH=$(readlink -f "$FLAGS_chroot") >>> debug "Unmounting chroot environment." >>> # sort the list of mounts in reverse order, to ensure umount of >>> >>> >>> >> >
Can move the current const higher, but I prefered to keep it locally with the code, at the top of the polling function. On Mon, Apr 4, 2011 at 6:52 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > Ok good points. Just was a bit wary of how this might scale. > > I t hink personally I'd still prefer an option (or at least a const/define > type setting at the top) but no actual objections on the CL as is. > > On Sat, Apr 2, 2011 at 6:50 PM, Zdenek Behan <zbehan@chromium.org> wrote: > >> The performance impact is really unmeasurable. Most of the time is spent >> comparing the files, which are in the vincity of 100 bytes in size and kept >> in caches because they're constantly accessed by most network-related >> processes. In the rare cases where host files have changed, another >> unmeasurable overhead is added to copy the files. >> >> Honestly, if there should be any concerns, i wouldn't expect them to be >> performance related. >> >> Adding an option for the poll interval, well, could be, but I don't want >> to overcomplicate this. Less user visible options = good, IMO. >> >> On Sat, Apr 2, 2011 at 11:01 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: >> >>> How much impact will this have with multiple chroots open in different >>> environments? I typically have ~6 chroots open at a given point on my >>> machine (although most are idle) - will the impact still be low? (it looks >>> like it at a glance but wanted to make sure. especially if this is being >>> written mainly for the laptop use case which usually has less CPU available >>> than a workstation) >>> >>> Maybe an option to adjust the poll period? For files like this which >>> shouldn't change often 10 seconds is probably quicker than needed. >>> >>> >>> On Sat, Apr 2, 2011 at 1:33 AM, <zbehan@chromium.org> wrote: >>> >>>> Reviewers: robotboy, Dave Parker, sosa, >>>> >>>> Description: >>>> enter_chroot: introduce a sync process that synchronizes given files >>>> between >>>> chroot and host >>>> >>>> Currently used for resolv.conf and hosts, because these files can >>>> change during the lifetime of a chroot, for example on computers >>>> with more dynamic network (laptops). >>>> >>>> While this creates a persistent process in the background for the >>>> sole purpose of syncing files, the performance impact is negligible. >>>> >>>> BUG=12316 >>>> TEST=below >>>> 1) enter_chroot once+quit, many times + quit, verify correct exit >>>> behaviour >>>> 2) enter_chroot, modify host resolv.conf, see chroot being updated >>>> >>>> Change-Id: I26573570c027acc2c214a00838a6f982a7585b13 >>>> >>>> R=robotboy@chromium.org,dparker@chromium.org,sosa@chromium.org >>>> >>>> >>>> Please review this at http://codereview.chromium.org/6720005/ >>>> >>>> SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master >>>> >>>> Affected files: >>>> M enter_chroot.sh >>>> >>>> >>>> Index: enter_chroot.sh >>>> diff --git a/enter_chroot.sh b/enter_chroot.sh >>>> index >>>> bca9c68866b365e58899af344589004058e88264..5c7f07b52a14043bf0d174966fed85f3a69b2651 >>>> 100755 >>>> --- a/enter_chroot.sh >>>> +++ b/enter_chroot.sh >>>> @@ -129,6 +129,7 @@ SAVED_AUTOMOUNT_PREF_FILE="/tmp/.automount_pref" >>>> sudo chmod 0777 "$FLAGS_chroot/var/lock" >>>> >>>> LOCKFILE="$FLAGS_chroot/var/lock/enter_chroot" >>>> +SYNCERPIDFILE="${FLAGS_chroot}/var/tmp/enter_chroot_sync.pid" >>>> >>>> >>>> function ensure_mounted { >>>> @@ -153,12 +154,46 @@ function ensure_mounted { >>>> fi >>>> } >>>> >>>> +function env_sync_proc { >>>> + # This function runs and performs periodic updates to the chroot env, >>>> if >>>> + # necessary. >>>> + >>>> + local poll_interval=10 >>>> + local sync_files="etc/resolv.conf etc/hosts" >>>> + >>>> + # Make sure the synced files are writable by normal user, so that we >>>> + # don't have to sudo inside the loop. >>>> + for file in ${sync_files}; do >>>> + sudo chown ${USER} ${FLAGS_chroot}/${file} 1>&2 >>>> + done >>>> + >>>> + while true; do >>>> + # Sync files >>>> + for file in ${sync_files}; do >>>> + if ! cmp /${file} ${FLAGS_chroot}/${file} &> /dev/null; then >>>> + cp -f /${file} ${FLAGS_chroot}/${file} >>>> + fi >>>> + done >>>> + >>>> + sleep ${poll_interval} >>>> + done >>>> +} >>>> + >>>> function setup_env { >>>> # Validate sudo timestamp before entering the critical section so that >>>> we >>>> # don't stall for a password while we have the lockfile. >>>> # Don't use sudo -v since that has issues on machines w/ no password. >>>> sudo echo "" > /dev/null >>>> >>>> + # Syncer proc, but only the first time >>>> + if ! [ -f "${SYNCERPIDFILE}" ] || \ >>>> + ! [ -d /proc/$(cat "${SYNCERPIDFILE}") ]; then >>>> + debug "Starting sync process" >>>> + env_sync_proc & >>>> + echo $! > "${SYNCERPIDFILE}" >>>> + disown $! >>>> + fi >>>> + >>>> ( >>>> flock 200 >>>> echo $$ >> "$LOCKFILE" >>>> @@ -283,6 +318,9 @@ function teardown_env { >>>> debug "At least one other pid is running in the chroot, so not" >>>> debug "tearing down env." >>>> else >>>> + debug "Stopping syncer process" >>>> + kill $(cat "${SYNCERPIDFILE}") >>>> + >>>> MOUNTED_PATH=$(readlink -f "$FLAGS_chroot") >>>> debug "Unmounting chroot environment." >>>> # sort the list of mounts in reverse order, to ensure umount of >>>> >>>> >>>> >>> >> >
Why copy/sync rather than mount bind them read-only?
Mount binding files is too funky. I'm not even sure it's officially supported, more like working because of a miracle, and i'm fairly sure it doesn't ever do what you expect it to. Consider this example: # touch test # mount -o bind,ro /etc/hosts test mount: warning: test seems to be mounted read-write. # vim /etc/hosts *deletes a line* # cmp /etc/hosts test cmp: EOF on /etc/hosts While things like echo "x" >> /etc/hosts do work, mount bind files is not a reliable thing to do. I assume the mount bind is associated with a particular inode and any use that causes the file to be rewritten (deleted && created anew) rather than appended is going to not propagate its changes to the bind point. Needless to say, mount binding the whole /etc is not a great idea. On Mon, Apr 4, 2011 at 11:12 PM, <sosa@chromium.org> wrote: > Why copy/sync rather than mount bind them read-only? > > > http://codereview.chromium.org/6720005/ >
Ping? :) On Tue, Apr 5, 2011 at 1:03 AM, Zdenek Behan <zbehan@chromium.org> wrote: > Mount binding files is too funky. I'm not even sure it's officially > supported, more like working because of a miracle, and i'm fairly sure it > doesn't ever do what you expect it to. Consider this example: > > # touch test > # mount -o bind,ro /etc/hosts test > mount: warning: test seems to be mounted read-write. > # vim /etc/hosts > *deletes a line* > # cmp /etc/hosts test > cmp: EOF on /etc/hosts > > While things like echo "x" >> /etc/hosts do work, mount bind files is not a > reliable thing to do. I assume the mount bind is associated with a > particular inode and any use that causes the file to be rewritten (deleted > && created anew) rather than appended is going to not propagate its changes > to the bind point. > > Needless to say, mount binding the whole /etc is not a great idea. > > On Mon, Apr 4, 2011 at 11:12 PM, <sosa@chromium.org> wrote: > >> Why copy/sync rather than mount bind them read-only? >> >> >> http://codereview.chromium.org/6720005/ >> > >
LGTM, thanks.
As a reference point, a sync process that has been running in my chroot since 2/4 (6 days) has consumed 34 seconds of cpu time, that translates to an average load of 0.006% on a single-core machine. On Wed, Apr 6, 2011 at 11:25 PM, <robotboy@chromium.org> wrote: > LGTM, thanks. > > > http://codereview.chromium.org/6720005/ > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
