Bug 239874 - sysutils/consul: Update to 1.6.1
Summary: sysutils/consul: Update to 1.6.1
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: Steve Wills
URL: https://github.com/hashicorp/consul/b...
Keywords:
Depends on:
Blocks:
 
Reported: 2019-08-15 12:59 UTC by Dmitry Wagin
Modified: 2019-09-27 20:13 UTC (History)
0 users

See Also:


Attachments
consul.diff (4.44 KB, patch)
2019-08-15 12:59 UTC, Dmitry Wagin
no flags Details | Diff
consul.diff (698 bytes, patch)
2019-08-15 15:56 UTC, Dmitry Wagin
no flags Details | Diff
consul.diff (5.14 KB, patch)
2019-08-15 15:57 UTC, Dmitry Wagin
no flags Details | Diff
consul.diff (5.15 KB, patch)
2019-08-15 18:59 UTC, Dmitry Wagin
no flags Details | Diff
consul.diff (5.22 KB, patch)
2019-08-15 19:17 UTC, Dmitry Wagin
no flags Details | Diff
consul.diff (5.43 KB, patch)
2019-08-15 20:30 UTC, Dmitry Wagin
no flags Details | Diff
consul.diff (6.13 KB, patch)
2019-08-16 12:18 UTC, Dmitry Wagin
no flags Details | Diff
consul.diff (4.72 KB, patch)
2019-09-15 21:07 UTC, Dmitry Wagin
no flags Details | Diff
consul.diff (4.74 KB, patch)
2019-09-16 02:08 UTC, Dmitry Wagin
no flags Details | Diff
consul.diff (4.77 KB, patch)
2019-09-17 15:07 UTC, Dmitry Wagin
no flags Details | Diff
consul.diff (4.99 KB, patch)
2019-09-27 16:49 UTC, Dmitry Wagin
no flags Details | Diff
patch to update to 1.6.1 only (849 bytes, patch)
2019-09-27 17:49 UTC, Steve Wills
no flags Details | Diff
consul.diff (5.24 KB, patch)
2019-09-27 19:35 UTC, Dmitry Wagin
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Wagin 2019-08-15 12:59:48 UTC
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
Comment 1 Dmitry Wagin 2019-08-15 15:56:48 UTC
Created attachment 206586 [details]
consul.diff
Comment 2 Dmitry Wagin 2019-08-15 15:57:45 UTC
Created attachment 206587 [details]
consul.diff
Comment 3 Dmitry Wagin 2019-08-15 18:59:11 UTC
Created attachment 206591 [details]
consul.diff
Comment 4 Dmitry Wagin 2019-08-15 19:17:11 UTC
Created attachment 206592 [details]
consul.diff
Comment 5 Dmitry Wagin 2019-08-15 20:30:00 UTC
Created attachment 206594 [details]
consul.diff
Comment 6 Dmitry Wagin 2019-08-16 12:18:02 UTC
Created attachment 206613 [details]
consul.diff
Comment 7 Dmitry Wagin 2019-09-15 21:07:18 UTC
Created attachment 207512 [details]
consul.diff

sync
Comment 8 Dmitry Wagin 2019-09-16 02:08:18 UTC
Created attachment 207529 [details]
consul.diff

Update to 1.6.1
Comment 10 Dmitry Wagin 2019-09-17 15:07:01 UTC
Created attachment 207578 [details]
consul.diff
Comment 11 Steve Wills freebsd_committer 2019-09-17 15:09:21 UTC
Sorry for the slow response, but 1.6.1 did not work when I tried it.
Comment 12 Dmitry Wagin 2019-09-17 15:14:04 UTC
(In reply to Steve Wills from comment #11)

Please try it again, on my test environment all works.
Comment 13 Steve Wills freebsd_committer 2019-09-27 15:21:02 UTC
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.
Comment 14 Steve Wills freebsd_committer 2019-09-27 16:20:46 UTC
(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.
Comment 15 Steve Wills freebsd_committer 2019-09-27 16:35:25 UTC
(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.
Comment 16 Dmitry Wagin 2019-09-27 16:49:03 UTC
Created attachment 207891 [details]
consul.diff
Comment 17 Steve Wills freebsd_committer 2019-09-27 17:49:05 UTC
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.
Comment 18 Dmitry Wagin 2019-09-27 18:32:46 UTC
(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.
Comment 19 Steve Wills freebsd_committer 2019-09-27 18:50:05 UTC
(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.
Comment 20 Dmitry Wagin 2019-09-27 19:30:27 UTC
(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
Comment 21 Dmitry Wagin 2019-09-27 19:35:15 UTC
Created attachment 207896 [details]
consul.diff
Comment 22 Steve Wills freebsd_committer 2019-09-27 20:05:58 UTC
(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.
Comment 23 commit-hook freebsd_committer 2019-09-27 20:08:12 UTC
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
Comment 24 Steve Wills freebsd_committer 2019-09-27 20:13:31 UTC
Update committed, closing. Please open separate PRs for specific rc file changes if desired.