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.

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

Merged
merged 17 commits into from
Jun 22, 2018
Merged

Conversation

ambuc
Copy link
Contributor

@ambuc ambuc commented Jun 21, 2018

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.

ambuc added 16 commits June 15, 2018 16:12
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>
Copy link
Member

@htuch htuch left a 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.

}
}

for (const auto& provider : getStaticRouteConfigProviders()) {
ASSERT(provider->configInfo());
config_dump->mutable_static_route_configs()->Add()->MergeFrom(
provider->configInfo().value().config_);

Copy link
Member

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

Copy link
Member

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_);

Copy link
Member

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

Copy link
Member

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

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.

@htuch htuch self-assigned this Jun 21, 2018
Signed-off-by: James Buckland <jbuckland@google.com>
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@htuch htuch merged commit a22d960 into envoyproxy:master Jun 22, 2018
@ambuc ambuc deleted the last-updated-ConfigDump branch June 22, 2018 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants