proto: force link missing v2 protos#9615
proto: force link missing v2 protos#9615htuch merged 6 commits intoenvoyproxy:masterfrom junr03:hds-link
Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
| const envoy::api::v2::CdsDummy _cds_dummy_v2; | ||
| const envoy::api::v2::EdsDummy _eds_dummy_v2; | ||
| const envoy::api::v2::SrdsDummy _srds_dummy_v2; | ||
| const envoy::service::discovery::v2::Capability _hds_dummy_v2; |
There was a problem hiding this comment.
@htuch I used an already defined message in the file.
| const envoy::service::route::v3alpha::SrdsDummy _srds_dummy_v4; | ||
| const envoy::service::route::v3alpha::SrdsDummy _srds_dummy_v3; | ||
|
|
||
| // note: hds force linking is not needed because it is still implicitly linked via |
There was a problem hiding this comment.
Actually nevermind. According to #9210 the fact that the v3alpha descriptor is present should be enough to link in the v2 descriptor. So this PR was indeed indicative of a larger issue.
#9191's issue with alwayslink in iOS is at play here. The iOS build of Envoy Mobile fails at runtime trying to find the descriptors for hds, the metrics service (both the config type and the service method), and more importantly the v2 Bootstrap type. However, for example, the v2 Cluster type which is still being used in the codebase is present. This means that descriptors for types that are not being used in the codebase are not being linked.
I fixed locally with this change and by forcing bootstrap v2 linking in this file, and forcing metrics service linking in the extension. This reinforces my explanation above.
@goaway I think this (specially the fact that the v2 Bootstrap descriptor was missing) disproves the working theory that this is a latent problem of implicit, transitive inclusion that is absent in larger build configurations. It seems to point squarely in the direction that for some reason our builds are causing the alwayslink that is in all proto packages in Envoy to not be respected.
@htuch lmk what you think.
There was a problem hiding this comment.
cc @keith in case you have a second to take a look, and could provide any insight.
There was a problem hiding this comment.
Yeah, TBH, I think I'm still confused. I did end up needing to add those dummy messages, despite the claim in protocolbuffers/protobuf#4221 that if we set always_link, it shouldn't be needed. This was for a normal Linux build.
However, this only applies to the "proto file with no messages and only service definitions" scenario. What you are describing is different, thing like bootstrap shouldn't be an issue. I think there is a deeper iOS side issue, in addition to the service definition only link issue.
There was a problem hiding this comment.
What I am thinking is that:
What you observed is most likely due to the linker dropping unused symbols (i.e., the descriptor registering code is not reachable from your application code so they got removed by linker)
is happening to v2 types for headers that are not explicitly included/used in the Envoy codebase anymore; even though they should be linked because their build targets are included in the v3 bazel targets, and all of them have always_link. Hence, we I explicitly use them in the codebase again, even just to declare a dummy variable they are not dropped.
There was a problem hiding this comment.
Yeah, for bootstrap (for example), the v3alpha proto depends on the v2. It would be good to understand why the linker is doing this to avoid whack-a-mole.
There was a problem hiding this comment.
I agree that we should understand what the linker is doing in order to avoid whack-a-mole as we include fewer and fewer v2 headers. For the time being, I pushed a potential stopgap addition to the hacks to get us (Envoy Mobile unblocked). We can investigate the linker internally as it is obvious now that the problem is wider than previously thought. wdyt?
| // which is checked for by proto_descriptors.cc. This type **is** getting linked because its headers | ||
| // is still included in cds_api_impl.cc. On the other side because the v2 hds header is not included | ||
| // anywhere the v2 service type is getting dropped, and thus the descriptor is not present in the | ||
| // descriptor pool. |
There was a problem hiding this comment.
Can you include a link to the tracking issue here?
There was a problem hiding this comment.
yep, was waiting to see what you thought. Will create and link now.
| namespace MetricsService { | ||
|
|
||
| void validateProtoDescriptors() { | ||
| const envoy::service::metrics::v2::StreamMetricsMessage _dummy_v2; |
There was a problem hiding this comment.
Can this be in protobuf_link_hacks? That way we can centralize this and more easily cleanup later.
There was a problem hiding this comment.
I don't know all the intricacies of the extensions build system but I thought that because this was an extension I would leave this under the extensions tree. I'll put a link to the tracking issue once I create it.
Signed-off-by: Jose Nino <jnino@lyft.com>
2 participants
Description: some v2 symbols required at runtime are being dropped under certain circumstances. This PR adds force linking under the protobuf_link_hacks.h file as a stop-gap while we investigate a long term solution. More detail in #9639
Risk Level: low
Testing: compiled with Envoy Mobile's iOS dist target which, as discussed here, requires the hack currently in place.
Signed-off-by: Jose Nino jnino@lyft.com