Integrating Simulacra with Diagnostic Tool Output #1363

shafinsiddique · 2023-08-03T16:35:14Z

Description

We don’t want the Ops Agent on-call team to manually have to create this configuration file. Currently, we have a diagnostic tool that our customers run whenever their Ops Agent is not behaving accordingly. The diagnostic tool collects various information about the customer's environment and outputs it into different files.

We can integrate the diagnostic tool with Simulacra so that the Simulacra configuration file can be automatically created from the information that the diagnostic tool generates. This would automate the process of manually creating Simulacra config files.

This PR adds support in Simulacra for reading diagnostic tool output and configuring itself from that output.

How has this been tested?

Used a diagnostic tool output and passed it to Simulacra. It created the VM correctly.

Checklist:

Unit tests
- Unit tests do not apply.
- Unit tests have been added/modified and passed for this PR.
Integration tests
- Integration tests do not apply.
- Integration tests have been added/modified and passed for this PR.
Documentation
- This PR introduces no user visible changes.
- This PR introduces user visible changes and the corresponding documentation change has been made.
Minor version bump
- This PR introduces no new features.
- This PR introduces new features, and there is a separate PR to bump the minor version since the last release already.
- This PR bumps the version.

cmd/simulacra/simulacra.go

braydonk · 2023-08-03T18:25:02Z

integration_test/gce/gce_testing.go

+func getImageFamilyName(image string) string {
+	if isFullImageName(image) {
+		components := strings.Split(image, "/")
+		return components[len(components)-1]


So the last component of the array is the image family name? I thought that was the image name itself

okay this was definitely poor naming on my part,

this function essentially does 2 things : it either gets the image name if prefix "/project" is in the string OR it gets family name if that prefix is not there.

could I get an example of what image looks like in a case where it starts with projects/?

I have a feeling that you will need to specify options.ImageProject in cases like this...

Yep! So this is what the image looks like most of the time coming from the metadata file : " projects/debian-cloud/global/images/debian-11-bullseye-v20230711"

OK so yeah the "debian-cloud" part should get passed in as options.ImageProject. The "global" part should probably get passed in to --image-family-scope, though there's no VMOptions for that yet.

I think we should parse this string projects/debian-cloud/global/images/debian-11-bullseye-v20230711 in simulacra.go, adding some things to VMOptions in the process:

a TODO to rename VMOptions.Platform -> ImageFamily (overdue)

VMOptions.Image: an alternative to VMOptions.Platform

VMOptons.ImageFamilyScope: doesn't seem very important, but it's probably technically correct to include it. It's kind of described here: https://screenshot.googleplex.com/BEPtcVek32VbZnr

Then we'd need to change some things at the start of attemptCreateInstance() to:

check that one of VMOptions.Platform and VMOptions.Image are specified, not 0 or 2 of them

put the nonempty one into VM.Platform (similar to today, vm.Platform would continue to hold either an image family or a specific image),

change the code that computes imageOrImageFamilyFlag to look at the VMOptions fields instead of VM.Platform

add some code to turn VMOptions.ImageFamilyScope into --image-family-scope.

In addition, I think you'd need some light modifications to simulacra.go:

new fields in Config called Image and ImageProject, I guess ImageFamilyScope too

some code around here to parse metadata.image into Image/ImageProject/ImageFamilyScope: https://screenshot.googleplex.com/7MVkQHDG6AUV6oK

sorry to ask for relatively big changes. I think it's important to make gce_testing.go as "dumb" as we can and avoid smuggling features into it if we can.

Happy to discuss and/or help out.

This is a great idea! I'm working on implementing these rn, thank you for describing it in such details

Hey @martijnvans , I have made all the suggested changes with the exception of renaming Platform in VMOption to 'ImageFamily.'

Since other files use that field name, it would be a big change for this PR.

I have created a task https://buganizer.corp.google.com/issues/295174028 and assigned it to myself to make that change in a seperate PR.

Thanks! I left a comment about a suggested 3-step approach.

martijnvans · 2023-08-03T21:05:03Z

integration_test/gce/gce_testing.go

+func getImageFamilyName(image string) string {
+	if isFullImageName(image) {
+		components := strings.Split(image, "/")
+		return components[len(components)-1]


could I get an example of what image looks like in a case where it starts with projects/?

I have a feeling that you will need to specify options.ImageProject in cases like this...

martijnvans · 2023-08-03T21:11:57Z

integration_test/gce/gce_testing.go

 // attemptCreateInstance creates a VM instance and waits for it to be ready.
 // Returns a VM object or an error (never both). The caller is responsible for
 // deleting the VM if (and only if) the returned error is nil.
 func attemptCreateInstance(ctx context.Context, logger *log.Logger, options VMOptions) (vmToReturn *VM, errToReturn error) {
 	vm := &VM{
 		Project:  options.Project,
-		Platform: options.Platform,
+		Platform: getImageFamilyName(options.Platform),


I would prefer keeping this line as is (Platform: options.Platform), and doing something like this down below (line 1130ish):

if isFullImageName(options.Platform) { components := strings.Split(options.Platform, "/") imageOrImageFamilyFlag = "--image=" + components[len(components)-1] // maybe parse out the image project too? if so, this chunk and lines 1124-1127 should move up above line 1107. Unless I'm misunderstanding and the image project isn't part of this string in the first place. }

I think if we did this, it would become inconsistent throughout the rest of the function. The vm.Platform value would be like this : " projects/debian-cloud/global/images/debian-11-bullseye-v20230711". Then in line 1213/1227/1246, we would get a runtime error because they are expecting the vm.platform value to be just the image family name but instead it's that weird format from the metadata output. This is why i think we need to set vm.Platform to the image family name when we create the vm struct from the options input.

true true. I thought about this a lot, see my other comment.

martijnvans · 2023-08-08T16:32:43Z

integration_test/gce/gce_testing.go

+func getImageFamilyName(image string) string {
+	if isFullImageName(image) {
+		components := strings.Split(image, "/")
+		return components[len(components)-1]


OK so yeah the "debian-cloud" part should get passed in as options.ImageProject. The "global" part should probably get passed in to --image-family-scope, though there's no VMOptions for that yet.

I think we should parse this string projects/debian-cloud/global/images/debian-11-bullseye-v20230711 in simulacra.go, adding some things to VMOptions in the process:

a TODO to rename VMOptions.Platform -> ImageFamily (overdue)

VMOptions.Image: an alternative to VMOptions.Platform

VMOptons.ImageFamilyScope: doesn't seem very important, but it's probably technically correct to include it. It's kind of described here: https://screenshot.googleplex.com/BEPtcVek32VbZnr

Then we'd need to change some things at the start of attemptCreateInstance() to:

check that one of VMOptions.Platform and VMOptions.Image are specified, not 0 or 2 of them

put the nonempty one into VM.Platform (similar to today, vm.Platform would continue to hold either an image family or a specific image),

change the code that computes imageOrImageFamilyFlag to look at the VMOptions fields instead of VM.Platform

add some code to turn VMOptions.ImageFamilyScope into --image-family-scope.

In addition, I think you'd need some light modifications to simulacra.go:

new fields in Config called Image and ImageProject, I guess ImageFamilyScope too

some code around here to parse metadata.image into Image/ImageProject/ImageFamilyScope: https://screenshot.googleplex.com/7MVkQHDG6AUV6oK

sorry to ask for relatively big changes. I think it's important to make gce_testing.go as "dumb" as we can and avoid smuggling features into it if we can.

Happy to discuss and/or help out.

martijnvans · 2023-08-08T16:32:47Z

integration_test/gce/gce_testing.go

 // attemptCreateInstance creates a VM instance and waits for it to be ready.
 // Returns a VM object or an error (never both). The caller is responsible for
 // deleting the VM if (and only if) the returned error is nil.
 func attemptCreateInstance(ctx context.Context, logger *log.Logger, options VMOptions) (vmToReturn *VM, errToReturn error) {
 	vm := &VM{
 		Project:  options.Project,
-		Platform: options.Platform,
+		Platform: getImageFamilyName(options.Platform),


true true. I thought about this a lot, see my other comment.

cmd/simulacra/simulacra.go

integration_test/gce/gce_testing.go

cmd/simulacra/simulacra.go

braydonk · 2023-08-17T14:35:12Z

cmd/simulacra/simulacra.go

@@ -204,9 +217,61 @@ func getConfigFromYaml(configPath string) (*Config, error) {
 	return &config, nil
 }

+// Parse the metadata image name with format 'projects/debian-cloud/global/images/debian-11-bullseye-v20230711'


How certain are we that the image will always be in this format? Have we looked at any custom images to make sure they follow this format too?

Discussed offline. What we ended up doing was if the image name in the metadata file does not have the prefix "/projects", then we ask the user (using command line input) for the image family.

cmd/simulacra/simulacra.go

integration_test/gce/gce_testing.go

Co-authored-by: Braydon Kains <[email protected]>

shafinsiddique requested review from ridwanmsharif, martijnvans and braydonk August 3, 2023 16:35

braydonk reviewed Aug 3, 2023

View reviewed changes

martijnvans reviewed Aug 3, 2023

View reviewed changes

shafinsiddique added 3 commits August 8, 2023 14:10

integrated simulacra with diagnostic tool output.

9436706

rm spaces.

2b3b145

changes according to PR.

a7ce7dd

shafinsiddique force-pushed the shafinsiddique-diagnostic-integration branch from 8f7eb4a to a7ce7dd Compare August 8, 2023 14:10

martijnvans requested changes Aug 8, 2023

View reviewed changes

shafinsiddique added 3 commits August 9, 2023 15:52

added new image field to VMOptions and other changes based on that.

26c2ce0

rm unnneseccary function.

d7cbf2c

image family scope on the simulacra side

c56f710

martijnvans approved these changes Aug 9, 2023

View reviewed changes

cmd/simulacra/simulacra.go Outdated Show resolved Hide resolved

cmd/simulacra/simulacra.go Outdated Show resolved Hide resolved

add image project.

ebc44e5

martijnvans reviewed Aug 14, 2023

View reviewed changes

cmd/simulacra/simulacra.go Outdated Show resolved Hide resolved

change ordering

6114791

martijnvans reviewed Aug 14, 2023

View reviewed changes

integration_test/gce/gce_testing.go Outdated Show resolved Hide resolved

integration_test/gce/gce_testing.go Show resolved Hide resolved

change from vm.platform -> options.platform.

0159703

braydonk reviewed Aug 17, 2023

View reviewed changes

shafinsiddique and others added 4 commits August 17, 2023 11:14

Update error

6ef5ca5

Co-authored-by: Braydon Kains <[email protected]>

update error

e9fab58

Co-authored-by: Braydon Kains <[email protected]>

command line input for image family.

3a05040

change path to filepath lib

e5792b4

braydonk approved these changes Aug 23, 2023

View reviewed changes

Merge branch 'master' into shafinsiddique-diagnostic-integration

7979d3d

shafinsiddique merged commit c7a3989 into master Aug 25, 2023
9 checks passed

ridwanmsharif deleted the shafinsiddique-diagnostic-integration branch July 23, 2024 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrating Simulacra with Diagnostic Tool Output #1363

Integrating Simulacra with Diagnostic Tool Output #1363

shafinsiddique commented Aug 3, 2023

braydonk Aug 3, 2023

shafinsiddique Aug 3, 2023

martijnvans Aug 3, 2023

shafinsiddique Aug 4, 2023

martijnvans Aug 8, 2023

shafinsiddique Aug 9, 2023

shafinsiddique Aug 9, 2023

martijnvans Aug 9, 2023

martijnvans Aug 3, 2023

martijnvans Aug 3, 2023

shafinsiddique Aug 8, 2023

martijnvans Aug 8, 2023

martijnvans Aug 8, 2023

martijnvans Aug 8, 2023

braydonk Aug 17, 2023

shafinsiddique Aug 21, 2023

Integrating Simulacra with Diagnostic Tool Output #1363

Integrating Simulacra with Diagnostic Tool Output #1363

Conversation

shafinsiddique commented Aug 3, 2023

Description

How has this been tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment