fix: parse instance types from VM config's annotation - #1
Conversation
| return "" | ||
| } | ||
|
|
||
| // Try JSON first |
There was a problem hiding this comment.
Lets stick to the 1 format for now. Let say yaml
There was a problem hiding this comment.
I would agree right away, but let me share my findings first, since I invested some time in that already.
The annotation field of VirtualMachineConfigInfo object is defined that way:
`// Description for the virtual machine.
Annotation string `xml:"annotation,omitempty" json:"annotation,omitempty"``
Which kinda hints me that it must be XML or JSON.
This config is different from the one that you showed me defined in apex-fleet-apps/vsphere-config/templates/vsphere-cloud-configmap.yaml. That one was indeed YAML, but , AFAIU, it is mapped to another type pkg/common/config/types_yaml.go where fields are defined as YAML indeed.
E.g.
type GlobalYAML struct { SecretNamespace string yaml:"secretNamespace"``
Less code is always better and yes I am glad to stick with one format. It is just I still cannot understand which format to choose.
There was a problem hiding this comment.
VM object is xml on vsphere side.
Annotation is string (including multiline value)
There was a problem hiding this comment.
Then it sounds like it is a choice between YAML and human readable string incl. multiline.
YAML does seem to fit here well because of counterexample below:
cloned_from:value \n instanceType: value
This is invalid YAML, because first line doesn't have space between : and value. This seems to be our valid case used for cloned_from field.
It looks like the only format must be the multiline human readable string is the way, right?
| var instanceType string | ||
| if instanceType = getInstanceType(oVM.Config.Annotation); instanceType == "" { | ||
| // store instance type in nodeinfo map | ||
| instanceType = fmt.Sprintf("vsphere-vm.cpu-%d.mem-%dgb.os-%s", | ||
| oVM.Summary.Config.NumCpu, | ||
| (oVM.Summary.Config.MemorySizeMB / 1024), | ||
| os, | ||
| ) | ||
| } |
There was a problem hiding this comment.
If MemorySizeMB will be 3000 than instanceType would be vsphere-vm.cpu-5.mem-2gb.os-linux (2 gigs). Is that expected ?
| var instanceType string | ||
| if instanceType = getInstanceType(oVM.Config.Annotation); instanceType == "" { |
There was a problem hiding this comment.
maybe:
var instanceType = getInstanceType(oVM.Config.Annotation);
if len(instanceType) == 0 {
...
}| instanceType = fmt.Sprintf("vsphere-vm.cpu-%d.mem-%dgb.os-%s", | ||
| oVM.Summary.Config.NumCpu, | ||
| (oVM.Summary.Config.MemorySizeMB / 1024), | ||
| os, |
There was a problem hiding this comment.
maybe getInstanceType function can return this as fallback instead of returning "" and later test for ""
| var jsonData map[string]any | ||
| if err := json.Unmarshal([]byte(annotation), &jsonData); err == nil { | ||
| if instanceTypeFieldValue, ok := jsonData[instanceTypeFieldKey].(string); ok && instanceTypeFieldValue != "" { | ||
| return instanceTypeFieldValue | ||
| } | ||
| } |
There was a problem hiding this comment.
if json is invalid e.g: { "key":"value",} - the code rerurns "" (or some fallback). Is that correct to ignore invalid inputs ? Shouldnt we propagate error up and fail fast (FailFast practice) ?
| var yamlData map[string]any | ||
| if err := yaml.Unmarshal([]byte(annotation), &yamlData); err == nil { | ||
| if instanceTypeFieldValue, ok := yamlData[instanceTypeFieldKey].(string); ok && instanceTypeFieldValue != "" { | ||
| return instanceTypeFieldValue | ||
| } | ||
| } |
3 participants
What this PR does / why we need it:
Instance type must be parsed from VM config's annotation first.
If it is present and non-empty, it is used as NodeType; otherwise it fallsback to the existing vsphere-vm.cpu-%d.mem-%dgb.os-%s construction.
Special notes for your reviewer:
Release note: