Support Deployments

[gcapizzi]

The Eirini team is planning to support Kubernetes Deployments in addition to StatefulSets for workloads. In order to do so, metric-proxy would have to support Deployments too.

This would mean introducing a feature flag and changing the way the instance indexes are determined when it is turned on. In particular, this function:

func getInstanceID(podMetric v1beta1.PodMetrics) string {
	s := strings.Split(podMetric.Name, "-")
	return s[len(s)-1]
}

Pods created by Deployments don't have a numeric index in their name, but an 5 characters alphanumeric ID. This can be translated to a numeric ID by treating it as a base 36 number, leading to indexes ranging from 1 to 60,466,177. These indexes would still be unique, but they wouldn't be stable (they would change every time an instance is recreated for any reason), ordered or contiguous.

We couldn't find any contraindications to this in metric-proxy, what do you think? We'd be happy to PR this as part of our track of work is the team agrees!

For more context, see cloudfoundry/eirini-release#207.

Thanks!
/cc @cloudfoundry/eirini

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/177909652

The labels on this github issue will be updated when the story is started.

[tcdowney]

@gcapizzi is there a particular reason it needs to be translated to an integer instead of just keeping the alphanumeric ID? I feel that may lead to more confusion since it will look like a fragmented amount of indices for the app. Like I'm imagining an operator seeing indices that appear valid but don't start at 0 and jump around and being confused.

A hard switch to the alphanumeric ID may give them a better idea that these IDs are not stable and have no inherent meaning.

Cloud Controller should be able to handle this fine as long as the indices are consistent between metric-proxy and Eirini.
https://github.com/cloudfoundry/cloud_controller_ng/blob/637fe7d6449a593e89fc344d0745aadfad221f5c/lib/cloud_controller/diego/reporters/instances_stats_reporter.rb#L57

I guess the CLI may have issues with it, though. 😞
https://github.com/cloudfoundry/cli/blob/80dddc726c1668dc68b085c3fb267c1ef5af8756/api/cloudcontroller/ccv3/process_instance.go#L25

[tcdowney]

cc our product-minded folks @emalm, @piyalibanerjee, @angelachin, @rainmaker 🙂

[emalm]

I see the benefits of moving to a more general alphanumeric identifier for app instances, and it would certainly reduce confusion compared to non-contiguous integer IDs. (It could even apply to Diego-based apps today, with the existing GUID-format instance IDs!) I had talked to @gcapizzi and @mnitchev about the idea of providing the alphanumeric identifier in an additional response field, but also providing the corresponding pure-numeric one in the existing field so as not to break clients that expect it to be an integer type, so that might be an interesting option to pursue.

[tcdowney]

Related Cloud Controller issue: cloudfoundry/cloud_controller_ng#2247

[davewalter]

@gcapizzi We just wanted to follow up on this to see if there was anything you needed us to do at this time, or whether you are still discussing the best solution?

We also wanted to make sure that you were including log-cache in the list of components that would require a change. It uses the same API as metric-proxy, so we think that it would need similar changes. Our understanding is that Cloud Controller would get confused if the instance index was not in sync between logging and metrics.

[gcapizzi]

It looks like we need to do some more research here, including testing alphanumeric indexes. I'm going to close these issues for now!