This repository was archived by the owner on Aug 19, 2019. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 11
Add validation to only start updaters if they're configured properly. #63
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
9886f3e
Add validation to only initiate pollers if configured properly.
bmoyles0117 ca3b09c
Move ValidateConfiguration to readers and updaters.
bmoyles0117 a445d3f
Add name property to updater.
bmoyles0117 89d6cf8
Make the reader an updater property to validate configuration.
bmoyles0117 5bd4add
Address feedback.
bmoyles0117 2b99257
Revert "Make the reader an updater property to validate configuration."
bmoyles0117 05fbbfb
Revert Reader base class and add virtual [Start|Stop]Updater methods.
bmoyles0117 a333634
Limit Docker container request to one container, and validate node
bmoyles0117 3e56c5c
Address feedback.
bmoyles0117 475c035
Address feedback.
bmoyles0117 812559c
Address feedback.
bmoyles0117 9b37540
Address feedback.
bmoyles0117 c5fbc29
Address feedback.
bmoyles0117 275bfbb
Address feedback.
bmoyles0117 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -961,6 +961,33 @@ json::value KubernetesReader::FindTopLevelOwner( | |
| return FindTopLevelOwner(ns, GetOwner(ns, ref->As<json::Object>())); | ||
| } | ||
|
|
||
| bool KubernetesReader::ValidateConfiguration() const { | ||
| try { | ||
| (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/nodes?limit=1"); | ||
| } catch (const QueryException& e) { | ||
| // Already logged. | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| const std::string pod_label_selector( | ||
| config_.KubernetesPodLabelSelector().empty() | ||
| ? "" : "&" + config_.KubernetesPodLabelSelector()); | ||
|
|
||
| (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1" + | ||
| pod_label_selector); | ||
| } catch (const QueryException& e) { | ||
| // Already logged. | ||
| return false; | ||
| } | ||
|
|
||
| if (CurrentNode().empty()) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| void KubernetesReader::PodCallback( | ||
| MetadataUpdater::UpdateCallback callback, | ||
| const json::Object* pod, Timestamp collected_at, bool is_deleted) const | ||
|
|
@@ -1034,8 +1061,15 @@ void KubernetesReader::WatchNode(MetadataUpdater::UpdateCallback callback) | |
| LOG(INFO) << "Watch thread (node) exiting"; | ||
| } | ||
|
|
||
| void KubernetesUpdater::start() { | ||
| PollingMetadataUpdater::start(); | ||
| bool KubernetesUpdater::ValidateConfiguration() const { | ||
| if (!PollingMetadataUpdater::ValidateConfiguration()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this not also be in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forgot about |
||
| return false; | ||
| } | ||
|
|
||
| return reader_.ValidateConfiguration(); | ||
| } | ||
|
|
||
| void KubernetesUpdater::StartUpdater() { | ||
| if (config().KubernetesUseWatch()) { | ||
| // Wrap the bind expression into a function to use as a bind argument. | ||
| UpdateCallback cb = std::bind(&KubernetesUpdater::MetadataCallback, this, | ||
|
|
@@ -1044,6 +1078,9 @@ void KubernetesUpdater::start() { | |
| std::thread(&KubernetesReader::WatchNode, &reader_, cb); | ||
| pod_watch_thread_ = | ||
| std::thread(&KubernetesReader::WatchPods, &reader_, cb); | ||
| } else { | ||
| // Only try to poll if watch is disabled. | ||
| PollingMetadataUpdater::StartUpdater(); | ||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Same question as for Docker -- can we confirm that the first
limit=1will take precedence over whatever is in the selector?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.
pod_label_selectordoes not imply passing a limit, it implies passing labels, if a user is passing a limit there, that's a hack, and should be validated elsewhere.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.
I agree that it's a hack, but a
limit=500000in the (user-specified) selector should not cause us to suddenly grab all the pods in the cluster. Hence I was just asking whether thislimit=1will override it where it is now, or whether it should be at the end.That said, I've just verified that the first occurrence of
limittakes precedence, so we're good.