Allow use of K8s node name instead of machine_id

This is optional and disabled by default but is necessary on platforms
like EKS and PKS that have duplicated machine-ids across nodes.
Eventually it would be better to migrate away from machine-id altogether
since it is so unreliable, but this would break backwards compatibility
so it will have to be a major release

Author: Ben Keith

deployments/k8s/README.md

@@ -17,6 +17,10 @@ A few things to do before deploying these:
 	reference in [./clusterrolebinding.yaml](./clusterrolebinding.yaml) to the
 	namespace in which you are deploying the agent.
 
+ 3. Create a secret in K8s with your org's access token:
+
+	`kubectl create secret generic --from-literal access-token=MY_ACCESS_TOKEN signalfx-agent`
+
 Then to deploy run the following from the present directory:
 
 `cat *.yaml | kubectl apply -f -`

docs/kubernetes-setup.md

@@ -15,7 +15,12 @@ cluster and configure it to auto-discover SignalFx-supported integrations to
 monitor.
 
 1. Store your organization's Access Token as a key named `access-token` in a
-   Kubernetes secret named `signalfx-agent`.
+   Kubernetes secret named `signalfx-agent`:
+
+   ```sh
+    $ kubectl create secret generic --from-literal access-token=MY_ACCESS_TOKEN signalfx-agent`
+   ```
+
 2. If you use [Helm](https://github.com/kubernetes/helm), you can use [our
    chart](https://github.com/kubernetes/charts/tree/master/stable/signalfx-agent)
    in the stable Helm chart repository.  Otherwise, download the following
@@ -43,6 +48,17 @@ monitor.
       **If you are using Rancher for your Kubernetes deployment,** complete the
       instructions in [Rancher](#rancher) before proceeding with the next step.
 
+	  **If you are using AWS Elastic Container Service for Kubernetes (EKS)for
+	  your Kubernetes deployment,** complete the instructions in [AWS Elastic
+	  Container Service for Kubernetes
+	  (EKS)](#aws-elastic-container-service-for-kubernetes-eks) before
+	  proceeding with the next step.
+
+	  **If you are using Pivotal Container Service (PKS) for your Kubernetes
+	  deployment,** complete the instructions in [Pivotal Container Service
+	  (PKS)](#pivotal-container-service-pks) before proceeding with the next
+	  step.
+
 	  **If you are using Google Container Engine (GKE) for your Kubernetes
 	  deployment,** complete the instructions in [Google Container Engine
 	  (GKE)](#google-container-engine-gke) before proceeding with the next
@@ -257,6 +273,32 @@ Cadvisor runs on port 9344 instead of the standard 4194.
 When you have completed these steps, continue with step 3 in
 [Installation](#installation).
 
+
+## AWS Elastic Container Service for Kubernetes (EKS)
+On EKS, machine ids are identical across worker nodes, which makes that value
+useless for identification.  Therefore, there are two changes you should make
+to the configmap to use the K8s node name instead of machine-id.
+
+ 1) In the configmap.yaml, change the top-level config option `sendMachineId`
+    to `false`.  This will cause the agent to omit the machine_id dimension from
+    all datapoints and instead send the `kubernetes_node` dimension on all
+    datapoints emitted by the agent.
+
+ 2) Under the kubernetes-cluster monitor configuration, set the option
+    `useNodeName: true`.  This will cause that monitor to sync node labels to the
+    `kubernetes_node` dimension instead of the `machine_id` dimension.
+
+Note that in EKS there is no concept of a "master" node (at least not that is
+exposed via the K8s API) and so all nodes will be treated as workers.
+
+
+## Pivotal Container Service (PKS)
+
+See [AWS Elastic Container Service for
+Kubernetes](#aws-elastic-container-service-for-kubernetes-eks) -- the setup for
+PKS is identical because of the similar lack of reliable machine ids.
+
+
 ## Google Container Engine (GKE)
 
 On GKE, access to the kubelet is highly restricted and service accounts will

docs/monitors/kubernetes-cluster.md

@@ -37,6 +37,7 @@ Monitor Type: `kubernetes-cluster`
 | Config option | Required | Type | Description |
 | --- | --- | --- | --- |
 | `alwaysClusterReporter` | no | `bool` | If `true`, leader election is skipped and metrics are always reported. (**default:** `false`) |
+| `useNodeName` | no | `bool` | If set to true, the Kubernetes node name will be used as the dimension to which to sync properties about each respective node.  This is necessary if your cluster's machines do not have unique machine-id values, as can happen when machine images are improperly cloned. (**default:** `false`) |
 | `kubernetesAPI` | no | `object (see below)` | Config for the K8s API client |
 
 
@@ -85,8 +86,9 @@ dimensions may be specific to certain metrics.
 | ---  | ---         |
 | `kubernetes_name` | The name of the resource that the metric describes |
 | `kubernetes_namespace` | The namespace of the resource that the metric describes |
+| `kubernetes_node` | The name of the node, as defined by the `name` field of the node resource. |
 | `kubernetes_pod_uid` | The UID of the pod that the metric describes |
-| `machine_id` | The machine ID from /etc/machine-id.  This should be unique across all nodes in your cluster, but some cluster deployment tools don't guarantee this. |
+| `machine_id` | The machine ID from /etc/machine-id.  This should be unique across all nodes in your cluster, but some cluster deployment tools don't guarantee this.  This will not be sent if the `useNodeName` config option is set to true. |
 | `metric_source` | This is always set to `kubernetes` |
 
 ## Properties
@@ -97,7 +99,7 @@ are set on the dimension values of the dimension specified.
 
 | Name | Dimension | Description |
 | ---  | ---       | ---         |
-| `<node label>` | `machine_id` | All non-blank labels on a given node will be synced as properties to the `machine_id` dimension value for that node. Any blank values will be synced as tags on that same dimension. |
+| `<node label>` | `machine_id/kubernetes_node` | All non-blank labels on a given node will be synced as properties to the `machine_id` or `kubernetes_node` dimension value for that node.  Which dimension gets the properties is determined by the `useNodeName` config option.  Any blank values will be synced as tags on that same dimension. |
 | `<pod label>` | `kubernetes_pod_uid` | Any labels with non-blank values on the pod will be synced as properties to the `kubernetes_pod_uid` dimension. Any blank labels will be synced as tags on that same dimension. |
 
 

internal/core/hostid/dims.go

@@ -1,15 +1,19 @@
 package hostid
 
-import log "github.com/sirupsen/logrus"
+import (
+	"sync"
+
+	log "github.com/sirupsen/logrus"
+)
 
 // Dimensions returns a map of host-specific dimensions that are derived from
 // the environment.
 func Dimensions(sendMachineID bool, hostname string, useFullyQualifiedHost *bool) map[string]string {
 	log.Info("Fetching host id dimensions")
-	// Fire off all lookups simultaneously so we delay agent startup as little
-	// as possible.
 
-	hostProvider := callConcurrent(func() string {
+	var g dimGatherer
+
+	g.GatherDim("host", func() string {
 		if hostname != "" {
 			return hostname
 		}
@@ -18,38 +22,53 @@ func Dimensions(sendMachineID bool, hostname string, useFullyQualifiedHost *bool
 		// if the user specified it explicitly as false with this logic.
 		return getHostname(useFullyQualifiedHost == nil || *useFullyQualifiedHost)
 	})
-	awsProvider := callConcurrent(AWSUniqueID)
-	gcpProvider := callConcurrent(GoogleComputeID)
-	machineIDProvider := callConcurrent(MachineID)
-	azureProvider := callConcurrent(AzureUniqueID)
-
-	dims := make(map[string]string)
-	insertNextChanValue(dims, "host", hostProvider)
-	insertNextChanValue(dims, "AWSUniqueId", awsProvider)
-	insertNextChanValue(dims, "gcp_id", gcpProvider)
+	g.GatherDim("AWSUniqueId", AWSUniqueID)
+	g.GatherDim("gcp_id", GoogleComputeID)
 	if sendMachineID {
-		insertNextChanValue(dims, "machine_id", machineIDProvider)
+		g.GatherDim("machine_id", MachineID)
+	} else {
+		// If not running on k8s, this will be blank and thus omitted.  It is
+		// only sent as an alternative to machine id because k8s node labels
+		// are synced as properties to this instead of machine_id when
+		// machine_id isn't available.
+		g.GatherDim("kubernetes_node", KubernetesNodeName)
 	}
-	insertNextChanValue(dims, "azure_resource_id", azureProvider)
+	g.GatherDim("azure_resource_id", AzureUniqueID)
+
+	dims := g.WaitForDimensions()
 
 	log.Infof("Using host id dimensions %v", dims)
 	return dims
 }
 
-func callConcurrent(f func() string) <-chan string {
-	res := make(chan string)
+// Helper to fire off the dim lookups in parallel to minimize delay to agent
+// start up.
+type dimGatherer struct {
+	lock sync.Mutex
+	dims map[string]string
+	wg   sync.WaitGroup
+}
+
+// GatherDim inserts the given dim key based on the output of the provider
+// func.  If the output is blank, the dimension will not be inserted.
+func (dg *dimGatherer) GatherDim(key string, provider func() string) {
+	dg.wg.Add(1)
 	go func() {
-		res <- f()
+		res := provider()
+		if res != "" {
+			dg.lock.Lock()
+			if dg.dims == nil {
+				dg.dims = make(map[string]string)
+			}
+
+			dg.dims[key] = res
+			dg.lock.Unlock()
+		}
+		dg.wg.Done()
 	}()
-	return res
 }
 
-func insertNextChanValue(m map[string]string, k string, ch <-chan string) {
-	select {
-	case val := <-ch:
-		// Don't insert blank values
-		if val != "" {
-			m[k] = val
-		}
-	}
+func (dg *dimGatherer) WaitForDimensions() map[string]string {
+	dg.wg.Wait()
+	return dg.dims
 }

internal/core/hostid/k8s.go

@@ -0,0 +1,10 @@
+package hostid
+
+import "os"
+
+// KubernetesNodeName returns the name of the current K8s node name, if running
+// on K8s and if the appropriate envvar (MY_NODE_NAME) has been injected in the
+// agent pod by the downward API mechanism.
+func KubernetesNodeName() string {
+	return os.Getenv("MY_NODE_NAME")
+}

internal/monitors/kubernetes/cluster/metrics/cache.go

@@ -24,20 +24,26 @@ type cachedResourceKey struct {
 	UID  types.UID
 }
 
+var logger = log.WithFields(log.Fields{
+	"monitorType": "kubernetes-cluster",
+})
+
 // DatapointCache holds an up to date copy of datapoints pertaining to the
 // cluster.  It is updated whenever the HandleChange method is called with new
 // K8s resources.
 type DatapointCache struct {
 	dpCache      map[cachedResourceKey][]*datapoint.Datapoint
 	dimPropCache map[cachedResourceKey]*atypes.DimProperties
+	useNodeName  bool
 	mutex        sync.Mutex
 }
 
 // NewDatapointCache creates a new clean cache
-func NewDatapointCache() *DatapointCache {
+func NewDatapointCache(useNodeName bool) *DatapointCache {
 	return &DatapointCache{
 		dpCache:      make(map[cachedResourceKey][]*datapoint.Datapoint),
 		dimPropCache: make(map[cachedResourceKey]*atypes.DimProperties),
+		useNodeName:  useNodeName,
 	}
 }
 
@@ -109,8 +115,8 @@ func (dc *DatapointCache) HandleAdd(newObj runtime.Object) interface{} {
 	case *v1beta1.ReplicaSet:
 		dps = datapointsForReplicaSet(o)
 	case *v1.Node:
-		dps = datapointsForNode(o)
-		dimProps = dimPropsForNode(o)
+		dps = datapointsForNode(o, dc.useNodeName)
+		dimProps = dimPropsForNode(o, dc.useNodeName)
 	default:
 		log.WithFields(log.Fields{
 			"obj": spew.Sdump(newObj),

internal/monitors/kubernetes/cluster/metrics/node.go

@@ -1,12 +1,9 @@
 package metrics
 
 import (
-	"reflect"
-
 	"github.com/signalfx/golib/datapoint"
 	"github.com/signalfx/golib/sfxclient"
 	atypes "github.com/signalfx/signalfx-agent/internal/monitors/types"
-	log "github.com/sirupsen/logrus"
 	"k8s.io/api/core/v1"
 )
 
@@ -15,48 +12,70 @@ import (
 
 // DIMENSION(machine_id): The machine ID from /etc/machine-id.  This should be
 // unique across all nodes in your cluster, but some cluster deployment tools
-// don't guarantee this.
+// don't guarantee this.  This will not be sent if the `useNodeName` config
+// option is set to true.
+
+// DIMENSION(kubernetes_node): The name of the node, as defined by the `name`
+// field of the node resource.
 
-// PROPERTY(machine_id:<node label>): All non-blank labels on a given node will
-// be synced as properties to the `machine_id` dimension value for that node.
-// Any blank values will be synced as tags on that same dimension.
+// PROPERTY(machine_id/kubernetes_node:<node label>): All non-blank labels on a
+// given node will be synced as properties to the `machine_id` or
+// `kubernetes_node` dimension value for that node.  Which dimension gets the
+// properties is determined by the `useNodeName` config option.  Any blank
+// values will be synced as tags on that same dimension.
 
 // A map to check for duplicate machine IDs
-var machineIDToHostMap = make(map[string]string)
+var machineIDToNodeNameMap = make(map[string]string)
 
-func datapointsForNode(node *v1.Node) []*datapoint.Datapoint {
+func datapointsForNode(node *v1.Node, useNodeName bool) []*datapoint.Datapoint {
 	dims := map[string]string{
-		"host":       firstNodeHostname(node),
-		"machine_id": node.Status.NodeInfo.MachineID,
+		"host":            firstNodeHostname(node),
+		"kubernetes_node": node.Name,
+	}
+
+	// If we aren't using the node name as the node id, then we need machine_id
+	// to sync properties to.  Eventually we should just get rid of machine_id
+	// if it doesn't become more reliable and dependable across k8s deployments.
+	if !useNodeName {
+		dims["machine_id"] = node.Status.NodeInfo.MachineID
 	}
 
 	return []*datapoint.Datapoint{
 		sfxclient.Gauge("kubernetes.node_ready", dims, nodeConditionValue(node, v1.NodeReady)),
 	}
 }
 
-func dimPropsForNode(node *v1.Node) *atypes.DimProperties {
+func dimPropsForNode(node *v1.Node, useNodeName bool) *atypes.DimProperties {
 	props, tags := propsAndTagsFromLabels(node.Labels)
 
 	if len(props) == 0 && len(tags) == 0 {
 		return nil
 	}
 
-	host := firstNodeHostname(node)
-	machineID := node.Status.NodeInfo.MachineID
-
-	if otherHost, ok := machineIDToHostMap[machineID]; ok && otherHost != host {
-		log.Errorf("Your K8s cluster appears to have duplicate node machine IDs, "+
-			"host %s and %s both have machine ID %s.  This is probably kubelet's fault.", host, otherHost, machineID)
-		return nil
+	dim := atypes.Dimension{
+		Name:  "kubernetes_node",
+		Value: node.Name,
 	}
-	machineIDToHostMap[machineID] = host
 
-	return &atypes.DimProperties{
-		Dimension: atypes.Dimension{
+	if !useNodeName {
+		machineID := node.Status.NodeInfo.MachineID
+		dim = atypes.Dimension{
 			Name:  "machine_id",
 			Value: machineID,
-		},
+		}
+
+		if otherNodeName, ok := machineIDToNodeNameMap[machineID]; ok && otherNodeName != node.Name {
+			logger.Errorf("Your K8s cluster appears to have duplicate node machine IDs, "+
+				"node %s and %s both have machine ID %s.  Please set the `useNodeName` option "+
+				"in this monitor config and set the top-level config option `sendMachineID` to "+
+				"false.", node.Name, otherNodeName, machineID)
+		}
+
+		machineIDToNodeNameMap[machineID] = node.Name
+	}
+
+	return &atypes.DimProperties{
+		Dimension:  dim,
 		Properties: props,
 		Tags:       tags,
 	}
@@ -87,25 +106,3 @@ func firstNodeHostname(node *v1.Node) string {
 	}
 	return ""
 }
-
-// Nodes get updated a lot due to heartbeat checks that alter the
-// lastHeartbeatCheck field within condition items.  Also the images can
-// sometimes come in different orderings and we don't really care about them
-// anyway, so just get rid of them before comparing.
-func nodesDifferent(n1 *v1.Node, n2 *v1.Node) bool {
-	if n2 == nil {
-		return true
-	}
-	c1 := *n1
-	c2 := *n2
-
-	c1.ResourceVersion = c2.ResourceVersion
-
-	c1.Status.Conditions = nil
-	c2.Status.Conditions = nil
-
-	c1.Status.Images = nil
-	c2.Status.Images = nil
-
-	return !reflect.DeepEqual(c1, c2)
-}