Add implicit instance resource mapping. by igorpeshansky · Pull Request #55 · Stackdriver/metadata-agent

igorpeshansky · 2018-02-22T18:38:41Z

Also factor out an InstanceUpdater that polls the instance resource and add more configuration options.

bmoyles0117

Looks reasonable, however, I'm confused by the merging of technical debt fixes with the overall goal of the PR.

bmoyles0117 · 2018-02-22T18:42:19Z

    8*1024*1024;
+constexpr const char kMetadataIngestionDefaultRawContentVersion[] = "0.1";
+constexpr const int kInstanceUpdaterDefaultIntervalSeconds = 60*60;
+constexpr const char kDefaultInstanceResourceType[] = "";


Should this be blank?

Blank means "get it from the environment".

Just added a comment to that effect.

bmoyles0117 · 2018-02-22T18:43:59Z

    "/resourceMetadata:batchUpdate";
 constexpr const int kMetadataIngestionDefaultRequestSizeLimitBytes =
    8*1024*1024;
+constexpr const char kMetadataIngestionDefaultRawContentVersion[] = "0.1";


This was previously only used for Kubernetes, is it reasonable to say that the agent will evolve all resources linearly with a single version? Is it possible for the raw content to change over time at different rates over different resource types? I'm not sure how this contract works with the API.

Yes, this is the version of the envelope schema for the metadata ingestion API. The next PR will use it for Docker container resources as well.

bmoyles0117 · 2018-02-22T18:45:58Z

        });

-        const json::Object* container_desc = parsed_metadata->As<json::Object>();
+        const json::Object* container_desc = raw_docker->As<json::Object>();


I'm not sure why these changes are being bundled in here. This looks like a separate PR.

Yes, this is basically drive-by cleanup. I'm happy to move these into the next PR that deals specifically with Docker.
Done.

igorpeshansky

Thanks for the review. PTAL.

igorpeshansky · 2018-02-22T19:05:12Z

    8*1024*1024;
+constexpr const char kMetadataIngestionDefaultRawContentVersion[] = "0.1";
+constexpr const int kInstanceUpdaterDefaultIntervalSeconds = 60*60;
+constexpr const char kDefaultInstanceResourceType[] = "";


Blank means "get it from the environment".

igorpeshansky · 2018-02-22T19:05:40Z

    "/resourceMetadata:batchUpdate";
 constexpr const int kMetadataIngestionDefaultRequestSizeLimitBytes =
    8*1024*1024;
+constexpr const char kMetadataIngestionDefaultRawContentVersion[] = "0.1";


Yes, this is the version of the envelope schema for the metadata ingestion API. The next PR will use it for Docker container resources as well.

igorpeshansky · 2018-02-22T19:06:41Z

        });

-        const json::Object* container_desc = parsed_metadata->As<json::Object>();
+        const json::Object* container_desc = raw_docker->As<json::Object>();


Yes, this is basically drive-by cleanup. I'm happy to move these into the next PR that deals specifically with Docker.
Done.

qingling128 · 2018-02-22T19:27:57Z

@@ -0,0 +1,63 @@
+/*
+ * Copyright 2017 Google Inc.


Ah, yes, time flies... 😄 Done.

qingling128 · 2018-02-22T19:40:36Z

 constexpr const int kMetadataIngestionDefaultRequestSizeLimitBytes =
    8*1024*1024;
 constexpr const char kMetadataIngestionDefaultRawContentVersion[] = "0.1";
+constexpr const int kInstanceUpdaterDefaultIntervalSeconds = 60*60;


An interval of 1 hour? Because GCE instance don't get updated often?

Correct. Actually, the bits we care about (the resource id) never change. I thought of actually making it 24 hours, or even larger, because we really only need the initial load on startup. Finally settled on 1 hour, for no good reason.

If we choose to actually send mutable instance metadata, including tags, etc, we would poll this more often (e.g., once a minute), just like any other resource.

Makes sense. Thanks for the clarification.

qingling128 · 2018-02-22T19:46:40Z

+    } else {
+      // Default to a GCE instance.
+      // TODO: Detect other resources.
+      instance_resource_type_ = kGceInstanceResourceType;


A question not directly related to this PR:
For other resources to be detected, is k8s_node one of them, assuming the metadata agent runs on the same node?

No, this is the underlying instance resource. "Other resources" in this case would be "aws_ec2_instance", for example. Adjusted the comment to make this clearer.

Ah, right. Thanks for adding the comment!

igorpeshansky

Thanks, PTAL.

igorpeshansky · 2018-02-22T19:52:19Z

+    } else {
+      // Default to a GCE instance.
+      // TODO: Detect other resources.
+      instance_resource_type_ = kGceInstanceResourceType;


No, this is the underlying instance resource. "Other resources" in this case would be "aws_ec2_instance", for example. Adjusted the comment to make this clearer.

igorpeshansky · 2018-02-22T19:53:38Z

@@ -0,0 +1,63 @@
+/*
+ * Copyright 2017 Google Inc.


Ah, yes, time flies... 😄 Done.

igorpeshansky · 2018-02-22T19:55:03Z

 constexpr const int kMetadataIngestionDefaultRequestSizeLimitBytes =
    8*1024*1024;
 constexpr const char kMetadataIngestionDefaultRawContentVersion[] = "0.1";
+constexpr const int kInstanceUpdaterDefaultIntervalSeconds = 60*60;


Correct. Actually, the bits we care about (the resource id) never change. I thought of actually making it 24 hours, or even larger, because we really only need the initial load on startup. Finally settled on 1 hour, for no good reason.

If we choose to actually send mutable instance metadata, including tags, etc, we would poll this more often (e.g., once a minute), just like any other resource.

bmoyles0117

LGTM

bmoyles0117 · 2018-02-22T20:20:04Z

+#ifndef INSTANCE_H_
+#define INSTANCE_H_
+
+//#include "config.h"


Nit: Remove if unnecessary.

This is the pattern in all other files, just to allow using autotools later. I'd like to keep it consistent.

qingling128

LGTM.

qingling128 · 2018-02-22T20:27:26Z

 constexpr const int kMetadataIngestionDefaultRequestSizeLimitBytes =
    8*1024*1024;
 constexpr const char kMetadataIngestionDefaultRawContentVersion[] = "0.1";
+constexpr const int kInstanceUpdaterDefaultIntervalSeconds = 60*60;


Makes sense. Thanks for the clarification.

qingling128 · 2018-02-22T20:28:13Z

+    } else {
+      // Default to a GCE instance.
+      // TODO: Detect other resources.
+      instance_resource_type_ = kGceInstanceResourceType;


Ah, right. Thanks for adding the comment!

qingling128 · 2018-02-22T20:28:41Z

@@ -0,0 +1,63 @@
+/*
+ * Copyright 2017 Google Inc.


qingling128 · 2018-02-22T20:28:50Z

+#ifndef INSTANCE_H_
+#define INSTANCE_H_
+
+//#include "config.h"


supriyagarg · 2018-02-22T21:00:06Z

+    } else {
+      // Default to a GCE instance.
+      // TODO: Detect other instance resources.
+      instance_resource_type_ = kGceInstanceResourceType;


Is there a reason that config_.InstanceResourceType cannot default to "gce_instance"?

The current default (empty value) means "detect via the environment". I've added a comment in configuration.cc to that effect.

igorpeshansky

Thanks, PTAL.

igorpeshansky · 2018-02-22T23:06:41Z

    8*1024*1024;
+constexpr const char kMetadataIngestionDefaultRawContentVersion[] = "0.1";
+constexpr const int kInstanceUpdaterDefaultIntervalSeconds = 60*60;
+constexpr const char kDefaultInstanceResourceType[] = "";


Just added a comment to that effect.

igorpeshansky · 2018-02-22T23:07:38Z

+    } else {
+      // Default to a GCE instance.
+      // TODO: Detect other instance resources.
+      instance_resource_type_ = kGceInstanceResourceType;


The current default (empty value) means "detect via the environment". I've added a comment in configuration.cc to that effect.

igorpeshansky · 2018-02-22T23:09:34Z

+#ifndef INSTANCE_H_
+#define INSTANCE_H_
+
+//#include "config.h"


This is the pattern in all other files, just to allow using autotools later. I'd like to keep it consistent.

supriyagarg

LGTM

…Version into configuration options.

igorpeshansky · 2018-02-23T20:57:40Z

Rebased off master.

igorpeshansky requested review from bmoyles0117, qingling128 and supriyagarg February 22, 2018 18:38

bmoyles0117 suggested changes Feb 22, 2018

View reviewed changes

igorpeshansky commented Feb 22, 2018

View reviewed changes

qingling128 suggested changes Feb 22, 2018

View reviewed changes

igorpeshansky commented Feb 22, 2018

View reviewed changes

bmoyles0117 approved these changes Feb 22, 2018

View reviewed changes

qingling128 approved these changes Feb 22, 2018

View reviewed changes

supriyagarg reviewed Feb 22, 2018

View reviewed changes

igorpeshansky commented Feb 22, 2018

View reviewed changes

supriyagarg approved these changes Feb 23, 2018

View reviewed changes

igorpeshansky added 5 commits February 23, 2018 15:48

Turn MetadataApiResourceTypeSeparator and MetadataIngestionRawContent…

6906804

…Version into configuration options.

Factor out instance resources into an instance updater.

cedbf03

Revert docker.cc cleanup.

83ffd88

What year is it?! And other minor tweaks.

e39adc6

Explain the empty default for a config value.

c1fa702

igorpeshansky force-pushed the igorp-instance-resource branch from 5a9e66c to c1fa702 Compare February 23, 2018 20:57

igorpeshansky merged commit 419d9ca into master Feb 23, 2018

igorpeshansky deleted the igorp-instance-resource branch February 23, 2018 21:13

Conversation

igorpeshansky commented Feb 22, 2018

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!

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!

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!

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!