RFC: Update the agent to send data to the v1beta3 API #141

supriyagarg · 2018-05-25T19:08:22Z

This PR deletes all docker related code, drops k8s cluster / services / endpoints, and allows sending nodes and pods to the v1beta3 API

igorpeshansky

General comments on the approach.

igorpeshansky · 2018-05-25T20:48:30Z

src/reporter.cc

+              {metadata.version,
+                json::object({
+                  {"schemaName", json::string(metadata.schema_name)},
+                  {"stringContent", json::string(metadata.metadata->ToString())},


You should be able to just use {"structContent", std::move(metadata.metadata)} here...

Yes - that is doable.
But sending a string is better since that is the format the backend validates / provides on the read path.
The current implementation will minimize conversions on the backend, and lower chances of data corruption.
We are doing JSON -> string right now.
Sending structContent would mean: JSON -> proto -> JSON -> string

I'm confused — we're sending JSON already. What you wrote amounts to sending:

{ "<version>": { "schemaName": "<schema_name>", "stringContent": "{\"my_metadata_field\": \"some value\",...}" } }

Why would we want to go through the extra layer of encoding at the agent level and parsing at the API level?

Using structContent would mean that the API is seeing a struct proto, not pure JSON. This would also mean conversion from JSON -> struct proto -> JSON, possibly leading to some data corruption. That is what I am trying to avoid here.

Wouldn't the conversion be done at the ESF translation layer, before the API gets the data? I'm really wary of sending raw strings around — if this is what is required for fidelity, then this is bad API design.

That is correct. I really want to prevent any conversion of the JSON to proto, since that can cause possible mismatches. Sending it as string means that the only conversion is back to JSON

igorpeshansky · 2018-05-25T20:53:41Z

src/kubernetes.h

                 MetadataUpdater::UpdateCallback callback) const;

-  // Service watcher.
-  void WatchServices(MetadataUpdater::UpdateCallback callback);


Are we not supposed to be sending in service/endpoints metadata?

Yes we are, but I wanted to focus on getting Nodes / Pods right first before expanding to other types.
The code for services / endpoints will change a lot anyway because we can now publish them individually. Further, we can abstract the code so we don't have separate watch / callbacks per type, but rather one (or a few) so that they can simply be called as:

WatchResource("Pod", "v1", callback)

This is done, right?

igorpeshansky · 2018-05-25T21:08:29Z

src/store.cc

                                   Metadata&& entry) {
+  if (full_resource_name.empty()) {
+    if (config_.VerboseLogging()) {
+      LOG(INFO) << "Dropping metadata entry with a blank full resource name";


This now does the same thing as entry.ignored (except that I never thought of just dropping that entry in the first place). Do we want to get rid of one or the other?

Updated this code to to check for both: empty full resource name OR entry.ignored, and removed the check from MetadataReporter::SendMetadata()

igorpeshansky · 2018-05-25T21:12:04Z

src/reporter.cc

-    LOG(INFO) << "About to send request: POST " << endpoint
-              << " User-Agent: " << user_agent
-              << " " << *update_metadata_request;
+  // TODO: Convert this into a batch request.


It's good to have this TODO, but I think we actually need to do it before this is ready to be tested.

Updated the code to send batch requests.

igorpeshansky · 2018-06-15T19:58:37Z

src/reporter.cc

+              {metadata.version,
+                json::object({
+                  {"schemaName", json::string(metadata.schema_name)},
+                  {"stringContent", json::string(metadata.metadata->ToString())},


I'm confused — we're sending JSON already. What you wrote amounts to sending:

{ "<version>": { "schemaName": "<schema_name>", "stringContent": "{\"my_metadata_field\": \"some value\",...}" } }

Why would we want to go through the extra layer of encoding at the agent level and parsing at the API level?

igorpeshansky · 2018-06-15T20:01:52Z

src/configuration.cc

-      config["MetadataIngestionEndpointFormat"].as<std::string>(
-          metadata_ingestion_endpoint_format_);
+  metadata_ingestion_host_ =
+      config["MetadataIngestionHost"].as<std::string>(


This makes the change not backwards-compatible, and thus this is no longer a drop-in replacement for the v1beta2 version.

igorpeshansky · 2018-06-15T20:09:54Z

src/reporter.cc

-              << " User-Agent: " << user_agent
-              << " " << *update_metadata_request;
-  }
+  if (use_batch) {


Why would you ever not use batch?

I added the switch since the batch endpoint was not ready. Even now, it is not quite working yet.

igorpeshansky · 2018-06-15T20:13:53Z

src/reporter.cc

+    const std::string location) const {
+
+  int num_dashes = std::count(location.begin(), location.end(), '-');
+  if (num_dashes == 2) {


Yuck! This API is broken.

igorpeshansky · 2018-06-15T20:17:50Z

src/kubernetes.cc

-        reader_.WatchEndpoints(cb);
-      });
-    }
+    service_watch_thread_ = std::thread([=]() {


These are still cluster-level, aren't they? Are you going to have every node agent watch for services and endpoints?
I would also prefer to keep the KubernetesServiceMetadata option, at least for now.

Yes - they are still cluster-level. Added back the two checks.

What does keeping the KubernetesServiceMetadata option for now mean? Would it only apply to services and endpoints? Or other cluster level resources too?

igorpeshansky · 2018-06-15T20:18:20Z

src/kubernetes.h

                 MetadataUpdater::UpdateCallback callback) const;

-  // Service watcher.
-  void WatchServices(MetadataUpdater::UpdateCallback callback);


This is done, right?

The related config variables have been kept around for now.

…adataCallback. The ResourceMetadata now has a full_resource_name component. Use "" as a dummy full_resource_name when initiliazing ResourceMetadata.

Drop all code for watching services and endpoints.

Main change is to handle batch requests to the v1beta3 endpoint.

Also, update the GetPodMetadata / GetNodeMetadata tests to test all properties of the Metadata object.

For services and endpoints, we use generic code, since computing resource full name, type, location etc does not need to be type specific.

The API is about to deprecate it.

The batch endpoint works fine now, so we no longer need the switch. However, batch requests need > 1 individual requests inside, so we still have the code to send a single request without the batch wrapper.

This allows the updated agent to be a drop in replacement for the v1beta2 version of the agent.

When we happen to have a single pending request, make a duplicate, without the "views" field, and send it in a batch request.

supriyagarg requested review from bmoyles0117 and igorpeshansky May 25, 2018 19:08

supriyagarg force-pushed the pod-v1beta3 branch 2 times, most recently from c1a5806 to d9612e4 Compare May 25, 2018 21:23

igorpeshansky suggested changes May 25, 2018

View reviewed changes

supriyagarg force-pushed the pod-v1beta3 branch 2 times, most recently from 682b5f9 to 7fe7b36 Compare June 14, 2018 15:29

igorpeshansky suggested changes Jun 15, 2018

View reviewed changes

supriyagarg force-pushed the pod-v1beta3 branch 5 times, most recently from 82e0f72 to 01277e5 Compare June 22, 2018 20:50

supriyagarg added 17 commits July 19, 2018 16:41

Remove the code to collect docker containers.

1f4a26e

The related config variables have been kept around for now.

Update the default API endpoint format.

bcffd26

Update the MetadataStore to use full resource name in the metadata_map_

1d7844f

Update MetadataUpdater to use the full resource name during UpdateMet…

c21dd66

…adataCallback. The ResourceMetadata now has a full_resource_name component. Use "" as a dummy full_resource_name when initiliazing ResourceMetadata.

Update MetadataStore::Metadata to have fields matching the v1beta3 API.

a0eb475

Update the kubernetes code to generate the required fields for v1beta3.

ee48839

Drop all code for watching services and endpoints.

Fix the reporter to send single resources per publish request.

81093e2

Address comments.

4a4da79

Main change is to handle batch requests to the v1beta3 endpoint.

Compute the location of Unscheduled pods correctly.

973b9e6

Also, update the GetPodMetadata / GetNodeMetadata tests to test all properties of the Metadata object.

Add extra logs for the API response

0094b2b

Add support to switch between individual and batch writes

ad8a4ec

Update the location format reported to the metadata agent

5c124b7

Update the batch endpoint

5b6be34

Update the location for nodes and pods to the cluster location.

887a257

Add support for watching and ingesting Services and Endponts.

71b3122

For services and endpoints, we use generic code, since computing resource full name, type, location etc does not need to be type specific.

Run service and endpoint watches only at cluster level.

1f62cd5

Drop the created_at field from the Metadata object.

51bc70c

The API is about to deprecate it.

supriyagarg added 5 commits July 19, 2018 16:42

Minor fixes to the batch code.

94fad2d

Remove the MetadataIngestionUseBatch flag.

0348a4c

The batch endpoint works fine now, so we no longer need the switch. However, batch requests need > 1 individual requests inside, so we still have the code to send a single request without the batch wrapper.

Revert to using MetadataIngestionEndpointFormat.

1328d3a

This allows the updated agent to be a drop in replacement for the v1beta2 version of the agent.

Update the metadata ingestion endpoint to point to the batch URI.

59cec10

When we happen to have a single pending request, make a duplicate, without the "views" field, and send it in a batch request.

Update the location format for GCP resources.

f779ed3

supriyagarg force-pushed the pod-v1beta3 branch from 035d02c to f779ed3 Compare July 19, 2018 20:42

supriyagarg added the v1beta3 label Jul 20, 2018

supriyagarg force-pushed the v1beta3-working-branch branch 2 times, most recently from 14cd869 to f46fc20 Compare July 24, 2018 02:50

supriyagarg force-pushed the v1beta3-working-branch branch 3 times, most recently from 8b06da6 to 74f1e02 Compare September 14, 2018 23:38

supriyagarg force-pushed the v1beta3-working-branch branch 2 times, most recently from 9eb2e79 to a8e609c Compare September 29, 2018 00:07

RFC: Update the agent to send data to the v1beta3 API #141

Are you sure you want to change the base?

RFC: Update the agent to send data to the v1beta3 API #141

Uh oh!

Conversation

supriyagarg commented May 25, 2018

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants