Fix container spec/status order mismatch. #47

igorpeshansky · 2017-12-21T15:20:40Z

No description provided.

…er, look up containers by name, rather than by index.

bmoyles0117

LGTM 🦑 Only one minor comment.

bmoyles0117 · 2017-12-21T16:02:27Z

src/kubernetes.cc

+    result.emplace_back(GetLegacyResource(pod, name));
    result.emplace_back(
-        GetContainerMetadata(pod, i, associations->Clone(), collected_at));
+        GetContainerMetadata(pod, container, container_spec,


Can we use container_status instead of container?

qingling128

LGTM.

qingling128 · 2017-12-21T18:25:02Z

src/kubernetes.cc

+    result.emplace_back(GetLegacyResource(pod, name));
    result.emplace_back(
-        GetContainerMetadata(pod, i, associations->Clone(), collected_at));
+        GetContainerMetadata(pod, container, container_spec,


supriyagarg · 2017-12-21T19:27:21Z

src/kubernetes.h

@@ -89,12 +89,13 @@ class KubernetesReader {
      const throw(json::Exception);
  // Given a pod object and container index, return the container metadata.


Please update the comment here. There is no more container index being passed in.

Good catch. Done.

supriyagarg · 2017-12-21T19:27:38Z

src/kubernetes.h

+      const json::Object* container_spec, json::value associations,
      Timestamp collected_at) const throw(json::Exception);
  // Given a pod object and container index, return the legacy resource.
  // The returned "metadata" field will be Metadata::IGNORED.


supriyagarg · 2017-12-21T19:32:33Z

src/kubernetes.cc

-  const std::string container_id =
-      container->Get<json::String>("containerID").substr(
-          docker_prefix_end);
+  const std::string container_id = docker_id.substr(docker_prefix_end);


the container ID is not used anywhere.

It's used in line 318 (hidden by the diff).

supriyagarg · 2017-12-21T19:33:16Z

src/kubernetes.cc

      {"status", json::object({
        {"version", json::string(kKubernetesApiVersion)},
-        {"raw", container->Clone()},
+        {"raw", container_status->Clone()},


why not add the spec to the "raw" metadata as well?

It is, the lines are right above hidden in the diff.

ah, thanks Brian.

- Rename variable. - Update method comments.

igorpeshansky

Addressed feedback. PTAL.

igorpeshansky · 2017-12-29T18:22:58Z

src/kubernetes.cc

-  const std::string container_id =
-      container->Get<json::String>("containerID").substr(
-          docker_prefix_end);
+  const std::string container_id = docker_id.substr(docker_prefix_end);


It's used in line 318 (hidden by the diff).

igorpeshansky · 2017-12-29T18:24:00Z

src/kubernetes.cc

+    result.emplace_back(GetLegacyResource(pod, name));
    result.emplace_back(
-        GetContainerMetadata(pod, i, associations->Clone(), collected_at));
+        GetContainerMetadata(pod, container, container_spec,


igorpeshansky · 2017-12-29T18:25:19Z

src/kubernetes.h

@@ -89,12 +89,13 @@ class KubernetesReader {
      const throw(json::Exception);
  // Given a pod object and container index, return the container metadata.


Good catch. Done.

igorpeshansky · 2017-12-29T18:25:24Z

src/kubernetes.h

+      const json::Object* container_spec, json::value associations,
      Timestamp collected_at) const throw(json::Exception);
  // Given a pod object and container index, return the legacy resource.
  // The returned "metadata" field will be Metadata::IGNORED.


Since containerStatuses and containerSpecs may not be in the same ord…

6705868

…er, look up containers by name, rather than by index.

igorpeshansky requested review from bmoyles0117, qingling128 and supriyagarg December 21, 2017 15:20

bmoyles0117 approved these changes Dec 21, 2017

View reviewed changes

qingling128 approved these changes Dec 21, 2017

View reviewed changes

supriyagarg reviewed Dec 21, 2017

View reviewed changes

Address feedback:

e0eded9

- Rename variable. - Update method comments.

igorpeshansky commented Dec 29, 2017

View reviewed changes

supriyagarg approved these changes Jan 2, 2018

View reviewed changes

igorpeshansky merged commit 8a58544 into master Jan 2, 2018

igorpeshansky deleted the igorp-kubernetes-container-lists branch January 2, 2018 17:16

		@@ -89,12 +89,13 @@ class KubernetesReader {
		const throw(json::Exception);
		// Given a pod object and container index, return the container metadata.

Fix container spec/status order mismatch. #47

Fix container spec/status order mismatch. #47

Uh oh!

Conversation

igorpeshansky commented Dec 21, 2017

Uh oh!

bmoyles0117 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!

qingling128 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants