Retrieve cluster location from metadata attributes, rather than using the instance zone. #57

igorpeshansky · 2018-02-23T18:49:17Z

bmoyles0117 · 2018-02-23T20:24:52Z

src/environment.cc

+      std::istringstream in(kube_env);
+      for (std::string line; std::getline(in, line); ) {
+        if (line.find("ZONE: ") == 0) {
+          kubernetes_cluster_location_ = line.substr(line.find(':') + 2);


Let's break out of this early when we find a match.

Obsolete. See the new shiny in the next push.

bmoyles0117 · 2018-02-23T20:25:58Z

src/environment.cc


 #include <boost/network/protocol/http/client.hpp>
 #include <fstream>
+#include <sstream>


Naive c++ question, where is this actually being used? Is this in the std namespace when you use std::istringstream in(kube_env);?

Yeah, this is the header for std::istringstream.

remove this import now that std::istringstream is no longer used.

Good catch. Done.

kawych · 2018-02-23T21:32:27Z

src/environment.cc

+    } else {
+      // Get the kube-env.
+      const std::string kube_env =
+          GetMetadataString("instance/attributes/kube-env");


For all clusters with Kubernetes in version 1.10.0 or newer, Metadata Server exports instance attribute "cluster-location". This works also for all GKE clusters created or upgraded after last GKE release (with cl/185806663) included. You shouldn't need to extract this from "kube-env". Do we need to support older clusters?

I think we discussed this earlier in email thread, sorry if I didn't clarify this enough.

Sorry, I didn't find the email thread in time. Updated the bug with the info and fixed.

kawych · 2018-02-23T21:34:37Z

Please reassign to @x13n if the review is needed within a week, I'm on vacation until March 5.

supriyagarg · 2018-02-23T22:45:06Z

src/environment.cc

+      const std::string kube_env =
+          GetMetadataString("instance/attributes/kube-env");
+      // kube-env is a list of NAME: VALUE pairs, one per line.
+      // The actual location is in the ZONE variable.


Is this true for regional clusters too? Or is there a new region / location field?

This was stale anyway. See the new shiny in the next push.

igorpeshansky

Thanks, all. PTAL.

igorpeshansky · 2018-02-24T00:48:32Z

src/environment.cc


 #include <boost/network/protocol/http/client.hpp>
 #include <fstream>
+#include <sstream>


Yeah, this is the header for std::istringstream.

igorpeshansky · 2018-02-24T00:49:04Z

src/environment.cc

+    } else {
+      // Get the kube-env.
+      const std::string kube_env =
+          GetMetadataString("instance/attributes/kube-env");


Sorry, I didn't find the email thread in time. Updated the bug with the info and fixed.

igorpeshansky · 2018-02-24T00:49:29Z

src/environment.cc

+      const std::string kube_env =
+          GetMetadataString("instance/attributes/kube-env");
+      // kube-env is a list of NAME: VALUE pairs, one per line.
+      // The actual location is in the ZONE variable.


This was stale anyway. See the new shiny in the next push.

igorpeshansky · 2018-02-24T00:49:41Z

src/environment.cc

+      std::istringstream in(kube_env);
+      for (std::string line; std::getline(in, line); ) {
+        if (line.find("ZONE: ") == 0) {
+          kubernetes_cluster_location_ = line.substr(line.find(':') + 2);


Obsolete. See the new shiny in the next push.

supriyagarg

LGTM

supriyagarg · 2018-02-24T15:01:27Z

src/environment.cc


 #include <boost/network/protocol/http/client.hpp>
 #include <fstream>
+#include <sstream>


remove this import now that std::istringstream is no longer used.

bmoyles0117

LGTM

bmoyles0117

LGTM

igorpeshansky · 2018-02-25T20:03:49Z

FYI, I've rebased this off the HTTP status changes, as those are required for correctly detecting that the attribute is absent.

…ce zone.

igorpeshansky · 2018-02-26T12:21:23Z

Rebased off master.

bmoyles0117

LGTM

igorpeshansky requested review from bmoyles0117, kawych and supriyagarg February 23, 2018 18:49

bmoyles0117 suggested changes Feb 23, 2018

View reviewed changes

kawych reviewed Feb 23, 2018

View reviewed changes

supriyagarg reviewed Feb 23, 2018

View reviewed changes

igorpeshansky commented Feb 24, 2018

View reviewed changes

igorpeshansky changed the title ~~Retrieve cluster location from kube-env, rather than using the instance zone.~~ Retrieve cluster location from metadata attributes, rather than using the instance zone. Feb 24, 2018

supriyagarg approved these changes Feb 24, 2018

View reviewed changes

igorpeshansky requested a review from kawych February 24, 2018 18:34

bmoyles0117 approved these changes Feb 25, 2018

View reviewed changes

igorpeshansky force-pushed the igorp-kubernetes-cluster-location branch from 75119f2 to a98bf11 Compare February 25, 2018 05:41

igorpeshansky changed the base branch from master to igorp-handle-http-codes February 25, 2018 05:42

bmoyles0117 approved these changes Feb 25, 2018

View reviewed changes

igorpeshansky force-pushed the igorp-handle-http-codes branch from 7bc2c05 to 32e3db1 Compare February 26, 2018 02:11

igorpeshansky added 4 commits February 26, 2018 07:18

Retrieve cluster location from kube-env, rather than using the instan…

2340968

…ce zone.

Use the cluster-location instance attribute instead.

752ab62

Remove unneeded header.

8bf0238

Fall back on kube-env if the newer attribute is not available.

f30ec5c

igorpeshansky force-pushed the igorp-kubernetes-cluster-location branch from a98bf11 to f30ec5c Compare February 26, 2018 12:20

igorpeshansky changed the base branch from igorp-handle-http-codes to master February 26, 2018 12:20

igorpeshansky requested a review from x13n February 26, 2018 12:47

x13n approved these changes Feb 26, 2018

View reviewed changes

bmoyles0117 approved these changes Feb 26, 2018

View reviewed changes

igorpeshansky merged commit b6b6142 into master Feb 26, 2018

igorpeshansky deleted the igorp-kubernetes-cluster-location branch February 26, 2018 14:35

Retrieve cluster location from metadata attributes, rather than using the instance zone. #57

Retrieve cluster location from metadata attributes, rather than using the instance zone. #57

Uh oh!

Conversation

igorpeshansky commented Feb 23, 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!

supriyagarg Feb 24, 2018 • 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!

kawych commented Feb 23, 2018

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!

supriyagarg left a comment

Choose a reason for hiding this comment

Uh oh!

supriyagarg Feb 24, 2018 • 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!

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

igorpeshansky commented Feb 25, 2018

Uh oh!

igorpeshansky commented Feb 26, 2018

Uh oh!

bmoyles0117 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

supriyagarg Feb 24, 2018 •

edited by igorpeshansky

Loading

supriyagarg Feb 24, 2018 •

edited by igorpeshansky

Loading