Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consul: 0.9.3 -> 1.3.0 #48714

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@vdemeester
Copy link
Contributor

vdemeester commented Oct 19, 2018

Signed-off-by: Vincent Demeester vincent@sbr.pm

Motivation for this change

Bump consul to a more recent version

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

consul: 0.9.3 -> 1.3.0
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@c0bw3b

This comment has been minimized.

Copy link
Contributor

c0bw3b commented Oct 19, 2018

Please have a look at #44192 #41243 and #35602 and check questions raised before regarding consul.

python-consul has been updated in #43972 at least
Terraform and Vault should be ok / Prometheus too
Maybe confd is still going to be broken here but the package needs refreshing anyway...

/cc @nh2 for review

@c0bw3b

This comment has been minimized.

Copy link
Contributor

c0bw3b commented Oct 26, 2018

@GrahamcOfBorg build consul

@GrahamcOfBorg

This comment has been minimized.

Copy link

GrahamcOfBorg commented Oct 26, 2018

Failure on aarch64-linux (full log)

Attempted: consul

Partial log (click to expand)

github.com/hashicorp/consul/vendor/github.com/hashicorp/go-multierror
github.com/hashicorp/consul/vendor/github.com/armon/go-metrics
github.com/hashicorp/consul/vendor/golang.org/x/net/ipv4
github.com/hashicorp/consul/vendor/golang.org/x/net/ipv6
github.com/hashicorp/consul/vendor/github.com/hashicorp/serf/coordinate
github.com/hashicorp/consul/vendor/golang.org/x/crypto/ed25519
github.com/hashicorp/consul/vendor/github.com/miekg/dns
github.com/hashicorp/consul/vendor/github.com/hashicorp/raft
builder for '/nix/store/7g0b95d47dvyfgkhdf6h8pvqapaqqj10-consul-1.3.0.drv' failed with exit code 32
error: build of '/nix/store/7g0b95d47dvyfgkhdf6h8pvqapaqqj10-consul-1.3.0.drv' failed

@GrahamcOfBorg

This comment has been minimized.

Copy link

GrahamcOfBorg commented Oct 26, 2018

Success on x86_64-linux (full log)

Attempted: consul

Partial log (click to expand)

post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/10pxm6lksdcmxylaavshjpw0riqyiyjd-consul-1.3.0-bin
shrinking /nix/store/10pxm6lksdcmxylaavshjpw0riqyiyjd-consul-1.3.0-bin/bin/consul
shrinking /nix/store/10pxm6lksdcmxylaavshjpw0riqyiyjd-consul-1.3.0-bin/bin/certgen
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/10pxm6lksdcmxylaavshjpw0riqyiyjd-consul-1.3.0-bin/bin
patching script interpreter paths in /nix/store/10pxm6lksdcmxylaavshjpw0riqyiyjd-consul-1.3.0-bin
checking for references to /build in /nix/store/10pxm6lksdcmxylaavshjpw0riqyiyjd-consul-1.3.0-bin...
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
/nix/store/10pxm6lksdcmxylaavshjpw0riqyiyjd-consul-1.3.0-bin

@nh2

This comment has been minimized.

Copy link
Contributor

nh2 commented Oct 26, 2018

@c00w @vdemeester A couple questions:

  • The stuff I did in https://github.com/NixOS/nixpkgs/pull/41243/files, is any of that still necessary?
  • Specifically, should we include the README I added that describes how to update consul in nixpkgs?
  • Did you try whether the consul UI works?
  • Can you let me know what from #43972 I have to cherry-patch in order to test this with python-consul? Update: I think it's bd2ce52 and 719290d

Thanks!

@nh2

This comment has been minimized.

Copy link
Contributor

nh2 commented Oct 26, 2018

Oops, I missed that these questions should go to @vdemeester, not @c0bw3b.

@arianvp

This comment has been minimized.

Copy link
Contributor

arianvp commented Oct 26, 2018

The UI actually breaks with this update. we either have to explicitly enable LEGACY_UI or do the same as this PR: #49082

@c0bw3b

This comment has been minimized.

Copy link
Contributor

c0bw3b commented Oct 26, 2018

😅 but I do think your README from #41243 should be kept @nh2

@arianvp

This comment has been minimized.

Copy link
Contributor

arianvp commented Oct 26, 2018

Some context: They switched to a yarn based UI instead of the current ruby based build process. And that's enabled by default in 1.3.0

@nh2

This comment has been minimized.

Copy link
Contributor

nh2 commented Oct 26, 2018

Hmm, just tried out this PR's consul-1.3.0, but I'm a bit confused: The UI works for me as before, but doesn't look at all like the new UI on https://demo.consul.io/ui/dc1/services (which as per https://www.consul.io/intro/getting-started/ui.html is the default since 1.2.0), but instead looks like the old UI. That desipte I cannot find CONSUL_UI_LEGACY being set anywhere.

Why is that?

@arianvp

This comment has been minimized.

Copy link
Contributor

arianvp commented Oct 26, 2018

We currently do not at all build the new UI. We need some extra patches for that. So (Guesswork here) maybe it fallbacks to the old UI if the new UI isn't found? In that case, we can just merge this as is, and add another patch later to add the new UI.

@nh2

This comment has been minimized.

Copy link
Contributor

nh2 commented Oct 26, 2018

We currently do not at all build the new UI. We need some extra patches for that.

@arianvp I don't understand that comment. Consul upstream docs say the new UI is built by default in 1.3.0, and nixpkgs doesn't seem to have anything that disables that. So the new UI should be built.

@arianvp

This comment has been minimized.

Copy link
Contributor

arianvp commented Oct 26, 2018

@nh2 yes this is indeed the case. See https://github.com/hashicorp/consul/blob/25f04fbd21f6a8cc63385cba80092c8c8b1223b2/agent/config/flags.go#L106-L107

We enable -ui-dir in the configuration of consul and thus it uses the legacy UI https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/networking/consul.nix#L10

So.. we can merge this as is, without breaking the UI. and we can later create a PR for adding the v2 UI

@nh2

This comment has been minimized.

Copy link
Contributor

nh2 commented Oct 26, 2018

Ah, here is more info:

Specifying ui_dir seems to disable the new UI and enable the legacy UI.

We do that here:

(if cfg.webUi then { ui_dir = "${cfg.package.ui}"; } else { }) //

Switching that to ui = true enables the new UI and it seems to work as expected out of the box.

I've asked on the Consul gitter whether this behavoiur is intended, it doesn't seem to be documented.

@arianvp

This comment has been minimized.

Copy link
Contributor

arianvp commented Oct 26, 2018

I think when we switch from ui _dir = ${cfg.package.ui} (which is defined here https://github.com/NixOS/nixpkgs/blob/master/pkgs/servers/consul/ui.nix) to ui = true we'll have to adjust the build so that it actually builds the ui-v2 project and includes it in the consul static binary. (which requires yarn, and will require some patching on our side, just like vault).

@nh2

This comment has been minimized.

Copy link
Contributor

nh2 commented Oct 26, 2018

we'll have to adjust the build so that it actually builds the ui-v2 project and includes it in the consul static binary. (which requires yarn, and will require some patching on our side, just like vault).

@arianvp It doens't seem to need anything extra for me (see above): It works out of the box for me if I use

-    (if cfg.webUi then { ui_dir = "${cfg.package.ui}"; } else { }) //
+    (if cfg.webUi then { ui = true; } else { }) //
@arianvp

This comment has been minimized.

Copy link
Contributor

arianvp commented Oct 26, 2018

That's... very surprising. as buildGoPackage only does go specific things, and doesn't call the Makefile of the consul project which actually takes care of building the UI...

@nh2

This comment has been minimized.

Copy link
Contributor

nh2 commented Oct 26, 2018

We found the solution.

Apparently (and apparently starting with 1.2.0), for the purpose of bundling the UI into the binary, Consul started pre-building all UI stuff and putting it hex-encoded into hashicorp/consul@e875783#diff-a10b38c8ecb31208b7f65fa239f23226. They seem to generate that file with every release commit.

This means we can throw away my README and all Ruby related UI building stuff, and we only keep (if cfg.webUi then { ui = true; } else { })

@c0bw3b

This comment has been minimized.

Copy link
Contributor

c0bw3b commented Oct 26, 2018

Ok good! That would make the service module compatible with v1.3.0 with minimal changes.
-> Do we embed that change in this PR? (adding a commit to @vdemeester branch)

Also: the build failure on aarch64 seems to be just a thread number limitation on ofBorg ARM build server but if anyone can double check

@nh2 nh2 referenced this pull request Oct 26, 2018

Merged

consul: 0.9.3 -> 1.3.0 with vendored UI #49165

5 of 9 tasks complete
@nh2

This comment has been minimized.

Copy link
Contributor

nh2 commented Oct 26, 2018

Do we embed that change in this PR?

No, I made #49165 instead. We should close this.

@vdemeester

This comment has been minimized.

Copy link
Contributor Author

vdemeester commented Oct 26, 2018

Closing in favor of #49165 😉

@vdemeester vdemeester closed this Oct 26, 2018

@vdemeester vdemeester deleted the vdemeester:bump-consul branch Oct 26, 2018

nh2 added a commit to nh2/nixpkgs that referenced this pull request Nov 3, 2018

consul: 0.9.3 -> 1.3.0.
Removes the old UI build tooling; it is no longer necessary
because as of 1.2.0 it's bundled into the server binary.
It doesn't even need to have JS built, because it's bundled into
the release commit's source tree (see NixOS#48714).

The UI is enabled by default, so the NixOS service is
updated to directly use `ui = webUi;` now.

Fixes NixOS#48714.
Fixes NixOS#44192.
Fixes NixOS#41243.
Fixes NixOS#35602.

Signed-off-by: Niklas Hambüchen <mail@nh2.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.