| Message ID | 20191030093243.2936111-1-m.niestroj@grinn-global.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] package/netdata: new package | expand |
Marcin, On Wed, Oct 30, 2019 at 4:33 AM Marcin Niestroj <m.niestroj@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@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". To fix that target install issue, I'd propose the following patch be added. https://pastebin.com/TL3znCJs Regards, Matt > -- > 2.23.0 >
Hi Matt, Matthew Weber <matthew.weber@collins.com> writes: > Marcin, > > On Wed, Oct 30, 2019 at 4:33 AM Marcin Niestroj > <m.niestroj@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@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. > To fix that target install issue, I'd propose the following patch be > added. > > https://pastebin.com/TL3znCJs 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 :) 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? > > Regards, > Matt > > >> -- >> 2.23.0 >> -- Marcin Niestrój
Marcin, On Thu, Oct 31, 2019 at 11:18 AM Marcin Niestrój <m.niestroj@grinn-global.com> wrote: > > Hi Matt, > > Matthew Weber <matthew.weber@collins.com> writes: > > > Marcin, > > > > On Wed, Oct 30, 2019 at 4:33 AM Marcin Niestroj > > <m.niestroj@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@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
Hi Matt, Matthew Weber <matthew.weber@collins.com> writes: > Marcin, > > On Thu, Oct 31, 2019 at 11:18 AM Marcin Niestrój > <m.niestroj@grinn-global.com> wrote: >> >> Hi Matt, >> >> Matthew Weber <matthew.weber@collins.com> writes: >> >> > Marcin, >> > >> > On Wed, Oct 30, 2019 at 4:33 AM Marcin Niestroj >> > <m.niestroj@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@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. Yes, I am aware of that. So both /var/log/netdata and /var/cache/netdata point to non-existing (during system boot) /tmp/netdata directory. > If you want different runtime behavior with folders for cache and logs > you have to stage those with a startup script. Correct. And with current patch series we just install netdata within Buildroot and rely on configuring and starting netdata by user. I am not sure > 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? If we will provide S60netdata, then we also need some equivalent for systemd. What do you think about creating (mkdir -p) only /var/cache/netdata in such init script? This directory is used as current (home) directory for netdata. /var/log/cache looks like optional for netdata, because it will safely continue execution if log files cannot be opened. In default skeleton however both would point to /tmp/netdata, so there would be no problem with logs. > 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. We can also simply remove the installation of .keep files with https://pastebin.com/raw/P9DQC1ry and it solves the problem with even less changes (in my opinion). This is also what you suggested below I think. > >> 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. The problem with netdata is that it is not clear which autotools or cmake they are targetting to support in the future. I don't feel comfortable in investing time now to support some feature in autotools (I barely know it as developer) if cmake will be the only supported solution in near future. On the other hand cmake seems to work differently, e.g. there is no .keep installation there at all, but unfortunately more dependencies are required instead of optional. So in order to do things properly we would need to ask netdata about their prefered build system for future and do improvements there :) > > Regards, > Matt > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hi Matt, Marcin Niestrój <m.niestroj@grinn-global.com> writes: > Hi Matt, > > [snip] > > So in order to do things properly we would need to ask netdata about > their prefered build system for future and do improvements there :) > Looking at Github issues there is an ongoing effort to support cmake as the main build system. So I would go with autotools for now and add patch that removes .keep installation. In future we can convert to cmake if that will be ready in next netdata release. Then we will drop Buildroot specific patch and do improvements in cmake installation if neccessary. What do you think about that?
On Mon, Nov 4, 2019 at 4:42 AM Marcin Niestrój <m.niestroj@grinn-global.com> wrote: > > Hi Matt, > > Marcin Niestrój <m.niestroj@grinn-global.com> writes: > > > Hi Matt, > > > > [snip] > > > > So in order to do things properly we would need to ask netdata about > > their prefered build system for future and do improvements there :) > > > > Looking at Github issues there is an ongoing effort to support cmake as > the main build system. So I would go with autotools for now and add > patch that removes .keep installation. In future we can convert to cmake > if that will be ready in next netdata release. Then we will drop > Buildroot specific patch and do improvements in cmake installation if > neccessary. What do you think about that? Ah that is a good catch, if they are changing we'll have to integrate this again soon. I'd suggest putting in a ticket to bring visibility to this .keep collision such that they don't keep it in the cmake update. Then you can reference that ticket as your upstream issue/bug in your patch in buildroot. Matt
Marcin, On Mon, Nov 4, 2019 at 3:54 AM Marcin Niestrój <m.niestroj@grinn-global.com> wrote: > > Hi Matt, > > Matthew Weber <matthew.weber@collins.com> writes: > > > Marcin, > > > > On Thu, Oct 31, 2019 at 11:18 AM Marcin Niestrój > > <m.niestroj@grinn-global.com> wrote: > >> > >> Hi Matt, > >> > >> Matthew Weber <matthew.weber@collins.com> writes: > >> > >> > Marcin, > >> > > >> > On Wed, Oct 30, 2019 at 4:33 AM Marcin Niestroj > >> > <m.niestroj@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@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. > > Yes, I am aware of that. So both /var/log/netdata and /var/cache/netdata > point to non-existing (during system boot) /tmp/netdata directory. > > > If you want different runtime behavior with folders for cache and logs > > you have to stage those with a startup script. > > Correct. And with current patch series we just install netdata within > Buildroot and rely on configuring and starting netdata by user. I am not sure > > > 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? > > If we will provide S60netdata, then we also need some equivalent for > systemd. What do you think about creating (mkdir -p) only > /var/cache/netdata in such init script? This directory is used as > current (home) directory for netdata. /var/log/cache looks like optional > for netdata, because it will safely continue execution if log files > cannot be opened. In default skeleton however both would point to > /tmp/netdata, so there would be no problem with logs. Yeah the init script would need to create that netdata folder at runtime in /var/cache or /tmp or /var/log/ if not present. That would cover most of the use cases for tmpfs based or persistent. You could just add a sysvinit script to start with and then let others contribute systemd if that's not your focus for a init. > > > 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. > > We can also simply remove the installation of .keep files with > https://pastebin.com/raw/P9DQC1ry and it solves the problem with even > less changes (in my opinion). This is also what you suggested below I > think. Looks good, I replied to your other email related to this patch and strategy for now. > > > > >> 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. > > The problem with netdata is that it is not clear which autotools or > cmake they are targetting to support in the future. I don't feel > comfortable in investing time now to support some feature in autotools > (I barely know it as developer) if cmake will be the only supported > solution in near future. On the other hand cmake seems to work > differently, e.g. there is no .keep installation there at all, but > unfortunately more dependencies are required instead of optional. > > So in order to do things properly we would need to ask netdata about > their prefered build system for future and do improvements there :) I'd go with what we have for now and the basic patch to remove .keeps. I've noted in the other email thread about creating a issue ticket you can reference as well. Matt
diff --git a/DEVELOPERS b/DEVELOPERS index f41ac5f096..515626517f 100644 --- a/DEVELOPERS +++ b/DEVELOPERS @@ -1457,6 +1457,7 @@ F: package/lua-flu/ F: package/lua-stdlib/ F: package/luaossl/ F: package/murata-cyw-fw/ +F: package/netdata/ F: package/rs485conf/ F: package/turbolua/ diff --git a/package/Config.in b/package/Config.in index a1ac5069aa..e1e72beb2e 100644 --- a/package/Config.in +++ b/package/Config.in @@ -1875,6 +1875,7 @@ menu "Miscellaneous" source "package/linux-syscall-support/Config.in" source "package/mcrypt/Config.in" source "package/mobile-broadband-provider-info/Config.in" + source "package/netdata/Config.in" source "package/proj/Config.in" source "package/qemu/Config.in" source "package/qpdf/Config.in" diff --git a/package/netdata/0001-include-limits.h-before-using-LONG_MAX-7224.patch b/package/netdata/0001-include-limits.h-before-using-LONG_MAX-7224.patch new file mode 100644 index 0000000000..7234990862 --- /dev/null +++ b/package/netdata/0001-include-limits.h-before-using-LONG_MAX-7224.patch @@ -0,0 +1,29 @@ +From fdeac75f9c2e83799c7a44a8a8647d5a561ec96e Mon Sep 17 00:00:00 2001 +From: Marcin Niestroj <macius1990w@gmail.com> +Date: Tue, 29 Oct 2019 19:52:47 +0100 +Subject: [PATCH] include limits.h before using LONG_MAX (#7224) + +commit fdeac75f9c2e83799c7a44a8a8647d5a561ec96e upstream. + +This fixes build with musl standard C library. + +Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com> +--- + libnetdata/libnetdata.h | 1 + + 1 file changed, 1 insertion(+) + +diff --git a/libnetdata/libnetdata.h b/libnetdata/libnetdata.h +index ef883300..023ad2c2 100644 +--- a/libnetdata/libnetdata.h ++++ b/libnetdata/libnetdata.h +@@ -65,6 +65,7 @@ + #include <getopt.h> + #include <grp.h> + #include <pwd.h> ++#include <limits.h> + #include <locale.h> + #include <net/if.h> + #include <poll.h> +-- +2.23.0 + diff --git a/package/netdata/Config.in b/package/netdata/Config.in new file mode 100644 index 0000000000..30e8c30055 --- /dev/null +++ b/package/netdata/Config.in @@ -0,0 +1,16 @@ +config BR2_PACKAGE_NETDATA + bool "netdata" + depends on BR2_TOOLCHAIN_HAS_THREADS_NPTL + depends on BR2_USE_MMU # fork() + select BR2_PACKAGE_UTIL_LINUX + select BR2_PACKAGE_UTIL_LINUX_LIBUUID + help + Netdata is distributed, real-time, performance and health + monitoring for systems and applications. It is a highly + optimized monitoring agent you install on all your systems and + containers. + + https://github.com/netdata/netdata + +comment "netdata needs a toolchain w/ NPTL" + depends on !BR2_TOOLCHAIN_HAS_THREADS_NPTL diff --git a/package/netdata/netdata.hash b/package/netdata/netdata.hash new file mode 100644 index 0000000000..e3147a4af5 --- /dev/null +++ b/package/netdata/netdata.hash @@ -0,0 +1,3 @@ +# Locally calculated +sha256 c788ec01f5228768cbf5032324e041defbac3aaa57a074b98038444fc46ba2d4 netdata-1.18.1.tar.gz +sha256 0e5fd9d833efe9b79f784d1903281554af82d1b4261af67d35455728e5572aa6 LICENSE diff --git a/package/netdata/netdata.mk b/package/netdata/netdata.mk new file mode 100644 index 0000000000..2de283fe24 --- /dev/null +++ b/package/netdata/netdata.mk @@ -0,0 +1,56 @@ +################################################################################ +# +# netdata +# +################################################################################ + +NETDATA_VERSION = 1.18.1 +NETDATA_SITE = $(call github,netdata,netdata,v$(NETDATA_VERSION)) +NETDATA_LICENSE = GPL-3.0 +NETDATA_LICENSE_FILES = LICENSE +# netdata's source code is released without a generated configure script +NETDATA_AUTORECONF = YES +NETDATA_CONF_OPTS = --disable-dbengine +NETDATA_DEPENDENCIES = util-linux + +ifeq ($(BR2_GCC_ENABLE_LTO),y) +NETDATA_CONF_OPTS += --enable-lto +else +NETDATA_CONF_OPTS += --disable-lto +endif + +ifeq ($(BR2_PACKAGE_CUPS),y) +NETDATA_CONF_OPTS += --enable-plugin-cups +NETDATA_DEPENDENCIES += cups +else +NETDATA_CONF_OPTS += --disable-plugin-cups +endif + +ifeq ($(BR2_PACKAGE_JSON_C),y) +NETDATA_CONF_OPTS += --enable-jsonc +NETDATA_DEPENDENCIES += json-c +else +NETDATA_CONF_OPTS += --disable-jsonc +endif + +ifeq ($(BR2_PACKAGE_NFACCT),y) +NETDATA_CONF_OPTS += --enable-plugin-nfacct +NETDATA_DEPENDENCIES += nfacct +else +NETDATA_CONF_OPTS += --disable-plugin-nfacct +endif + +ifeq ($(BR2_PACKAGE_OPENSSL),y) +NETDATA_CONF_OPTS += --enable-https +NETDATA_DEPENDENCIES += openssl +else +NETDATA_CONF_OPTS += --disable-https +endif + +ifeq ($(BR2_PACKAGE_ZLIB),y) +NETDATA_DEPENDENCIES += zlib +else +NETDATA_CONF_OPTS += --without-zlib +endif + +$(eval $(autotools-package))
Always provide --disable-dbengine configuration option, because we do not support libjudy dependency that is required otherwise. Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com> --- changes v1 -> v2: - squash DEVELOPERS update (suggested by Arnout), - reduce number of required dependencies and support them as optional: - json-c: there are few custom JSON handling function if json-c library is not provided, - openssl: enable or disable https support based on its availability (and solve openssl not being in netdata.mk file - suggested by Matt), - add optional dependencies on cups, nfacct and zlib, - enable or disable -flto based on BR2_GCC_ENABLE_LTO - drop libuv and lz4 dependencies, because it is not required with --disable-dbengine option, - add comment about NETDATA_AUTORECONF (suggested by Matt), - apply upstreamed patch to fix musl build (musl build was not failing with v1 patch but did with v2). DEVELOPERS | 1 + package/Config.in | 1 + ...-limits.h-before-using-LONG_MAX-7224.patch | 29 ++++++++++ package/netdata/Config.in | 16 ++++++ package/netdata/netdata.hash | 3 + package/netdata/netdata.mk | 56 +++++++++++++++++++ 6 files changed, 106 insertions(+) create mode 100644 package/netdata/0001-include-limits.h-before-using-LONG_MAX-7224.patch create mode 100644 package/netdata/Config.in create mode 100644 package/netdata/netdata.hash create mode 100644 package/netdata/netdata.mk