cloud_controller_ng icon indicating copy to clipboard operation
cloud_controller_ng copied to clipboard

Add instance_id field in Process Stats response

Open tcdowney opened this issue 2 years ago • 5 comments

A short explanation of the proposed change:

  • Display the Diego instance identifier as part of the Process Stats response. This id is unique for each actual LRP instance and can be used as a substitute for the integer index id.
  • Eirini does not support this in their API at this time, so the OPI variant of the instances stats reporter returns an empty string for this field. In the future, if Eirini is enhanced to provide this we can update that client.
  • Issue: https://github.com/cloudfoundry/cloud_controller_ng/issues/2819

An explanation of the use cases your change solves:

The issue (https://github.com/cloudfoundry/cloud_controller_ng/issues/2819) describes this use case in more detail. The summary though is this can help developers debug their apps (this id is logged by app containers) and provide a pathway for clients to provide other container ids than just the integer index.

  • [x] I have reviewed the contributing guide

  • [x] I have viewed, signed, and submitted the Contributor License Agreement

  • [x] I have made this pull request to the main branch

  • [x] I have run all the unit tests using bundle exec rake

  • [ ] I have run CF Acceptance Tests

tcdowney avatar Jun 09 '22 22:06 tcdowney

@Gerg some additional context for this change:

On Korifi we're wanting to support workloads other than StatefulSets (e.g. Deployments, knative Services, etc.) so we're working toward reducing the reliance of system components on the numerical indexing of app instances. I'll be putting out a proposal / some sort of design doc to start discussing this in more detail later this week, but I see this change as a first step toward that.

tcdowney avatar Jun 21 '22 22:06 tcdowney

I'm putting this into draft state. I was looking at the AppRecipeBuilder and noticed that instance_id is that metric tag we're adding and it corresponds to instance index. We are calling this the process_instance_id on metrics/logs so maybe that name should be used. Worth discussing I think.

https://github.com/cloudfoundry/cloud_controller_ng/blob/7698bc407195cb30de7455ab877d9526b6d4bafe/lib/cloud_controller/diego/app_recipe_builder.rb#L108

tcdowney avatar Jun 23 '22 23:06 tcdowney

It's a little odd that we call index instance_id in the metrics tags. Looks like it came from this story.

index/guid is the most intuitive to me, but I could see other options. Process.instance.process_instance_id sounds a bit redundant.

Gerg avatar Jun 29 '22 22:06 Gerg

@Gerg

Yeah, I started calling it instance_guid here in the proposal around this stuff here: https://docs.google.com/document/d/118yYN4WGj1F2si8gk94xFZsYdewtOKoqO2Dv-1I-PgE/edit

tcdowney avatar Jun 29 '22 22:06 tcdowney

What about just guid?

Gerg avatar Jun 29 '22 22:06 Gerg