-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Last updated config dump #3691
Last updated config dump #3691
Conversation
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
…igDump Signed-off-by: James Buckland <jbuckland@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo tiny nits.
source/common/router/rds_impl.cc
Outdated
} | ||
} | ||
|
||
for (const auto& provider : getStaticRouteConfigProviders()) { | ||
ASSERT(provider->configInfo()); | ||
config_dump->mutable_static_route_configs()->Add()->MergeFrom( | ||
provider->configInfo().value().config_); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: more gratuitous blank line.
admin.getConfigTracker().add("clusters", [this] { return dumpClusterConfigs(); })) { | ||
admin.getConfigTracker().add("clusters", [this] { return dumpClusterConfigs(); })), | ||
system_time_source_(system_time_source) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: more gratuitous blank line.
std::make_unique<ClusterData>(cluster, version_info, added_via_api, std::move(new_cluster)); | ||
cluster_map[cluster_reference.info()->name()] = std::make_unique<ClusterData>( | ||
cluster, version_info, added_via_api, std::move(new_cluster), system_time_source_); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: more gratuitous blank line.
config_tracker_entry_(server.admin().getConfigTracker().add( | ||
"listeners", [this] { return dumpListenerConfigs(); })) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: more gratuitous blank line.
@@ -306,6 +331,12 @@ TEST_P(AdsIntegrationTest, Basic) { | |||
compareDiscoveryRequest(Config::TypeUrl::get().RouteConfiguration, "2", {"route_config_0"})); | |||
|
|||
makeSingleRequest(); | |||
ProtobufWkt::Timestamp first_active_listener_ts_2 = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Make all of these const
.
Signed-off-by: James Buckland <jbuckland@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Title: Add last_updated timestamp field to all XDSConfigDump protos.
Description:
This adds a new google.protobuf.Timestamp last_updated field to BootstrapConfigDump, ListenersConfigDump, ClustersConfigDump, and RoutesConfigDump, representing the time when the configs under that category were last updated.
Additionally, the ProdSystemTimeSource::instance_ singleton is passed through several layers of interfaces. This is to make testing easier, but I would love to find a workaround for injecting a MockSystemTimeSource closer to the point where it is needed in the tests.
Risk Level: Low
Testing:
Several tests were modified / updated to compare the dumped time (always 1234567890 seconds since epoch time) by using a MockSystemTimeSource which always returned 1234567890.
Recreated PR based off #3641.