ppc64le build #4183

clnperez · 2018-08-16T19:05:02Z

Build for Linux on Power (ppc64le). There is no luajit support for ppc64le, so
this PR puts in logic to not add that recipe to the sandbox speedup that
envoy uses for its build.

There were already some differences taken into account for Windows, so
hopefully this isn't too far out there.

--
Another alternative is to have subproject for Power that has its own
WORKSPACE file, like the ci, but in trying that route, I ran into the
issue of not being able to read the build_id in the bazel-out directory (up a level).

This way also has the advantage that you don't have to know to change
directories if you want to build for Power. And hopefully we can do the
same checks for ARM.

Signed-off-by: Christy Norman [email protected]

Description: Build for Linux on Power (ppc64le).
Risk Level: low
Testing: manual + have built this into istio/proxy for a few releases now
Docs Changes: Would need to add a bit about Power support
Release Notes: same

clnperez · 2018-08-16T19:06:15Z

Would love to hear thoughts on this. There are still some bazel issues to be worked out (e.g. the amd64 go binary is still pulled down), but hopefully this is a step in the right direction.

htuch

Nice to see this coming along, a couple of comments.

htuch · 2018-08-17T11:29:02Z

WORKSPACE

@@ -3,7 +3,8 @@ workspace(name = "envoy")
 load("//bazel:repositories.bzl", "envoy_dependencies")
 load("//bazel:cc_configure.bzl", "cc_configure")

-envoy_dependencies()
+PPC_SKIP_TARGETS=["luajit",]


I think it would be better to just inline this in repositories.bzl a a top-of-file constant; it probably won't change that much and should eventually shrink to zero. This simplifies the WORKSPACE by avoiding special casing.

htuch · 2018-08-17T11:31:39Z

source/extensions/all_extensions.bzl

+      if extension_path.find("lua") > 0:
+         luajit_path = extension_path
+   if luajit_path != '':
+      all_extensions.remove(luajit_path)


Ideally you use the suggested constant in repositories.bzl here (single source of truth). Also, I think this is something we'll want for other architectures/platforms (ARM), the ability to blacklist extensions. So, I would define a macro like: def is_blacklisted_extension(extension_name, extension_blacklist): to factor this logic out.

clnperez · 2018-08-17T21:52:21Z

source/extensions/all_extensions.bzl

+      if extension_path.find("lua") > 0:
+         luajit_path = extension_path
+   if luajit_path != '':
+      all_extensions.remove(luajit_path)


i cleaned that up a bit. since i'm still a bazel novice, i'm stuck on how to conditionally configure the skip_targets for envoy_dependencies, though. right now it's a check for the config_setting, but, that's a text string at that point. :) is there some way to query the target cpu that isn't a config setting?

@jmillikin-stripe can probably speak more to the best tricks here. I think the mechanism that Bazel supports best for this is select, e.g. see https://github.com/envoyproxy/envoy/blob/master/bazel/envoy_build_system.bzl#L30. How to combine this with blacklisting I'm not so sure of; it's really only suitable for building up sets/lists. You could make PPC a first class extensions.bzl target, like with Windows, at the expense of some boilerplate. This would work better I think.

yah, i had played with select(), but you can't put a select statement into anything called from a workspace file, so, i left the text string in for now as I'm stumped. If @jmillikin-stripe has a solution that would be awesome.

htuch · 2018-08-20T15:25:52Z

bazel/repositories.bzl

@@ -231,6 +233,11 @@ def _envoy_api_deps():
    )

 def envoy_dependencies(path = "@envoy_deps//", skip_targets = []):
+
+    skip_or_blacklist = depset() + skip_targets
+    if "//bazel:linux_ppc":


Yeah, this will always evaluate True today, since the semantics are the same as Python, i.e. True unless empty/None.

I'm not sure it's possible to use Bazel's built-in config settings at this point in the dependency resolution. You may need to do something messy to get this working until the Lua dependency can be Bazelified with the new C++ external deps work.

Maybe like this?

def _is_linux_ppc(): if ctx.os.name != "linux": return False res = ctx.execute(["uname", "-p"]) return "ppc" in res.stdout

jmillikin-stripe · 2018-08-20T17:08:13Z

WORKSPACE

@@ -13,4 +13,4 @@ load("@io_bazel_rules_go//go:def.bzl", "go_rules_dependencies", "go_register_too
 load("@com_lyft_protoc_gen_validate//bazel:go_proto_library.bzl", "go_proto_repositories")
 go_proto_repositories(shared=0)
 go_rules_dependencies()
-go_register_toolchains()
+go_register_toolchains(go_version="host")


This is going to do the wrong thing on most platforms, by using whatever random Go installation happens to exist in the environment instead of the hermetic versioned SDK downloaded by rules_go.

It looks like rules_go has supported detecting linux_ppc64le for several months (bazel-contrib/rules_go#1145), what does this particular change get us?

yes, this is definitely not going to stay, sorry. it's just easier for me to test.

that change you posted to rules_go may not be in the rules_go version envoy pulls in, b/c if i don't change this for us, it pulls down the amd64 go binary.

jmillikin-stripe · 2018-08-20T17:11:47Z

bazel/repositories.bzl

@@ -231,6 +233,11 @@ def _envoy_api_deps():
    )

 def envoy_dependencies(path = "@envoy_deps//", skip_targets = []):
+
+    skip_or_blacklist = depset() + skip_targets
+    if "//bazel:linux_ppc":


I'm not sure it's possible to use Bazel's built-in config settings at this point in the dependency resolution. You may need to do something messy to get this working until the Lua dependency can be Bazelified with the new C++ external deps work.

Maybe like this?

def _is_linux_ppc(): if ctx.os.name != "linux": return False res = ctx.execute(["uname", "-p"]) return "ppc" in res.stdout

clnperez · 2018-08-20T18:42:05Z

Maybe like this?

def _is_linux_ppc():
  if ctx.os.name != "linux":
    return False
  res = ctx.execute(["uname", "-p"])
  return "ppc" in res.stdout

that would work for a plain build, but i know it wouldn't work for cross-compiling. right now, as far as i know, cross-compiling isn't a good option for us (still c-go problems?) but, that's why i didn't that would be the best route. but if it's all we've got right now, i'm fine with it.

clnperez · 2018-08-20T18:51:16Z

wait, what's this? the new C++ external deps work :D

jmillikin-stripe · 2018-08-20T19:36:13Z

that would work for a plain build, but i know it wouldn't work for cross-compiling. right now, as far as i know, cross-compiling isn't a good option for us (still c-go problems?) but, that's why i didn't that would be the best route. but if it's all we've got right now, i'm fine with it.

Cross-compilation of the "build recipes" deps is very difficult, and I don't think this particular check would make it any worse.

wait, what's this? the new C++ external deps work :D

@htuch and I have been working with the Bazel devs on a way to merge the rest of the "build recipes" external deps into the Bazel-managed set. This should improve many parts of the Envoy build on non-default platforms.

clnperez · 2018-08-21T15:08:33Z

not sure what to do about the format failure. @htuch?

htuch · 2018-08-22T15:41:07Z

@clnperez you can run fix_format under Docker, e.g. ./ci/run_envoy_docker.sh './ci/do_ci.sh fix_format'

htuch

Looks good, hoping Bazel team can weight in on if this is the cleanest we can do. I'd like to merge to unblock your work here soon though.

htuch · 2018-08-22T15:43:35Z

source/extensions/all_extensions.bzl

+
+def is_blacklisted_extension(extension, blacklist):
+    for be in blacklist:
+        if be in extension:


This tests for substring search, is this really what you want? Do you want equality, suffix match, etc.? Seems too weak as is.

I agree. Not the best. I was worried about potential shared properties in names, e.g. envoy.a.x and envoy.a.y, where maybe x would be really common, like net, but really, you wanted to filter out a.x. In that case, I was envisioning that you'd have to add a.x to your blacklist (though that would break the name match from TARGET_RECIPES, but I think you could just rename whatever it is in TARGET_RECIPES if you need to go that route?).

I could only match the last token:

def is_blacklisted_extension(extension, blacklist): for be in blacklist: if extension.split(".")[-1] == be: return True return False

Can you just make it an exact match?

You mean have the blacklist contain the entire string (e.g. envoy.a.x)?

Sure, doesn't seem too bad.

sorry -- got sucked into 10 new things this week.

If we did that instead of the shorter name, it would mess up _build_recipe_repository_impl, which uses the short name for the recipe script match.
Another option is to do that in addition to the other ones. so the list would have two-three different required matches for everything we want to skip, e.g.
PPC_SKIP_TARGETS=["lua", "luajit", "envoy.filters.http.lua"] but then I think we still have the potential to run into the original problem I was worried about, since the shorter ones need to stay.

Actually, since the shorter ones wouldn't match in this usage of that list, I think that would be fine. But gross, with all those different representations of the same thing.

In this situation I would have something like:

PPC_SKIP_SPECIFIERS=[("lua", "envoy.filters.http.lua"), ...]

i.e. have some structured format with both short and long form for respective entries, and use whichever at the relevant spot.

ah, nice. i redid with a dict. in one place i check keys. in another i check values. thanks for that! lmk what you think @htuch

htuch · 2018-08-22T15:46:07Z

bazel/repositories.bzl

+    if ctxt.os.name != "linux":
+        return False
+    res = ctxt.execute(["uname", "-m"])
+    return "ppc" in res.stdout


@dslomov is this the best practice we can use right now for achieving this? Any thoughts on how we can condition external dependencies in a cleaner way?

htuch · 2018-09-03T13:36:09Z

I'm pretty happy with this now @clnperez, thanks. Can you fix_format and merge master, update title and commit message at the top of this PR?

clnperez · 2018-09-04T15:32:42Z

should be g2g now @htuch

clnperez · 2018-09-04T15:34:39Z

@htuch i missed the part about the commit message. What should I change there?

Build for Linux on Power (ppc64le). There is no luajit support for ppc64le, so this PR puts in logic to not add that recipe to the sandbox speedup that envoy uses for its build. There were already some differences taken into account for Windows, so hopefully this isn't too far out there. -- Another alternative is to have subproject for Power that has its own WORKSPACE file, like the ci, but in trying that route, I ran into the issue of not being able to read the build_id in the bazel-out directory (up a level). This way also has the advantage that you don't have to know to change directories if you want to build for Power. And hopefully we can do the same checks for ARM. Signed-off-by: Christy Norman <[email protected]>

htuch

LGTM, thanks!

Pulling the following changes from github.com/envoyproxy/envoy: f936fc6 ssl: serialize accesses to SSL socket factory contexts (envoyproxy#4345) e34dcd6 Fix crash in tcp_proxy (envoyproxy#4323) ae6a252 router: fix matching when all domains have wildcards (envoyproxy#4326) aa06142 test: Stop fake_upstream methods from accidentally succeeding (envoyproxy#4232) 5d73187 rbac: update the authenticated.user to a StringMatcher. (envoyproxy#4250) c6bfc7d time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (envoyproxy#4257) 752483e Fixing the fix (envoyproxy#4333) 83487f6 tls: update BoringSSL to ab36a84b (3497). (envoyproxy#4338) 7bc210e test: fixing interactions between waitFor and ignore_spurious_events (envoyproxy#4309) 69474b3 admin: order stats in clusters json admin (envoyproxy#4306) 2d155f9 ppc64le build (envoyproxy#4183) 07efc6d fix static initialization fiasco problem (envoyproxy#4314) 0b7e3b5 test: Remove declared but undefined class methods (envoyproxy#4297) 1485a13 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd6 test: set to zero when start_time exceeds limit (envoyproxy#4328) 0a1e92a test: fix heap use-after-free in ~IntegrationTestServer. (envoyproxy#4319) cddc732 CONTRIBUTING: Document 'kick-ci' trick. (envoyproxy#4335) f13ef24 docs: remove reference to deprecated value field (envoyproxy#4322) e947a27 router: minor doc fixes in stream idle timeout (envoyproxy#4329) 0c2e998 tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (envoyproxy#4296) 00ffe44 utility: fix strftime overflow handling. (envoyproxy#4321) af1183c Re-enable TcpProxySslIntegrationTest and make the tests pass again. (envoyproxy#4318) 3553461 fuzz: fix H2 codec fuzzer post envoyproxy#4262. (envoyproxy#4311) 42f6048 Proto string issue fix (envoyproxy#4320) 9c492a0 Support Envoy to fetch secrets using SDS service. (envoyproxy#4256) a857219 ratelimit: revert `revert rate limit failure mode config` and add tests (envoyproxy#4303) 1d34172 dns: fix exception unsafe behavior in c-ares callbacks. (envoyproxy#4307) 1212423 alts: add gRPC TSI socket (envoyproxy#4153) f0363ae fuzz: detect client-side resets in H2 codec fuzzer. (envoyproxy#4300) 01aa3f8 test: hopefully deflaking echo integration test (envoyproxy#4304) 1fc0f4b ratelimit: link legacy proto when message is being used (envoyproxy#4308) aa4481e fix rare List::remove(&target) segfault (envoyproxy#4244) 89e0f23 headers: fixing fast fail of size-validation (envoyproxy#4269) 97eba59 build: bump googletest version. (envoyproxy#4293) 0057e22 fuzz: avoid false positives in HCM fuzzer. (envoyproxy#4262) 9d094e5 Revert ac0bd74 (envoyproxy#4295) ddb28a4 Add validation context provider (envoyproxy#4264) 3b47cba added histogram latency information to Hystrix dashboard stream (envoyproxy#3986) cf87d50 docs: update SNI FAQ. (envoyproxy#4285) f952033 config: fix update empty stat for eds (envoyproxy#4276) 329e591 router: Add ability of custom headers to rely on per-request data (envoyproxy#4219) 68d20b4 thrift: refactor build files and imports (envoyproxy#4271) 5fa8192 access_log: log requested_server_name in tcp proxy (envoyproxy#4144) fa45bb4 fuzz: libc++ clocks don't like nanos. (envoyproxy#4282) 53f8944 stats: add symbol table for future stat name encoding (envoyproxy#3927) c987b42 test infra: Remove timeSource() from the ClusterManager api (envoyproxy#4247) cd171d9 websocket: tunneling websockets (and upgrades in general) over H2 (envoyproxy#4188) b9dc5d9 router: disallow :path/host rewriting in request_headers_to_add. (envoyproxy#4220) 0c91011 network: skip socket options and source address for UDS client connections (envoyproxy#4252) da1857d build: fixing a downstream compile error by noting explicit fallthrough (envoyproxy#4265) 9857cfe fuzz: cleanup per-test environment after each fuzz case. (envoyproxy#4253) 52beb06 test: Wrap proto string in std::string before comparison (envoyproxy#4238) f5e219e extensions/thrift_proxy: Add header matching to thrift router (envoyproxy#4239) c9ce5d2 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (envoyproxy#4260) 35103b3 fuzz: use nanoseconds for SystemTime in RequestInfo. (envoyproxy#4255) ba6ba98 fuzz: make runtime root hermetic in server_fuzz_test. (envoyproxy#4258) b0a9014 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (envoyproxy#4248) 8567460 access_log: support beginning of epoch in START_TIME. (envoyproxy#4254) 28d5f41 proto: unify envoy_proto_library/api_proto_library. (envoyproxy#4233) f7d3cb6 http: fix allocation bug introduced in envoyproxy#4211. (envoyproxy#4245) Fixes istio/istio#8310 (once pulled into istio/istio). Signed-off-by: Piotr Sikora <[email protected]>

Build for Linux on Power (ppc64le). There is no luajit support for ppc64le, so this PR puts in logic to not add that recipe to the sandbox speedup that envoy uses for its build. There were already some differences taken into account for Windows, so hopefully this isn't too far out there. Description: Build for Linux on Power (ppc64le). Risk Level: low Testing: manual + have built this into istio/proxy for a few releases now Docs Changes: Would need to add a bit about Power support Release Notes: same [[email protected]] - rebase for whitespaces changes; - extend the same mechanism to also apply to AArch64; Signed-off-by: Christy Norman <[email protected]> Signed-off-by: Alexandru Avadanii <[email protected]>

mattklein123 requested review from jmillikin-stripe and htuch August 16, 2018 21:21

mattklein123 assigned htuch Aug 16, 2018

htuch suggested changes Aug 17, 2018

View reviewed changes

clnperez force-pushed the build-ppc64le-inband branch 2 times, most recently from 591aeac to 76697b2 Compare August 17, 2018 21:46

clnperez commented Aug 17, 2018

View reviewed changes

htuch reviewed Aug 20, 2018

View reviewed changes

jmillikin-stripe reviewed Aug 20, 2018

View reviewed changes

clnperez force-pushed the build-ppc64le-inband branch from 76697b2 to 0712550 Compare August 20, 2018 22:46

htuch reviewed Aug 22, 2018

View reviewed changes

clnperez force-pushed the build-ppc64le-inband branch from 0712550 to 7617d99 Compare August 22, 2018 17:52

clnperez force-pushed the build-ppc64le-inband branch from 7617d99 to a016177 Compare August 30, 2018 22:12

clnperez changed the title ~~WIP: ppc64le build~~ ppc64le build Sep 4, 2018

clnperez force-pushed the build-ppc64le-inband branch from a016177 to 4931901 Compare September 4, 2018 15:32

clnperez force-pushed the build-ppc64le-inband branch from 4931901 to e83895d Compare September 4, 2018 15:33

clnperez force-pushed the build-ppc64le-inband branch from e83895d to 064c396 Compare September 4, 2018 15:49

htuch approved these changes Sep 4, 2018

View reviewed changes

htuch merged commit 2d155f9 into envoyproxy:master Sep 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ppc64le build #4183

ppc64le build #4183

clnperez commented Aug 16, 2018

clnperez commented Aug 16, 2018

htuch left a comment

htuch Aug 17, 2018

htuch Aug 17, 2018

clnperez Aug 17, 2018

htuch Aug 20, 2018

clnperez Aug 20, 2018

htuch Aug 20, 2018

jmillikin-stripe Aug 20, 2018

jmillikin-stripe Aug 20, 2018

clnperez Aug 20, 2018

jmillikin-stripe Aug 20, 2018

clnperez commented Aug 20, 2018 •

edited

Loading

clnperez commented Aug 20, 2018

jmillikin-stripe commented Aug 20, 2018

clnperez commented Aug 21, 2018

htuch commented Aug 22, 2018

htuch left a comment

htuch Aug 22, 2018

clnperez Aug 22, 2018

htuch Aug 22, 2018

clnperez Aug 23, 2018

htuch Aug 23, 2018

clnperez Aug 29, 2018

clnperez Aug 29, 2018

htuch Aug 29, 2018

clnperez Aug 30, 2018

htuch Aug 22, 2018

htuch commented Sep 3, 2018

clnperez commented Sep 4, 2018

clnperez commented Sep 4, 2018

htuch left a comment

ppc64le build #4183

ppc64le build #4183

Conversation

clnperez commented Aug 16, 2018

clnperez commented Aug 16, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clnperez commented Aug 20, 2018 • edited Loading

clnperez commented Aug 20, 2018

jmillikin-stripe commented Aug 20, 2018

clnperez commented Aug 21, 2018

htuch commented Aug 22, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Sep 3, 2018

clnperez commented Sep 4, 2018

clnperez commented Sep 4, 2018

htuch left a comment

Choose a reason for hiding this comment

clnperez commented Aug 20, 2018 •

edited

Loading