[Buildroot] [External] [PATCH v2 1/2] package/netdata: new package

Thu Oct 31 17:05:27 UTC 2019

Marcin,

On Thu, Oct 31, 2019 at 11:18 AM Marcin Niestrój
<m.niestroj at grinn-global.com> wrote:
>
> Hi Matt,
>
> Matthew Weber <matthew.weber at collins.com> writes:
>
> > Marcin,
> >
> > On Wed, Oct 30, 2019 at 4:33 AM Marcin Niestroj
> > <m.niestroj at grinn-global.com> wrote:
> >>
> >> Always provide --disable-dbengine configuration option, because we do
> >> not support libjudy dependency that is required otherwise.
> >>
> >> Signed-off-by: Marcin Niestroj <m.niestroj at grinn-global.com>
> > [snip]
> >
> >> --- /dev/null
> >> +++ b/package/netdata/netdata.mk
> >> @@ -0,0 +1,56 @@
> >> +################################################################################
> >> +#
> >> +# netdata
> >> +#
> >> +################################################################################
> >> +
> >
> > The package's default install step attempts to create folders in
> > $(TARGET_DIR)/var/lib/netdata, $(TARGET_DIR)/var/cache/netdata, and
> > $(TARGET_DIR)/var/log/netdata.  For the cache and log cases in the
> > default Buildroot skeleton, those point to ../tmp and results in a
> > race condition causing a "file exists error".
>
> I'll try to clarify what happens there. This is what we see in build
> log:
>
>    /usr/bin/install -c -m 644 packaging/installer/.keep '/buildroot/test-netdata/TestNetdata/target/var/lib/netdata'
>     /usr/bin/install -c netdata '/buildroot/test-netdata/TestNetdata/target/usr/sbin'
>    /usr/bin/install -c -m 644 packaging/installer/.keep '/buildroot/test-netdata/TestNetdata/target/var/lib/netdata/registry'
>    /usr/bin/install -c -m 644 packaging/installer/.keep '/buildroot/test-netdata/TestNetdata/target/var/log/netdata'
>    /usr/bin/install -c -m 644 packaging/installer/.keep '/buildroot/test-netdata/TestNetdata/target/var/cache/netdata'
>   /usr/bin/install: cannot change permissions of '/buildroot/test-netdata/TestNetdata/target/var/cache/netdata/.keep': No such file or directory
>
> The problem is that `install` is called in parallel. Looking at what
> `install` really does with strace:
>
>   stat("/buildroot/test-netdata/TestNetdata/target/var/cache/netdata", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
>   stat("packaging/installer/.keep", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
>   newfstatat(AT_FDCWD, "/buildroot/test-netdata/TestNetdata/target/var/cache/netdata/.keep", {st_mode=S_IFREG|0644, st_size=0, ...}, AT_SYMLINK_NOFOLLOW) = 0
>   unlink("/buildroot/test-netdata/TestNetdata/target/var/cache/netdata/.keep") = 0
>   openat(AT_FDCWD, "packaging/installer/.keep", O_RDONLY) = 3
>   fstat(3, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
>   openat(AT_FDCWD, "/buildroot/test-netdata/TestNetdata/target/var/cache/netdata/.keep", O_WRONLY|O_CREAT|O_EXCL, 0600) = 4
>   fstat(4, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
>   fadvise64(3, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
>   mmap(NULL, 139264, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f6dfa7ee000
>   read(3, "", 131072)                     = 0
>   fsetxattr(4, "system.posix_acl_access", "\2\0\0\0\1\0\6\0\377\377\377\377\4\0\0\0\377\377\377\377 \0\0\0\377\377\377\377", 28, 0) = 0
>   close(4)                                = 0
>   close(3)                                = 0
>   munmap(0x7f6dfa7ee000, 139264)          = 0
>   lstat("/buildroot/test-netdata/TestNetdata/target/var/cache/netdata/.keep", {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
>   chmod("/buildroot/test-netdata/TestNetdata/target/var/cache/netdata/.keep", 0644) = 0
>
> So what (most probably) happens is that one `install` process has
> created the `.keep` file, but didn't make it to change permission,
> because another `install` process has already unlinked it.

Yeah, it's inheritly because of the way the base skeleton is setup
with symlinks in buildroot.

>
> > To fix that target install issue, I'd propose the following patch be
> > added.
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__pastebin.com_TL3znCJs&d=DwIFaQ&c=ilBQI1lupc9Y65XwNblLtw&r=y1sOV0GV8NZUkffv7oCRxs2Sd3nOBS-NxDM3NY8lOgs&m=QZL88tMKpKawDLFC9yPt80FEa5Ojbic8ue1DfxOQ3mY&s=U45yjsRvZGNjr4DcvyCulMnguBFPNn2KgFqHYPPQBpA&e=
>
> With this patch cache files would be written in persistent storage under
> /var/lib/netdata/cache. I would rather leave those files in tmpfs.
>
> I also do not like the fact that logs would be put under /tmp/debug.log,
> /tmp/error.log and /tmp/access.log files. There are plenty of other
> daemons which would like to do the same :)
>

Keep in mind, none of the items written to /var/log or /var/cache
during build time end up on the target.  If you want different runtime
behavior with folders for cache and logs you have to stage those with
a startup script.  Maybe it makes sense to add a S60netdata script in
your new version of the series which could setup the folders in the
tmpfs to be used for logging and cache accordingly?  As part of this
startup script you could also symlink the /var/lib/netdata/cache
folder to a tmpfs location.  I suggested the patch to keep the changes
minimal to the upstream package and then at runtime we can tailor how
netdata actually uses the folder.

> So far we depend on creating /var/cache/netdata (because netdata tries
> to cd into it by default during init). So we have already the needed
> structure for both cache and log (both point to /tmp/netdata). So the
> only thing I would do is to remove .keep installation from Makefile.am
> to get rid of race condition. This means simple patch to maintain within
> Buildroot. What do you think?

Like previously mentioned you'll need an init script to setup any
folders in /var/cache or log as those locations are symlinked to /tmp
by default and the buildtime folders won't be present at runtime.  So
you could suggest a .keep cleanup script if that seems less intrusive
to the package and maybe you can even make it upstreamable?  In some
ways if we can just bypass this whole localstatedir folder setup
section of the makefile that would be ideal.

Regards,
Matt