Bug 255822 - net-mgmt/victoria-metrics: rc.d improvement
Summary: net-mgmt/victoria-metrics: rc.d improvement
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Alexey Dokuchaev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-05-12 18:50 UTC by Lapo Luchini
Modified: 2021-05-20 15:14 UTC (History)
1 user (show)

See Also:
bugzilla: maintainer-feedback? (danfe)


Attachments
net-mgmt/victoria-metrics: fix args string, ca_root_nss deps (2.38 KB, patch)
2021-05-20 08:47 UTC, Oleg Ginzburg
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lapo Luchini 2021-05-12 18:50:30 UTC
I guess this line should not reference $cbsd_mq_router_config:

victoria_metrics_args=${cbsd_mq_router_config-"--storageDataPath=/var/db/victoria-metrics --retentionPeriod=1 --httpListenAddr=:8428"}

https://github.com/freebsd/freebsd-ports/blob/da7851dcc648e66d2036f471c3084eb943063d3a/net-mgmt/victoria-metrics/files/victoria-metrics.in#L17

Also, it would be nice to map "service victoria-metrics reload" to a `kill -HUP`, as it is a supported signal to reload (scrape) configuration.

cheers,
Comment 1 Alexey Dokuchaev freebsd_committer 2021-05-13 02:47:10 UTC
(In reply to Lapo Luchini from comment #0)
> I guess this line should not reference $cbsd_mq_router_config:
I've also noticed it earlier and asked Oleg, he said it was a secret undocumented vmetrics' option. ;-)  I agree that it looks a bit weird to see embedded vague references to other, not directly related port(s), but you should probably talk to Oleg about it.

> Also, it would be nice to map "service victoria-metrics reload" to a `kill -HUP`
Could you try adding "extra_commands=reload" line to the rc script and see if it works as expected?
Comment 2 Lapo Luchini 2021-05-13 08:26:15 UTC
Ah, sorry, I misread that as "use these defaults if original variable is not null" but that's ${name:-} not ${name-} (which I can't find in "man sh", oh well).

> Could you try adding "extra_commands=reload" line to the rc script

I tried: it does. :)

% tail -n 2 /var/log/victoria_metrics/victoria_metrics.log
2021-05-13T08:24:12.607Z        info    /wrkdirs/usr/ports/net-mgmt/victoria-metrics/work/VictoriaMetrics-1.59.0/lib/promscrape/scraper.go:123      SIGHUP received; reloading Prometheus configs from "/usr/local/etc/victoria-scrape.yml"
Comment 3 Lapo Luchini 2021-05-19 15:30:42 UTC
I checked that "extra_commands=reload" also works in (vmutils')
/usr/local/etc/rc.d/vmagent
Comment 4 Oleg Ginzburg 2021-05-20 08:47:25 UTC
Created attachment 225114 [details]
net-mgmt/victoria-metrics: fix args string, ca_root_nss deps

- fix args string in rc.d
- added ca_root_nss runtime dependencies
Comment 5 Oleg Ginzburg 2021-05-20 08:51:44 UTC
Sorry, I forgot to remove cbsd_mq_router_config when Alexey informed me about it.
This is a copy-paste error from my previous port.

I added a patch with a fix.

Also added:
- dependency on ca_root_nss certificates
- rc.d reload extra command
Comment 6 Alexey Dokuchaev freebsd_committer 2021-05-20 09:17:33 UTC
So far so good.  I was looking at their OpenBSD port (https://github.com/VictoriaMetrics/VictoriaMetrics/tree/master/ports/OpenBSD) and noticed that they use shorter /var/db path and user/group names.  I'm considering doing the same for our port.  Any objections?

-USERS=         victoria-metrics
-GROUPS=        victoria-metrics
-VICTORIA_DATA?=        /var/db/victoria-metrics
+USERS=         vmetrics
+GROUPS=        vmetrics
+VICTORIA_DATA?=        /var/db/vmetrics
Comment 7 Alexey Dokuchaev freebsd_committer 2021-05-20 09:22:54 UTC
(In reply to Alexey Dokuchaev from comment #6)
> I'm considering doing the same for our port.
On the other hands, this would diverge from the rc-script which bakes full name in many places...  Need to think more about it.
Comment 8 commit-hook freebsd_committer 2021-05-20 10:28:21 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=69d8c3f90fd43297b044f3f99d99084216c9dc14

commit 69d8c3f90fd43297b044f3f99d99084216c9dc14
Author:     Alexey Dokuchaev <danfe@FreeBSD.org>
AuthorDate: 2021-05-20 10:18:36 +0000
Commit:     Alexey Dokuchaev <danfe@FreeBSD.org>
CommitDate: 2021-05-20 10:26:54 +0000

    net-mgmt/victoria-metrics: port had been improved (+)

    Add missing run-time dependency on `security/ca_root_nss': apparently,
    every Go application that uses TLS needs root CA certificates installed
    in the default place, which are used for verifying signatures provided
    by peers during establishing TLS connections.

    While here, remove accidental reference to `cbsd_mq_router_config' in
    the rc script and allow to reload (scrape) configuration by sending
    SIGHUP to processes.

    PR:     255822

 net-mgmt/victoria-metrics/Makefile                  | 3 +++
 net-mgmt/victoria-metrics/files/victoria-metrics.in | 3 ++-
 net-mgmt/victoria-metrics/files/vmagent.in          | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)
Comment 9 Oleg Ginzburg 2021-05-20 11:00:11 UTC
(In reply to Alexey Dokuchaev from comment #6)

I always like the idea of minimizing the difference between upstream and port. Linux packages use 'victoriametrics': 

https://github.com/patsevanton/victoriametrics-rpm/commit/8e38be7cd96e72a388cf58da414fc70ff3ef3ea2

However, I cannot find a reason why in the original patch I started using 'victoria-metrics'
Comment 10 Alexey Dokuchaev freebsd_committer 2021-05-20 11:03:46 UTC
(In reply to olevole from comment #9)
> I always like the idea of minimizing the difference between upstream and port.
Right.  Let's see if upstream makes any moves to unite the spelling in popular GNU/Linux packages and its own OpenBSD port example.  I've kept the status quo for now.
Comment 11 Lapo Luchini 2021-05-20 14:45:54 UTC
PS: dependency on ca_root_nss won't be necessary after either of this lands:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256003
https://github.com/golang/go/pull/46276
Comment 12 Lapo Luchini 2021-05-20 15:01:55 UTC
> However, I cannot find a reason why in the original patch I started using 'victoria-metrics'

I don't know about your reason, of course, but in my own porting attempt I was using "the executable name" as a base, and that has the hyphen.
Comment 13 Alexey Dokuchaev freebsd_committer 2021-05-20 15:14:45 UTC
(In reply to Lapo Luchini from comment #11)
> PS: dependency on ca_root_nss won't be necessary after either of this lands
No problem, we can always remove explicit dependency once it's no longer necessary.