Created attachment 206573 [details] consul.diff Update to 1.5.3 BUG FIXES: autopilot: update to also remove failed nodes from WAN gossip pool [GH-6028] agent: avoid reverting any check updates that occur while a service is being added or the config is reloaded [GH-6144] auto-encrypt: fix an issue that could cause cloud retry-join to fail when utilized with auto-encrypt by falling back to a default port [GH-6205] agent: fix several data races and bugs related to node-local alias checks [GH-5876] api: update link to agent caching in comments [GH-5935] connect: fix proxy address formatting for IPv6 addresses [GH-5460] connect: store signingKeyId instead of authorityKeyId [GH-6005] ui: fix service instance linking when multiple non-unique service id's exist on multiple nodes [GH-5933] ui: Improve error messaging for ACL policies [GH-5836] txn: Fixed an issue that would allow a CAS operation on a service to work when it shouldn't have. [GH-5971] etc
Created attachment 206586 [details] consul.diff
Created attachment 206587 [details] consul.diff
Created attachment 206591 [details] consul.diff
Created attachment 206592 [details] consul.diff
Created attachment 206594 [details] consul.diff
Created attachment 206613 [details] consul.diff
Created attachment 207512 [details] consul.diff sync
Created attachment 207529 [details] consul.diff Update to 1.6.1
https://github.com/hashicorp/consul/blob/v1.6.1/CHANGELOG.md
Created attachment 207578 [details] consul.diff
Sorry for the slow response, but 1.6.1 did not work when I tried it.
(In reply to Steve Wills from comment #11) Please try it again, on my test environment all works.
This version still does not work for me. The UI doesn't load no matter what I do, though the daemon does connect to the cluster. lsof doesn't show the process listening on port 8500 or any port I specify for http. The changes to the rc script are incorrect for a number of reasons. The config check fails because I do not have a bind address specified in my config, though I do have it specified in consul_args in rc.conf. For testing purposes I've moved this to my config file, but I believe we should still support it in consul_args The daemon does not shut down properly, it always hangs and I have to use kill -9 to stop it, and I seems to consume 400% CPU. Less importantly, I think maybe the syslog logging should be enabled by default. It may be better to use the -syslog and -pid-file flags to consul.
(In reply to Steve Wills from comment #13) Git bisect shows this commit as the culprit: https://github.com/hashicorp/consul/commit/4a4c63bda0b779b3ccce864e28ee4e50c9411458 And this is the PR for that: https://github.com/hashicorp/consul/pull/5468 And it looks like this issue: https://github.com/hashicorp/consul/issues/6120 is what I'm hitting. There's a PR to fix it, but no progress for a while: https://github.com/hashicorp/consul/pull/6128 I do have IPv6 recursors specified in my config.
(In reply to Steve Wills from comment #14) It seems adding the ":53" to my IPv6 recursor specification avoids the issue for now. I'll work on the other issues and provide an updated patch.
Created attachment 207891 [details] consul.diff
Created attachment 207893 [details] patch to update to 1.6.1 only There are a lot of changes to the RC script that need to be discussed and this has taken a while, so let's just get the update done and do the other changes separately. The attached patch look OK for that for now? As far as the RC script changes, here are some notes: It doesn't seem to be necessary to make it possible to override the pidfile. Not many rc scripts seem to do this, so unless there's an obvious reason it's needed, let's skip that. For the config test, as far as I know, consul doesn't really require you to have a config file, you can specify everything on the command line if you like, and we should support that. Consider the example I mentioned previously with the consul_args specifying -bind args and other things. Also, packaging the config dir doesn't seem to make sense, having the rc script create it works well. So, the config test should at least be something the user can disable, via a consul_configtest rc.conf variable (it can still default on). Perhaps like this: consul_checkconfig() { if checkyesno consul_configtest; then echo "Performing sanity check on ${name} configuration:" Avoiding packaging the config dir also means we don't need to support changing the user/group/config dir at build time, all the matters is the rc.conf settings at run time when the service is started up, after the package is installed. We should take a similar "default on but can be disabled" approach to the syslog output as well, such as: : ${consul_syslog:="YES"} if checkyesno consul_syslog; then consul_syslog_flags="-T ${name}" if [ -n "${consul_syslog_priority}" ]; then consul_syslog_flags="${consul_syslog_flags} -s ${consul_syslog_priority}" fi if [ -n "${consul_syslog_facility}" ]; then consul_syslog_flags="${consul_syslog_flags} -l ${consul_syslog_facility}" fi fi I don't understand why there would be a need to pass -u user to daemon, rc.conf handles that for you. Unless I'm missing something, the change only makes it so that daemon itself runs as root, which doesn't seem ideal to me. Changing consul_dir to consul_datadir is going to surprise users and we should at least warn and provide some backwards compatibility.
(In reply to Steve Wills from comment #17) the ability to override the pidfile is an additional freedom for customization, but it's not necessary. Configtest is very friendly for admins. Packaging directories allows you to create images for jail etc. Creating dir only at runtime is a bad idea. IMHO extended consul_syslog=YES is not need, because consul support native syslog output. Existing consul_debug help to log stdout on errors. Using the -u flag allows you to correctly create and delete a pidfile. Why are you afraid running daemon as root? Change consul_dir to consul_datadir made for a better corresponding with the configuration settings, and official documentation. Good idea to write this in pkg-message and maybe make it backward compatible. If you don't mind I'll make a patch proposal.
(In reply to Dmitry Wagin from comment #18) > Creating dir only at runtime is a bad idea. Why? > Why are you afraid running daemon as root? Why is it needed to run as root? > IMHO extended consul_syslog=YES is not need, because consul support native syslog output. Existing consul_debug help to log stdout on errors. Your patch doesn't use the consul syslog support, it passes the -T flag to daemon. I think it is best to use daemon's syslog support and give the user an easy way to set the facility and priority as mentioned.
(In reply to Steve Wills from comment #19) >> Creating dir only at runtime is a bad idea. >Why? Because directories may be needed as mount points before start consul. >> Why are you afraid running daemon as root? > Why is it needed to run as root? Using the -u flag allows you to correctly manipulate with a pidfile. >I think it is best to use daemon's syslog support and give the user an easy way >to set the facility and priority as mentioned. daemon syslog support is only usable for debugging. for example: native syslog output: Sep 27 19:11:38 dc01-consul-01 consul[52044]: agent: Caught signal: terminated vs daemon syslog output: Sep 27 19:11:38 dc01-consul-01 consul[52043]: 2019/09/27 19:11:38 [INFO] agent: Caught signal: terminated
Created attachment 207896 [details] consul.diff
(In reply to Dmitry Wagin from comment #20) >>> Creating dir only at runtime is a bad idea. >>Why? >Because directories may be needed as mount points before start consul. The rc script already ensure directories that are needed are created. >>> Why are you afraid running daemon as root? >> Why is it needed to run as root? >Using the -u flag allows you to correctly manipulate with a pidfile. But you already said modifying the pidfile path isn't needed. > daemon syslog support is only usable for debugging. for example: Not sure what this is supposed to show, but in my experience with it, passing the -T flag to daemon(8) gets all the output and status. Like I said, for now let's just get the update committed and we can work on any rc script changes separately.
A commit references this bug: Author: swills Date: Fri Sep 27 20:07:10 UTC 2019 New revision: 513055 URL: https://svnweb.freebsd.org/changeset/ports/513055 Log: sysutils/consul: update to 1.6.1 PR: 239874 Reported by: Dmitry Wagin <dmitry.wagin@ya.ru> Changes: head/sysutils/consul/Makefile head/sysutils/consul/distinfo
Update committed, closing. Please open separate PRs for specific rc file changes if desired.