Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.

Bump the GCE metadata server timeout to 2s. #114

Merged
igorpeshansky merged 1 commit into from
Mar 31, 2018
+1 −1
Merged

Bump the GCE metadata server timeout to 2s. #114

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/environment.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ Environment::Environment(const Configuration& config)

std::string Environment::GetMetadataString(const std::string& path) const {
http::client::options options;
http::client client(options.timeout(1));
http::client client(options.timeout(2));
Copy link
Contributor

@bmoyles0117 bmoyles0117 Mar 31, 2018

Choose a reason for hiding this comment

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

This would require more changes, but can we set this to be a configuration value instead? I'm hoping 2 is adequate for now, but should the metadata latency increase again, it would be nice to make a simple PSA to update the timeout, instead of requiring an entirely new binary.

Copy link
Contributor Author

@igorpeshansky igorpeshansky Mar 31, 2018

Choose a reason for hiding this comment

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

I've thought about this too, but it seems like this will not be a regular occurrence, and I would rather not add gratuitous options. Besides, in the Kubernetes world, a config bump and a binary bump require about the same amount of effort (i.e., a PR with a config change). WDYT?

Copy link
Contributor

@dhrupadb dhrupadb Mar 31, 2018

Choose a reason for hiding this comment

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

+1 to what Igor said. My argument against that is that this is only a timeout upper bound. If the metadata server timeout starts taking longer than say ~5 seconds we will have other bigger changes to potentially consider.

Copy link
Contributor

@bmoyles0117 bmoyles0117 Mar 31, 2018

Choose a reason for hiding this comment

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

I argue a user should have a temporary remedy as an option, with the current approach it requires the user to be broken until we ship out a fix. Updating a configmap / command line arg is not a big deal, compared to being blocked by a GCP bug while they figure out how to reduce their latency. Giving the user an option to become unblocked is better for our users.

Copy link
Contributor Author

@igorpeshansky igorpeshansky Mar 31, 2018

Choose a reason for hiding this comment

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

We have plenty of other timeouts in the code that aren't controllable via options. This one is so minor, that I would rather not bring it up to our users' attention. As Dhrupad says, if the metadata server takes that long, something else is terribly broken.

http::client::request request(kGceMetadataServerAddress + path);
request << boost::network::header("Metadata-Flavor", "Google");
try {
Expand Down