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.

Sign up for GitHub

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

Jump to bottom

consul: 0.9.3 -> 1.3.0 #48714

Closed
wants to merge 1 commit into from
Closed

consul: 0.9.3 -> 1.3.0 #48714

wants to merge 1 commit into from

Conversation

vdemeester
Copy link
Member

@vdemeester vdemeester commented Oct 19, 2018 •
edited

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.

@vdemeester
consul: 0.9.3 -> 1.3.0
1a539e8 
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@c0bw3b
Copy link
Contributor

c0bw3b commented Oct 19, 2018 •
edited

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

This was referenced Oct 19, 2018
Closed
Closed
@c0bw3b
Copy link
Contributor

c0bw3b commented Oct 26, 2018

@GrahamcOfBorg build consul

@GrahamcOfBorg
Copy link

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
Copy link

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
Copy link
Contributor

nh2 commented Oct 26, 2018 •
edited

@c00w @vdemeester A couple questions:

Thanks!

@nh2
Copy link
Contributor

nh2 commented Oct 26, 2018

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

@arianvp
Copy link
Member

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
Copy link
Contributor

c0bw3b commented Oct 26, 2018

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

@arianvp
Copy link
Member

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
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
Copy link
Member

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
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
Copy link
Member

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
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:

nixpkgs/nixos/modules/services/networking/consul.nix

Line 10 in 1a539e8

(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
Copy link
Member

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
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
Copy link
Member

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
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 { })

This was referenced Oct 26, 2018
Closed
Closed
@c0bw3b
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 mentioned this pull request Oct 26, 2018
Merged
9 tasks
@nh2
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
Copy link
Member Author

Closing in favor of #49165 😉

@vdemeester vdemeester closed this Oct 26, 2018
@vdemeester vdemeester deleted the bump-consul branch October 26, 2018 16:42
nh2 added a commit to nh2/nixpkgs that referenced this pull request Nov 3, 2018
@nh2
consul: 0.9.3 -> 1.3.0.
2cb7f5f 
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
Reviewers
No reviews
Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vdemeester @c0bw3b @GrahamcOfBorg @nh2 @arianvp