Update the FindTopLevelOwner function to extract only controllers. by supriyagarg · Pull Request #95 · Stackdriver/metadata-agent

supriyagarg · 2018-03-28T01:40:34Z

No description provided.

igorpeshansky · 2018-03-28T02:18:51Z

+  LOG(DEBUG) << "FindTopLevelController: refs is " << *refs;
 #endif
-  if (refs->size() > 1) {
+  std::vector<json::Object> controller_refs;


Let's not copy JSON values. Given that the refs array will live to the end of this function, I would suggest just storing const json::Object pointers in the array.

igorpeshansky · 2018-03-28T02:20:26Z

+    return object;
+  }
+  if (controller_refs.size() > 1) {
    LOG(WARNING) << "Found multiple owner references for " << *obj


s/owner/controller/, right?

correct. done.

igorpeshansky · 2018-03-28T02:21:34Z

-  json::value FindTopLevelOwner(const std::string& ns, json::value object) const
+  // For a given object, returns the top-level controller object.
+  // When there are multiple controller references, follows the first one.
+  json::value FindTopLevelController(const std::string& ns, json::value object) const


Let's fit this in 80 characters again.

igorpeshansky · 2018-03-28T02:22:16Z

  if (!metadata->Has("ownerReferences")) {
 #ifdef VERBOSE
-    LOG(DEBUG) << "FindTopLevelOwner: no owner references in " << *metadata;
+    LOG(DEBUG) << "FindTopLevelController: no owner references in " << *metadata;


Let's fit in 80 characters (line break before the <<).

igorpeshansky · 2018-03-28T02:24:06Z

+  std::vector<json::Object> controller_refs;
+  for (const json::value& ref : *refs) {
+    const json::Object* ref_obj = ref->As<json::Object>();
+    if (ref_obj->Has("controller") && ref_obj->Get<json::Boolean>("controller")) {


Let's fit this in 80 characters again.

igorpeshansky · 2018-03-28T02:24:40Z

+#endif
+  if (controller_refs.size() == 0) {
+#ifdef VERBOSE
+    LOG(DEBUG) << "FindTopLevelController: no controller owner references in " << *metadata;


Let's fit in 80 characters (line break before the <<).

bmoyles0117 · 2018-03-28T07:53:58Z

+#endif
+    return object;
+  }
+  if (controller_refs.size() > 1) {


Being that we're specifically looking for controllers now, this is from the Kubernetes documentation https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#objectmeta-v1-meta.

"There cannot be more than one managing controller."

How do you feel about removing this check?

Correct - this was more for defensive programming, just in case.

If we want to ignore any controller after the first one, I can simplify the for loop above to store just the first controller, and break after it finds one. WDYT?

Yes, let's just declare a single const json::Object* controller_ref = nullptr with a comment that Kubernetes objects are supposed to have at most one and a link to the doc that Bryan referenced. Then break out of the loop when you find one.

igorpeshansky · 2018-03-28T12:15:09Z

+    const json::Object* ref_obj = ref->As<json::Object>();
+    if (ref_obj->Has("controller") &&
+        ref_obj->Get<json::Boolean>("controller")) {
+      controller_refs.emplace_back(ref_obj);


Just push_back should be sufficient now.

igorpeshansky · 2018-03-28T12:16:10Z

                 << " picking the first one arbitrarily.";
  }
-  const json::value& ref = (*refs)[0];
+  const json::Object controller_ref = *controller_refs[0];


The comment about not copying JSON values applies here too. Just pass the pointers around.

this comment is no longer applicable.

igorpeshansky · 2018-03-28T12:17:52Z

+  // For a given object, returns the top-level controller object.
+  // When there are multiple controller references, follows the first one.
+  json::value FindTopLevelController(
+      const std::string& ns, json::value object) const


How about:

json::value FindTopLevelController(const std::string& ns, json::value object) const throw(QueryException, json::Exception);

?

igorpeshansky · 2018-03-28T13:42:24Z

+#endif
+    return object;
+  }
+  if (controller_refs.size() > 1) {


Yes, let's just declare a single const json::Object* controller_ref = nullptr with a comment that Kubernetes objects are supposed to have at most one and a link to the doc that Bryan referenced. Then break out of the loop when you find one.

igorpeshansky

A couple of comments.

igorpeshansky · 2018-03-28T14:37:26Z

    return object;
  }
  const json::Array* refs = metadata->Get<json::Array>("ownerReferences");
+


Let's move this blank line to after the #endif (so the log message is next to the declaration).

igorpeshansky · 2018-03-28T14:39:22Z

-  json::value FindTopLevelOwner(const std::string& ns, json::value object) const
-      throw(QueryException, json::Exception);
+  // For a given object, returns the top-level controller object.
+  // When there are multiple controller references, follows the first one.


This is no longer true.

It implicitly is - but yes confusing. Removed the line.

igorpeshansky

LGTM

bmoyles0117

LGTM

Update the FindTopLevelOwner function to extract only controllers.

2f24e40

supriyagarg assigned bmoyles0117 and igorpeshansky Mar 28, 2018

supriyagarg requested review from bmoyles0117 and igorpeshansky March 28, 2018 01:41

supriyagarg assigned supriyagarg and unassigned bmoyles0117 and igorpeshansky Mar 28, 2018

igorpeshansky suggested changes Mar 28, 2018

View reviewed changes

supriyagarg force-pushed the controller_fix branch from 61aa635 to fd148ce Compare March 28, 2018 03:13

Reformat long lines and use vector of <const json::Object*>

00bd7c2

supriyagarg force-pushed the controller_fix branch from fd148ce to 00bd7c2 Compare March 28, 2018 03:16

bmoyles0117 reviewed Mar 28, 2018

View reviewed changes

igorpeshansky suggested changes Mar 28, 2018

View reviewed changes

Update controller logic to only extract the first controller.

0159d57

igorpeshansky suggested changes Mar 28, 2018

View reviewed changes

Address comments

5b6ca70

igorpeshansky approved these changes Mar 28, 2018

View reviewed changes

bmoyles0117 approved these changes Mar 28, 2018

View reviewed changes

supriyagarg merged commit 6a72500 into Stackdriver:master Mar 28, 2018

supriyagarg deleted the controller_fix branch March 28, 2018 22:37

Conversation

supriyagarg commented Mar 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

bmoyles0117 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants