Implement new Kubernetes resources and metadata. by igorpeshansky · Pull Request #25 · Stackdriver/metadata-agent

igorpeshansky · 2017-09-07T23:01:54Z

No description provided.

qingling128 · 2017-09-08T01:28:09Z

  LOG(INFO) << "Kubernetes Query called";
+  std::vector<PollingMetadataUpdater::ResourceMetadata> result;
+
+  const std::string platform = "gce";  // TODO


Add some description for this TODO?

qingling128 · 2017-09-08T01:38:00Z

+    result.emplace_back(
+        std::vector<std::string>{k8s_node_name},
+        k8s_node,
+#ifdef ENABLE_KUBERNETES_METADATA


Just curious, for which case will we set ENABLE_KUBERNETES_METADATA false? I guess the Logging Agent / Monitoring Agent does not need metadata to be present.

For now, this is a compile-time flag. We might need to make it a configuration option at some point.
I'd prefer to leave it here, but we could also take it out and make this code unconditional. WDYT?

Ah, I see. I think it's fine to leave it as it is for now.

qingling128 · 2017-09-08T15:31:15Z

+                     path_component.second + "/" + name);
+}
+
+json::value KubernetesReader::FindPrimary(


:%s/FindPrimary/FindPrimaryController/g?

Gave an even more accurate name to this function.

Nice. Thanks.

qingling128 · 2017-09-08T16:21:30Z

 constexpr const char kKubernetesEndpointPath[] = "/api/v1";
 constexpr const char kResourceTypeSeparator[] = ".";

+constexpr const char kRawContentVersion[] = "0.1";


How do we decide which metadata is versioned by kRawContentVersion, while others are versioned by kKubernetesEndpointPath? Maybe a brief comment for clarification?

We don't version anything by kKubernetesEndpointPath -- I assume you meant kKubernetesApiVersion. This is the format defined by the target API. Blobs that come directly from Kubernetes and are ingested verbatim are versioned with the API version, while blobs that we have to construct are versioned by kRawContentVersion.

Got it. And yeah, I meant kKubernetesApiVersion.

bmoyles0117 · 2017-09-11T16:25:16Z

+        {"association", json::object({
+          {"version", json::string(kRawContentVersion)},
+          {"raw", json::object({
+            {"providerPlatform", json::string(platform)},


Is this camel cased for consistency with another reference to providerPlatform?

That's what the metadata API will be looking for. We can coordinate and change this if it becomes an issue.

bmoyles0117 · 2017-09-11T16:29:01Z

      config_.KubernetesPodLabelSelector().empty()
-      ? "" : "?" + config_.KubernetesPodLabelSelector());
+      ? "" : "&" + config_.KubernetesPodLabelSelector());
+  const std::string node_selector(kNodeSelectorPrefix + node_name);


nit: It was a little confusing having the node_selector defined second here, yet having the pod_label_selector appended with a & as the delimiter. For the mental sequencing, I feel that node_selector should be defined first.

Good point. Done.

bmoyles0117 · 2017-09-11T16:35:36Z

+                     << "have different sizes: "
+                     << container_specs->size() << " vs "
+                     << container_list->size() << " for pod "
+                     << pod_id << "(" << pod_name << ")";


Do we need some form of a return here? Will some sort of error occur in the next loop if these sizes don't align?

I've picked the minimum of the two for the loop.

bmoyles0117 · 2017-09-11T16:37:08Z

          bool is_deleted = false;

-          const MonitoredResource resource("gke_container", {
+          const MonitoredResource gke_resource("gke_container", {


Why do we call this gke_resource instead of gke_container?

No good reason other than it used to be called resource and I was lazy. Renamed.

bmoyles0117 · 2017-09-11T16:37:45Z

+              gke_resource,
+              MetadataAgent::Metadata::IGNORED());
+
+          const MonitoredResource k8s_container("k8s_container", {


If we should call the variable mentioned above "gke_resource", should this be called "k8s_resource"?

It started that way, but there are multiple k8s_* resources... Renamed gke_resource to gke_container to match.

bmoyles0117 · 2017-09-11T16:40:16Z

+  try {
+    http::client::response response = client.get(request);
+#ifdef DEBUG
+    LOG(INFO) << "QueryMaster: Response: " << body(response);


The conditional here is DEBUG, should we be logging with level DEBUG?

That would be an unfortunate clash of a macro with an enum value, actually... I've renamed the macro to VERBOSE, and changed the logging to DEBUG.

bmoyles0117 · 2017-09-11T16:42:47Z

+  if (current_node_.empty()) {
+    const std::string& ns = KubernetesNamespace();
+    // TODO: This is unreliable, find a better way.
+    const std::string pod_name = boost::asio::ip::host_name();


Agreed that this is unreliable, what are the alternatives?

kubernetes/kubernetes#52162

bmoyles0117 · 2017-09-11T16:44:18Z

+    const std::string& version, const std::string& kind) const {
+  std::lock_guard<std::recursive_mutex> lock(mutex_);
+  const std::string prefix(
+      (version.find('/') == std::string::npos) ? "/api" : "/apis");


Would it be better to check specifically against extensions instead of /?

It's not just extensions/*, it's also batch/* and a couple of others...

igorpeshansky

Thanks for the reviews. PTAL.

igorpeshansky · 2017-09-11T15:49:39Z

 constexpr const char kKubernetesEndpointPath[] = "/api/v1";
 constexpr const char kResourceTypeSeparator[] = ".";

+constexpr const char kRawContentVersion[] = "0.1";


We don't version anything by kKubernetesEndpointPath -- I assume you meant kKubernetesApiVersion. This is the format defined by the target API. Blobs that come directly from Kubernetes and are ingested verbatim are versioned with the API version, while blobs that we have to construct are versioned by kRawContentVersion.

igorpeshansky · 2017-09-11T16:12:23Z

  LOG(INFO) << "Kubernetes Query called";
+  std::vector<PollingMetadataUpdater::ResourceMetadata> result;
+
+  const std::string platform = "gce";  // TODO


igorpeshansky · 2017-09-11T16:16:05Z

+    result.emplace_back(
+        std::vector<std::string>{k8s_node_name},
+        k8s_node,
+#ifdef ENABLE_KUBERNETES_METADATA


For now, this is a compile-time flag. We might need to make it a configuration option at some point.
I'd prefer to leave it here, but we could also take it out and make this code unconditional. WDYT?

igorpeshansky · 2017-09-11T16:19:50Z

+                     path_component.second + "/" + name);
+}
+
+json::value KubernetesReader::FindPrimary(


Gave an even more accurate name to this function.

igorpeshansky · 2017-09-12T16:07:06Z

+        {"association", json::object({
+          {"version", json::string(kRawContentVersion)},
+          {"raw", json::object({
+            {"providerPlatform", json::string(platform)},


That's what the metadata API will be looking for. We can coordinate and change this if it becomes an issue.

igorpeshansky · 2017-09-12T16:17:23Z

          bool is_deleted = false;

-          const MonitoredResource resource("gke_container", {
+          const MonitoredResource gke_resource("gke_container", {


No good reason other than it used to be called resource and I was lazy. Renamed.

igorpeshansky · 2017-09-12T16:18:10Z

+              gke_resource,
+              MetadataAgent::Metadata::IGNORED());
+
+          const MonitoredResource k8s_container("k8s_container", {


It started that way, but there are multiple k8s_* resources... Renamed gke_resource to gke_container to match.

igorpeshansky · 2017-09-12T16:23:11Z

+  try {
+    http::client::response response = client.get(request);
+#ifdef DEBUG
+    LOG(INFO) << "QueryMaster: Response: " << body(response);


That would be an unfortunate clash of a macro with an enum value, actually... I've renamed the macro to VERBOSE, and changed the logging to DEBUG.

igorpeshansky · 2017-09-12T16:26:21Z

+  if (current_node_.empty()) {
+    const std::string& ns = KubernetesNamespace();
+    // TODO: This is unreliable, find a better way.
+    const std::string pod_name = boost::asio::ip::host_name();


kubernetes/kubernetes#52162

igorpeshansky · 2017-09-12T16:27:58Z

+    const std::string& version, const std::string& kind) const {
+  std::lock_guard<std::recursive_mutex> lock(mutex_);
+  const std::string prefix(
+      (version.find('/') == std::string::npos) ? "/api" : "/apis");


It's not just extensions/*, it's also batch/* and a couple of others...

qingling128

LGTM.

qingling128 · 2017-09-12T16:42:34Z

 constexpr const char kKubernetesEndpointPath[] = "/api/v1";
 constexpr const char kResourceTypeSeparator[] = ".";

+constexpr const char kRawContentVersion[] = "0.1";


Got it. And yeah, I meant kKubernetesApiVersion.

qingling128 · 2017-09-12T16:42:46Z

  LOG(INFO) << "Kubernetes Query called";
+  std::vector<PollingMetadataUpdater::ResourceMetadata> result;
+
+  const std::string platform = "gce";  // TODO


qingling128 · 2017-09-12T16:45:49Z

+    result.emplace_back(
+        std::vector<std::string>{k8s_node_name},
+        k8s_node,
+#ifdef ENABLE_KUBERNETES_METADATA


Ah, I see. I think it's fine to leave it as it is for now.

qingling128 · 2017-09-12T16:47:14Z

+                     path_component.second + "/" + name);
+}
+
+json::value KubernetesReader::FindPrimary(


Nice. Thanks.

bmoyles0117

LGTM

bmoyles0117

LGTM

dhrupadb · 2017-09-12T20:06:09Z

+          {"version", json::string(kRawContentVersion)},
+          {"raw", json::object({
+            {"providerPlatform", json::string(platform)},
+            {"instance_id", json::string(instance_id)},


Can we add a TODO here just to ensure we fix this inconsistency cases? i.e. change to instanceId once fix is implemented on the API side as well.

dhrupadb · 2017-09-12T20:08:21Z

+                                /*deleted=*/false, created_at, collected_at,
+                                std::move(node_raw_metadata))
+#else
+        MetadataAgent::Metadata::IGNORED()


Aren't we doing away with IGNORED metadata anyway?

No, we still need it for the old GKE container resources, at the very least. And we'll reuse it for instance resources, at least initially.
Or are you asking about ENABLE_KUBERNETES_METADATA?

Got it. Thanks.

dhrupadb · 2017-09-12T20:22:38Z

+  std::lock_guard<std::recursive_mutex> lock(mutex_);
+  if (kubernetes_api_token_.empty()) {
+    std::string filename =
+        "/var/run/secrets/kubernetes.io/serviceaccount/token";


Can we move this to a constant? The /var/run/secrets/kubernetes.io/serviceaccount/ bit can be shared across two methods and "token" or "namespace" appended on.

Done (also extracted the common functionality into a function).

igorpeshansky

Thanks, PTAL.

igorpeshansky · 2017-09-12T22:35:10Z

+          {"version", json::string(kRawContentVersion)},
+          {"raw", json::object({
+            {"providerPlatform", json::string(platform)},
+            {"instance_id", json::string(instance_id)},


igorpeshansky · 2017-09-12T22:36:52Z

+                                /*deleted=*/false, created_at, collected_at,
+                                std::move(node_raw_metadata))
+#else
+        MetadataAgent::Metadata::IGNORED()


No, we still need it for the old GKE container resources, at the very least. And we'll reuse it for instance resources, at least initially.
Or are you asking about ENABLE_KUBERNETES_METADATA?

igorpeshansky · 2017-09-12T22:37:12Z

+  std::lock_guard<std::recursive_mutex> lock(mutex_);
+  if (kubernetes_api_token_.empty()) {
+    std::string filename =
+        "/var/run/secrets/kubernetes.io/serviceaccount/token";


Done (also extracted the common functionality into a function).

dhrupadb

LGTM 👌

dhrupadb · 2017-09-12T23:02:12Z

+                                /*deleted=*/false, created_at, collected_at,
+                                std::move(node_raw_metadata))
+#else
+        MetadataAgent::Metadata::IGNORED()


Got it. Thanks.

igorpeshansky requested review from bmoyles0117, dhrupadb, qingling128 and rbuskens September 7, 2017 23:01

igorpeshansky force-pushed the igorp-kubernetes-resources branch from 5dcfa20 to 3060b15 Compare September 7, 2017 23:04

qingling128 reviewed Sep 8, 2017

View reviewed changes

bmoyles0117 suggested changes Sep 11, 2017

View reviewed changes

igorpeshansky commented Sep 12, 2017

View reviewed changes

qingling128 approved these changes Sep 12, 2017

View reviewed changes

bmoyles0117 reviewed Sep 12, 2017

View reviewed changes

bmoyles0117 approved these changes Sep 12, 2017

View reviewed changes

dhrupadb suggested changes Sep 12, 2017

View reviewed changes

igorpeshansky commented Sep 12, 2017

View reviewed changes

dhrupadb approved these changes Sep 12, 2017

View reviewed changes

igorpeshansky and others added 4 commits September 12, 2017 19:50

Add json::Object::Has(). Make json::Value::Clone() public throughout.

765c16a

Move KubernetesApiToken() into KubernetesReader.

89c0ed5

Implement new Kubernetes resources and metadata. #25

72fe327

Bump version to 0.0.12.

be35e2f

igorpeshansky force-pushed the igorp-kubernetes-resources branch from ed0756a to be35e2f Compare September 12, 2017 23:52

igorpeshansky merged commit 02d4b8f into master Sep 12, 2017

igorpeshansky added a commit that referenced this pull request Sep 12, 2017

Implement new Kubernetes resources and metadata. #25

22d2d43

igorpeshansky deleted the igorp-kubernetes-resources branch September 12, 2017 23:55

igorpeshansky added a commit that referenced this pull request Sep 13, 2017

Implement new Kubernetes resources and metadata. (#25)

c32bcf8

igorpeshansky added a commit that referenced this pull request Sep 14, 2017

Fix copy-paste error from #25.

90847c7

igorpeshansky added a commit that referenced this pull request Sep 14, 2017

Fix copy-paste error from #25. (#29)

908c76a

Conversation

igorpeshansky commented Sep 7, 2017

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 Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bmoyles0117 Sep 11, 2017 • edited by igorpeshansky Loading Uh oh! There was an error while loading. Please reload this page.

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!

bmoyles0117 Sep 11, 2017 • edited by igorpeshansky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmoyles0117 Sep 11, 2017 • edited by igorpeshansky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmoyles0117 Sep 11, 2017 • edited by igorpeshansky Loading Uh oh! There was an error while loading. Please reload this page.

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!

qingling128 Sep 8, 2017 •

edited

Loading

bmoyles0117 Sep 11, 2017 •

edited by igorpeshansky

Loading

bmoyles0117 Sep 11, 2017 •

edited by igorpeshansky

Loading

bmoyles0117 Sep 11, 2017 •

edited by igorpeshansky

Loading

bmoyles0117 Sep 11, 2017 •

edited by igorpeshansky

Loading