Skip to content

proto: force link missing v2 protos #9615

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

Merged
htuch merged 6 commits into from
Jan 10, 2020
Merged

proto: force link missing v2 protos #9615

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d08f0aa
proto: link HDS v2
Jan 9, 2020
61a4b8b
update
Jan 9, 2020
35c5e26
fmt
Jan 9, 2020
57e4556
forced into v2 by formatter
Jan 9, 2020
01af73e
linking issue
Jan 9, 2020
b3a2c26
fix link
Jan 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion source/common/config/protobuf_link_hacks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "envoy/api/v2/srds.pb.h"
#include "envoy/service/cluster/v3alpha/cds.pb.h"
#include "envoy/service/discovery/v2/ads.pb.h"
#include "envoy/service/discovery/v2/hds.pb.h"
#include "envoy/service/discovery/v2/rtds.pb.h"
#include "envoy/service/discovery/v2/sds.pb.h"
#include "envoy/service/discovery/v3alpha/ads.pb.h"
Expand Down Expand Up @@ -34,6 +35,7 @@ const envoy::api::v2::RdsDummy _rds_dummy_v2;
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;

@junr03 junr03 Jan 9, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@htuch I used an already defined message in the file.


const envoy::service::discovery::v3alpha::AdsDummy _ads_dummy_v3;
const envoy::service::ratelimit::v3alpha::RateLimitRequest _rls_dummy_v3;
Expand All @@ -43,5 +45,8 @@ const envoy::service::listener::v3alpha::LdsDummy _lds_dummy_v3;
const envoy::service::route::v3alpha::RdsDummy _rds_dummy_v3;
const envoy::service::cluster::v3alpha::CdsDummy _cds_dummy_v3;
const envoy::service::endpoint::v3alpha::EdsDummy _eds_dummy_v3;
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

@junr03 junr03 Jan 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@junr03 junr03 Jan 9, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @keith in case you have a second to take a look, and could provide any insight.

@htuch htuch Jan 9, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@junr03 junr03 Jan 9, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@htuch htuch Jan 9, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@junr03 junr03 Jan 9, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// health_discovery_service.h
} // namespace Envoy