Set instance_id in flintlock #394

richardcase · 2022-02-15T15:49:19Z

What this PR does / why we need it:

This change will set the instance_id in the metadata if its not set to the UID for the microvm

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist:

squashed commits into logical changes
includes documentation
adds unit tests
adds or updates e2e tests

codecov-commenter · 2022-02-15T15:53:46Z

Codecov Report

Merging #394 (29fc4f2) into main (f9607be) will increase coverage by 0.17%.
The diff coverage is 57.57%.

❗ Current head 29fc4f2 differs from pull request most recent head 9aae37c. Consider uploading reports for the commit 9aae37c to get more accurate results

@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
+ Coverage   56.71%   56.89%   +0.17%     
==========================================
  Files          52       52              
  Lines        2599     2626      +27     
==========================================
+ Hits         1474     1494      +20     
- Misses       1001     1005       +4     
- Partials      124      127       +3

Impacted Files	Coverage Δ
infrastructure/firecracker/config.go	`0.00% <0.00%> (ø)`
core/application/commands.go	`71.57% <69.23%> (-0.89%)`	⬇️
infrastructure/grpc/convert.go	`42.30% <100.00%> (+0.31%)`	⬆️
pkg/process/process.go	`39.62% <0.00%> (+1.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9607be...9aae37c. Read the comment docs.

bigkevmcd

Looks good, nothing major.

bigkevmcd · 2022-02-16T08:33:02Z

core/application/app_test.go

 	"errors"
 	"testing"
 	"time"

 	"github.com/golang/mock/gomock"
 	. "github.com/onsi/gomega"
 	"github.com/spf13/afero"
+	"gopkg.in/yaml.v2"


I'd probably use https://pkg.go.dev/sigs.k8s.io/yaml instead of v2 of go-yaml

Changed to use this wrapper.

bigkevmcd · 2022-02-16T08:35:15Z

core/application/commands.go

@@ -60,6 +65,10 @@ func (a *app) CreateMicroVM(ctx context.Context, mvm *models.MicroVM) (*models.M
 		}
 	}

+	if addErr := a.addInstanceData(mvm, logger); addErr != nil {


I'm not sure there's any benefit in addErr here?

You've already returned if err wasn't nil earlier, which implies you don't want to retain the value ?

That was a mistake but i have changed this to reuse err

bigkevmcd · 2022-02-16T08:38:28Z

core/application/commands.go

+	instanceData := &cloudinit.Metadata{}
+
+	meta, ok := vm.Spec.Metadata["meta-data"]
+	if ok {


Could you drop the comma, ok mechanism here, and check for vm.Spec.Metadata["meta-data"] not being ""?

You can decode "" but it would fail the YAML decoding, which implies that "" is not considered a valid value

Changed the logic here based on your suggestion

bigkevmcd · 2022-02-16T08:39:45Z

core/application/commands.go

+
+	meta, ok := vm.Spec.Metadata["meta-data"]
+	if ok {
+		logger.Info("Instance metadata exists")


Is there any extra logging data that could come out here? the MicroVM ID perhaps?

Added.

where?

Callisto13 · 2022-02-18T10:27:55Z

infrastructure/grpc/convert.go

@@ -32,6 +32,7 @@ func convertMicroVMToModel(spec *types.MicroVMSpec) (*models.MicroVM, error) {
 			},
 			VCPU:       int64(spec.Vcpu),
 			MemoryInMb: int64(spec.MemoryInMb),
+			Metadata:   map[string]string{},


This could technically be an instance.New() i guess

Callisto13

What's the plan for those MetadataOption funcs?

richardcase · 2022-02-18T14:22:45Z

What's the plan for those MetadataOption funcs?

I'm going to change CAPMVM to use them :)

Callisto13 · 2022-02-18T14:23:39Z

ah fab

yitsushi · 2022-02-18T14:32:46Z

client/cloudinit/instance/metadata.go

+			im[k] = v
+		}
+	}
+}


I think a custom WithKeyVaue or WithCustomField would be useful. Right now we are not using anything else, but in production ppl might want to define more data.

func WithKeyValue(key, value string) MetadataOption { return func(im Metadata) { im[key] = value } }

potentially all WithXXX can use that as well, so if we need validation we can do it in one place (like don't add if the value is empty)

Thats a good point. Let me change that.

yitsushi

It's funny to see my original map solution comes back and says "hi" 😄

When a request is made to create a microvm we will automatically add the UID of the microvm to the instance meta-data with the instance_id key if its not already set. Small refactor of the cloudinit package. Signed-off-by: Richard Case <richard@weave.works>

bigkevmcd · 2022-02-18T09:39:00Z

client/cloudinit/instance/metadata_test.go

+		instance.WithPlatform(testPlatformName),
+	)
+	Expect(m).NotTo(BeNil())
+	Expect(m).To(HaveLen(5))


You could simplify this to just HaveLen(5) because it can't be both nil and have len 5

The one above is fine (NotTo(BeNil())) because you want to ensure that it's a len 0 slice, and nil has len 0.

(There is another below).

richardcase added the kind/feature New feature or request label Feb 15, 2022

richardcase marked this pull request as ready for review February 15, 2022 18:01

richardcase changed the title ~~[WIP] Set instance_id in flintlock~~ Set instance_id in flintlock Feb 15, 2022

bigkevmcd reviewed Feb 16, 2022

View reviewed changes

Callisto13 reviewed Feb 18, 2022

View reviewed changes

Callisto13 previously approved these changes Feb 18, 2022

View reviewed changes

yitsushi reviewed Feb 18, 2022

View reviewed changes

yitsushi previously approved these changes Feb 18, 2022

View reviewed changes

richardcase dismissed stale reviews from yitsushi and Callisto13 via 9aae37c February 18, 2022 17:08

bigkevmcd reviewed Feb 21, 2022

View reviewed changes

Callisto13 approved these changes Feb 21, 2022

View reviewed changes

yitsushi approved these changes Feb 21, 2022

View reviewed changes

richardcase merged commit 66f81a6 into liquidmetal-dev:main Feb 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set instance_id in flintlock #394

Set instance_id in flintlock #394

richardcase commented Feb 15, 2022

codecov-commenter commented Feb 15, 2022 •

edited

Loading

bigkevmcd left a comment

bigkevmcd Feb 16, 2022

richardcase Feb 16, 2022

bigkevmcd Feb 16, 2022

richardcase Feb 16, 2022

bigkevmcd Feb 16, 2022 •

edited

Loading

richardcase Feb 16, 2022

bigkevmcd Feb 16, 2022

richardcase Feb 16, 2022

Callisto13 Feb 18, 2022

Callisto13 Feb 18, 2022

Callisto13 left a comment

richardcase commented Feb 18, 2022

Callisto13 commented Feb 18, 2022

yitsushi Feb 18, 2022

richardcase Feb 18, 2022

yitsushi left a comment

bigkevmcd Feb 18, 2022

Set instance_id in flintlock #394

Set instance_id in flintlock #394

Conversation

richardcase commented Feb 15, 2022

codecov-commenter commented Feb 15, 2022 • edited Loading

Codecov Report

bigkevmcd left a 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

bigkevmcd Feb 16, 2022 • edited Loading

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

Callisto13 left a comment

Choose a reason for hiding this comment

richardcase commented Feb 18, 2022

Callisto13 commented Feb 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitsushi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 15, 2022 •

edited

Loading

bigkevmcd Feb 16, 2022 •

edited

Loading