[kudu-CR] [utility] auto-detection of cloud VM instance type
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14866 ) Change subject: [utility] auto-detection of cloud VM instance type .. [utility] auto-detection of cloud VM instance type Added a utility to auto-detect the type of VM instance for AWS, Azure and GCE public clouds. Follow-up changelists will make use of this functionality if auto-configuring the built-in NTP client upon startup of kudu-master and kudu-tserver. Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Reviewed-on: http://gerrit.cloudera.org:8080/14866 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/util/CMakeLists.txt A src/kudu/util/cloud/instance_detector-test.cc A src/kudu/util/cloud/instance_detector.cc A src/kudu/util/cloud/instance_detector.h A src/kudu/util/cloud/instance_metadata.cc A src/kudu/util/cloud/instance_metadata.h 6 files changed, 719 insertions(+), 13 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin
[kudu-CR] [utility] auto-detection of cloud VM instance type
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 ) Change subject: [utility] auto-detection of cloud VM instance type .. Patch Set 7: > Having reviewed the code again, I noticed that none of the instance > metadata subclasses actually need the expressivity that inheritance > offers. All the overridden methods are just pass-through "return > this flag", and the only one that deviates slightly (AWS's "get NTP > server" method) can be modeled as a passthrough "return nullptr". > > What do you think of combining all of the classes into something > more monolithic? I'm thinking: > - A "registry" of sorts (vector of structs) that declaratively > describes the properties of each cloud provider. I'm tempted to say > it's static, but if that prevents gflag overrides from working > properly, it can be constructed on the fly with each instantiation > of the instance detector (since that's a rare operation). > - That registry would be consumed at runtime by the instance > detector, which would create a detection thread for each entry in > the registry. > - The detection thread would monolithically include the curl logic > (because at this point there's no advantage to the separation). > > That should drastically reduce the amount of code/comments and make > everything easier to follow, at the cost of lack of flexibility if > down the line we do need the full expressivity of inheritance to > control per-cloud behavior. > > Leaving you a +2 in case you'd rather keep the current approach. > Having reviewed the code again, I noticed that none of the instance > metadata subclasses actually need the expressivity that inheritance > offers. All the overridden methods are just pass-through "return > this flag", and the only one that deviates slightly (AWS's "get NTP > server" method) can be modeled as a passthrough "return nullptr". > > What do you think of combining all of the classes into something > more monolithic? I'm thinking: > - A "registry" of sorts (vector of structs) that declaratively > describes the properties of each cloud provider. I'm tempted to say > it's static, but if that prevents gflag overrides from working > properly, it can be constructed on the fly with each instantiation > of the instance detector (since that's a rare operation). > - That registry would be consumed at runtime by the instance > detector, which would create a detection thread for each entry in > the registry. > - The detection thread would monolithically include the curl logic > (because at this point there's no advantage to the separation). > > That should drastically reduce the amount of code/comments and make > everything easier to follow, at the cost of lack of flexibility if > down the line we do need the full expressivity of inheritance to > control per-cloud behavior. > > Leaving you a +2 in case you'd rather keep the current approach. The original idea was to have library methods to work with a cloud instance's metadata, where the IP address/FQDN of the dedicated NTP server is just one of many other properties. I think the suggested alternative approach looks like a good fit for the purpose of just detecting the type of the cloud instance, and that's exactly what we use it for now :) I think I can go ahead and implement the suggested alternative approach, but make it a separate follow-up patch. -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Wed, 22 Jan 2020 19:19:45 + Gerrit-HasComments: No
[kudu-CR] [utility] auto-detection of cloud VM instance type
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 ) Change subject: [utility] auto-detection of cloud VM instance type .. Patch Set 7: Code-Review+2 Having reviewed the code again, I noticed that none of the instance metadata subclasses actually need the expressivity that inheritance offers. All the overridden methods are just pass-through "return this flag", and the only one that deviates slightly (AWS's "get NTP server" method) can be modeled as a passthrough "return nullptr". What do you think of combining all of the classes into something more monolithic? I'm thinking: - A "registry" of sorts (vector of structs) that declaratively describes the properties of each cloud provider. I'm tempted to say it's static, but if that prevents gflag overrides from working properly, it can be constructed on the fly with each instantiation of the instance detector (since that's a rare operation). - That registry would be consumed at runtime by the instance detector, which would create a detection thread for each entry in the registry. - The detection thread would monolithically include the curl logic (because at this point there's no advantage to the separation). That should drastically reduce the amount of code/comments and make everything easier to follow, at the cost of lack of flexibility if down the line we do need the full expressivity of inheritance to control per-cloud behavior. Leaving you a +2 in case you'd rather keep the current approach. -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Wed, 22 Jan 2020 18:12:56 + Gerrit-HasComments: No
[kudu-CR] [utility] auto-detection of cloud VM instance type
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 ) Change subject: [utility] auto-detection of cloud VM instance type .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/14866/5/src/kudu/util/cloud/instance_metadata.cc File src/kudu/util/cloud/instance_metadata.cc: http://gerrit.cloudera.org:8080/#/c/14866/5/src/kudu/util/cloud/instance_metadata.cc@175 PS5, Line 175: don't > doesn't Whoops. Done. -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Wed, 22 Jan 2020 18:09:09 + Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Bankim Bhavsar, Volodymyr Verovkin, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14866 to look at the new patch set (#7). Change subject: [utility] auto-detection of cloud VM instance type .. [utility] auto-detection of cloud VM instance type Added a utility to auto-detect the type of VM instance for AWS, Azure and GCE public clouds. Follow-up changelists will make use of this functionality if auto-configuring the built-in NTP client upon startup of kudu-master and kudu-tserver. Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/cloud/instance_detector-test.cc A src/kudu/util/cloud/instance_detector.cc A src/kudu/util/cloud/instance_detector.h A src/kudu/util/cloud/instance_metadata.cc A src/kudu/util/cloud/instance_metadata.h 6 files changed, 719 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/7 -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin
[kudu-CR] [utility] auto-detection of cloud VM instance type
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Bankim Bhavsar, Volodymyr Verovkin, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14866 to look at the new patch set (#6). Change subject: [utility] auto-detection of cloud VM instance type .. [utility] auto-detection of cloud VM instance type Added a utility to auto-detect the type of VM instance for AWS, Azure and GCE public clouds. Follow-up changelists will make use of this functionality if auto-configuring the built-in NTP client upon startup of kudu-master and kudu-tserver. Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/cloud/instance_detector-test.cc A src/kudu/util/cloud/instance_detector.cc A src/kudu/util/cloud/instance_detector.h A src/kudu/util/cloud/instance_metadata.cc A src/kudu/util/cloud/instance_metadata.h 6 files changed, 719 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/6 -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin
[kudu-CR] [utility] auto-detection of cloud VM instance type
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 ) Change subject: [utility] auto-detection of cloud VM instance type .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/14866/5/src/kudu/util/cloud/instance_metadata.cc File src/kudu/util/cloud/instance_metadata.cc: http://gerrit.cloudera.org:8080/#/c/14866/5/src/kudu/util/cloud/instance_metadata.cc@175 PS5, Line 175: don't doesn't -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Wed, 22 Jan 2020 17:31:42 + Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 ) Change subject: [utility] auto-detection of cloud VM instance type .. Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector-test.cc File src/kudu/util/cloud/instance_detector-test.cc: http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector-test.cc@83 PS4, Line 83: > To reduce possibility of flaky tests, can we go lower than this, FromNanose A few microsecond interval seems to be low enough because the interval covers spawning detector threads as well, and I ran it many 1K times in different configurations, not seeing any flakes. But I agree that MonoDelta::FromNanoseconds(1) is a better choice here. Done. http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h File src/kudu/util/cloud/instance_detector.h: http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@38 PS4, Line 38: s run b > running-> run Done http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@56 PS4, Line 56: n didn't reco > auto-detection Done http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@60 PS4, Line 60: ut was > typo Done http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@70 PS4, Line 70: e specified inst > I can't find this field in this change. Perhaps it was renamed. Good catch: this has changed, indeed. I updated it. http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.cc File src/kudu/util/cloud/instance_detector.cc: http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.cc@84 PS4, Line 84: us wakeups are ignored by the > An instance can't be running on multiple cloud vendors, so the first one to Done -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Wed, 22 Jan 2020 01:43:42 + Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Bankim Bhavsar, Volodymyr Verovkin, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14866 to look at the new patch set (#5). Change subject: [utility] auto-detection of cloud VM instance type .. [utility] auto-detection of cloud VM instance type Added a utility to auto-detect the type of VM instance for AWS, Azure and GCE public clouds. Follow-up changelists will make use of this functionality if auto-configuring the built-in NTP client upon startup of kudu-master and kudu-tserver. Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/cloud/instance_detector-test.cc A src/kudu/util/cloud/instance_detector.cc A src/kudu/util/cloud/instance_detector.h A src/kudu/util/cloud/instance_metadata.cc A src/kudu/util/cloud/instance_metadata.h 6 files changed, 719 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/5 -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin
[kudu-CR] [utility] auto-detection of cloud VM instance type
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 ) Change subject: [utility] auto-detection of cloud VM instance type .. Patch Set 4: Code-Review+1 (6 comments) http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector-test.cc File src/kudu/util/cloud/instance_detector-test.cc: http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector-test.cc@83 PS4, Line 83: FromMicroseconds To reduce possibility of flaky tests, can we go lower than this, FromNanoseconds()? http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h File src/kudu/util/cloud/instance_detector.h: http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@38 PS4, Line 38: running running-> run http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@56 PS4, Line 56: autodetection auto-detection http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@60 PS4, Line 60: cloude typo http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@70 PS4, Line 70: result_metadata_ I can't find this field in this change. Perhaps it was renamed. http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.cc File src/kudu/util/cloud/instance_detector.cc: http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.cc@84 PS4, Line 84: result_detector_idx_ == kNoIdx An instance can't be running on multiple cloud vendors, so the first one to update result_detector_idx_ wins the race and breaks the loop in case of spurious wake ups for other threads? Might be worth adding a comment. -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Tue, 21 Jan 2020 22:31:17 + Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 ) Change subject: [utility] auto-detection of cloud VM instance type .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc File src/kudu/util/cloud/instance_metadata.cc: http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@86 PS1, Line 86: "169.254.169.123" > Thank you for the feedback. It seems some extra clarification is needed he All right, I added gflags to be able to control these. -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Tue, 21 Jan 2020 20:35:22 + Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Bankim Bhavsar, Volodymyr Verovkin, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14866 to look at the new patch set (#4). Change subject: [utility] auto-detection of cloud VM instance type .. [utility] auto-detection of cloud VM instance type Added a utility to auto-detect the type of VM instance for AWS, Azure and GCE public clouds. Follow-up changelists will make use of this functionality if auto-configuring the built-in NTP client upon startup of kudu-master and kudu-tserver. Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/cloud/instance_detector-test.cc A src/kudu/util/cloud/instance_detector.cc A src/kudu/util/cloud/instance_detector.h A src/kudu/util/cloud/instance_metadata.cc A src/kudu/util/cloud/instance_metadata.h 6 files changed, 708 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/4 -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin
[kudu-CR] [utility] auto-detection of cloud VM instance type
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/14866 )
Change subject: [utility] auto-detection of cloud VM instance type
..
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.h
File src/kudu/util/cloud/instance_detector.h:
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.h@79
PS3, Line 79: struct DetectorInfo {
: std::unique_ptr metadata;
> Could combine into one vector with a struct.
Done
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.cc
File src/kudu/util/cloud/instance_detector.cc:
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.cc@62
PS3, Line 62: const auto deadline = MonoTime::Now() + timeout_;
> I think you could use a loop here, perhaps with a vectorhttp://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h
File src/kudu/util/cloud/instance_metadata.h:
PS1:
> That's true; I'm mostly worried about tests that do a fair amount of daemon
With a follow-up changelist, this feature is used by the external-mini-cluster
test and by the external_mini_cluster-test-test, so not many tests use it now.
--
To view, visit http://gerrit.cloudera.org:8080/14866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Gerrit-Change-Number: 14866
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin
Gerrit-Comment-Date: Tue, 21 Jan 2020 20:34:00 +
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/14866 )
Change subject: [utility] auto-detection of cloud VM instance type
..
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.h
File src/kudu/util/cloud/instance_detector.h:
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.h@79
PS3, Line 79: std::vector> threads_;
: std::vector> metadata_;
Could combine into one vector with a struct.
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.cc
File src/kudu/util/cloud/instance_detector.cc:
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.cc@62
PS3, Line 62: {
I think you could use a loop here, perhaps with a
vector, string>>?
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h
File src/kudu/util/cloud/instance_metadata.h:
PS1:
> Probably, but I think that's better to done in a separate changelist.
That's true; I'm mostly worried about tests that do a fair amount of daemon
restarting. Maybe we don't have very many of those though.
--
To view, visit http://gerrit.cloudera.org:8080/14866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Gerrit-Change-Number: 14866
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin
Gerrit-Comment-Date: Sat, 18 Jan 2020 05:47:17 +
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/14866 )
Change subject: [utility] auto-detection of cloud VM instance type
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/14866/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14866/2//COMMIT_MSG@9
PS2, Line 9: it
> Nit: a
Done
http://gerrit.cloudera.org:8080/#/c/14866/2/src/kudu/util/cloud/instance_detector.h
File src/kudu/util/cloud/instance_detector.h:
http://gerrit.cloudera.org:8080/#/c/14866/2/src/kudu/util/cloud/instance_detector.h@66
PS2, Line 66:
:
> Could you use our built-in synchronization primitives? Then you can use Mon
Done
http://gerrit.cloudera.org:8080/#/c/14866/2/src/kudu/util/cloud/instance_detector.h@68
PS2, Line 68:
> For production code use kudu::Thread which will be included in metrics and
Done
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h
File src/kudu/util/cloud/instance_metadata.h:
PS1:
> Continuing the conversation, are you going to attempt to persist the result
Probably, but I think that's better to done in a separate changelist.
>From the other hand, an extra second on start of kudu-{master,tserver} is
>nothing compared with hours loading the on-disk data structures and
>bootstrapping tablets. In addition, it would be necessary to provide an extra
>flag/tool to clean the persisted information. So, I'm still weighing pros and
>contras.
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc
File src/kudu/util/cloud/instance_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@86
PS1, Line 86: "169.254.169.123"
> Personally I don't mind the hardcoding. These addresses have been defined b
Thank you for the feedback. It seems some extra clarification is needed here:
I'll re-read the docs making sure there is a clear contract for keeping these
parameters constant.
--
To view, visit http://gerrit.cloudera.org:8080/14866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Gerrit-Change-Number: 14866
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin
Gerrit-Comment-Date: Sat, 18 Jan 2020 05:04:47 +
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Bankim Bhavsar, Volodymyr Verovkin, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14866 to look at the new patch set (#3). Change subject: [utility] auto-detection of cloud VM instance type .. [utility] auto-detection of cloud VM instance type Added a utility to auto-detect the type of VM instance for AWS, Azure and GCE public clouds. Follow-up changelists will make use of this functionality if auto-configuring the built-in NTP client upon startup of kudu-master and kudu-tserver. Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/cloud/instance_detector-test.cc A src/kudu/util/cloud/instance_detector.cc A src/kudu/util/cloud/instance_detector.h A src/kudu/util/cloud/instance_metadata.cc A src/kudu/util/cloud/instance_metadata.h 6 files changed, 657 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/3 -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin
[kudu-CR] [utility] auto-detection of cloud VM instance type
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 ) Change subject: [utility] auto-detection of cloud VM instance type .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/14866/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14866/2//COMMIT_MSG@9 PS2, Line 9: an Nit: a http://gerrit.cloudera.org:8080/#/c/14866/2/src/kudu/util/cloud/instance_detector.h File src/kudu/util/cloud/instance_detector.h: http://gerrit.cloudera.org:8080/#/c/14866/2/src/kudu/util/cloud/instance_detector.h@66 PS2, Line 66: std::condition_variable cv_; : std::mutex cv_m_; Could you use our built-in synchronization primitives? Then you can use MonoDelta for timeouts plus have some better instrumentation in DEBUG builds. http://gerrit.cloudera.org:8080/#/c/14866/2/src/kudu/util/cloud/instance_detector.h@68 PS2, Line 68: std::vector detector_threads_; For production code use kudu::Thread which will be included in metrics and the web UI. http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h File src/kudu/util/cloud/instance_metadata.h: PS1: > It's a bit unclear how this is going to be used effectively. At startup, we Continuing the conversation, are you going to attempt to persist the result of cloud detection to avoid the delay with every startup? http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc File src/kudu/util/cloud/instance_metadata.cc: http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@86 PS1, Line 86: uest_timeout() co > OK, if we want to have that for peace of mind, I can do that. IMHO, it's h Personally I don't mind the hardcoding. These addresses have been defined by the cloud vendors for years and like Alexey said, if they were to change, it's very likely that the rest of the logic in this file will have to change too (i.e. just changing the value of the IP address via gflag won't be enough). -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Sat, 18 Jan 2020 00:38:25 + Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 ) Change subject: [utility] auto-detection of cloud VM instance type .. Patch Set 2: > (4 comments) > > I'm assuming there isn't a generic open-source library available to > fetch such information, yet? I didn't find any particular that would fit. There are many things like https://github.com/dgzlopes/cloud-detect , https://github.com/banzaicloud/cloudinfo , but that's not what's needed in this context. Please let me know if you find anything that seems appropriate. -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Fri, 17 Jan 2020 23:27:10 + Gerrit-HasComments: No
[kudu-CR] [utility] auto-detection of cloud VM instance type
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/14866 )
Change subject: [utility] auto-detection of cloud VM instance type
..
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h
File src/kudu/util/cloud/instance_metadata.h:
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h@35
PS1, Line 35: InstanceMetadata
> Personally I don't think there's much to be gained by separating interface
+1 to Adar's point.
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h@55
PS1, Line 55: InvalidState
> You mean IllegalState?
Whoops. Yes, IllegalState.
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h@108
PS1, Line 108: avaiable
> available
Done
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc
File src/kudu/util/cloud/instance_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@40
PS1, Line 40: 500
> Alexey and I talked about this offline: the idea is for the Init() function
Yes, as Adar explained, that's the plan. The idea is to spawn multiple
detectors at once and collect the results: the first successful response before
timing out each and every detector should give us the result. We strive to
reduce the startup times as much as possible.
I added corresponding comment.
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@86
PS1, Line 86: "169.254.169.123"
> I agree with Vlad here. IMHO, IPs and URLs that are not in our control shou
OK, if we want to have that for peace of mind, I can do that. IMHO, it's
highly unlikely these are going to change without the need to rewrite the whole
implementation of retrieving the information from cloud metadata servers.
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@92
PS1, Line 92: std::
> std:: not needed in this .cc file here and other places, right?
Done
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@109
PS1, Line 109: Status AzureInstanceMetadata::GetNtpServer(string* server) const
{
> warning: parameter 'server' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@142
PS1, Line 142: "http://metadata.google.internal;
: "/computeMetadata/v1/instance/id"
> This kind of information should be stored in some configuration file.
I added more docs on each class, so hopefully it's clearer these properties are
not going to change ever.
--
To view, visit http://gerrit.cloudera.org:8080/14866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Gerrit-Change-Number: 14866
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin
Gerrit-Comment-Date: Fri, 17 Jan 2020 21:32:19 +
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Bankim Bhavsar, Volodymyr Verovkin, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14866 to look at the new patch set (#2). Change subject: [utility] auto-detection of cloud VM instance type .. [utility] auto-detection of cloud VM instance type Added an utility to auto-detect the type of VM instance for AWS, Azure and GCE public clouds. Follow-up changelists will make use of this functionality if auto-configuring the built-in NTP client upon startup of kudu-master and kudu-tserver. Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/cloud/instance_detector-test.cc A src/kudu/util/cloud/instance_detector.cc A src/kudu/util/cloud/instance_detector.h A src/kudu/util/cloud/instance_metadata.cc A src/kudu/util/cloud/instance_metadata.h 6 files changed, 626 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/2 -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin
