vSphere: FindTemplate should search for a desktop template if no server templates are found #428
Conversation
kubevirt-bot
added
dco-signoff: yes
mansam
force-pushed
the
vsphere-template-finder-nil-workload
branch
from
df2a2b8
to
555043c
Compare
mansam
changed the title
| @@ -40,8 +39,7 @@ func (f *TemplateFinder) FindTemplate(vm *mo.VirtualMachine) (*templatev1.Templa | |||
| // We update metadata from the source vm so we default to medium flavor | |||
| namespace := TemplateNamespace | |||
| flavor := defaultFlavor | |||
| workload := defaultWorkload | |||
| tmpls, err := f.templateProvider.Find(&namespace, &os, &workload, &flavor) | |||
| tmpls, err := f.templateProvider.Find(&namespace, &os, nil, &flavor) | |||
There was a problem hiding this comment.
With nil workload the provider will return multiple matching templates, from which only the newest would be picked. I think that resigning from controlling the workload type will lead to less predictable behaviour and application of suboptimal templates (with CPU flags tuned to a completely different workload) to imported VMs.
I can imagine that user adds a new "desktop"-like template for a certain OS to the configuration and later all VMware imports for that OS use that template, even if most of them require server-type optimizations.
server is generally a quite safe default and I know that it has similar (but probably less severe) issues as described above (i.e. performance of Windows desktop VMs might suffer).
Maybe introduction of an optional workload property to the VirtualMachineImport would help here? It would give the user control over the workload type, if needed.
@pkliczewski, @machacekondra, @masayag, @fdupont-redhat - what do you think? Maybe it's actually not a real issue.
There was a problem hiding this comment.
I agree with @jakub-dzon. It is important to use proper workload type since optimizations part of the template are based on the workload type. I like the idea with workload property or having default workloads for specific OS types.
There was a problem hiding this comment.
@pkliczewski @jakub-dzon as a stopgap until a workload property can be added or default workloads can be determined for each OS, would it be acceptable to default to looking for a server template, and then fall back to looking for a desktop template if nothing is returned?
There was a problem hiding this comment.
@mansam I think we could try with this approach. @jakub-dzon what do you think?
There was a problem hiding this comment.
Maybe introduction of an optional
workloadproperty to the VirtualMachineImport would help here? It would give the user control over the workload type, if needed.
I'd prefer exposing template identifier (name + namespace) instead of specific properties for identifying the template.
Else, we'd end up with exposing the templates attributes used as field selectors.
Without having a template specified by the user, @mansam's approach to use server and to fallback to desktop sounds good to me.
There was a problem hiding this comment.
I'm okay with that approach. Moti's suggestion to pass template identifier instead of workload makes more sense than just the workload; it can provide easy workaround for any potential template resolution issues.
There was a problem hiding this comment.
As far as I remember we were explicitly asked not to use the name. @omeryahud Do you have any suggestions how to handle this?
There was a problem hiding this comment.
As far as I remember we were explicitly asked not to use the name. @omeryahud Do you have any suggestions how to handle this?
Yes, using template names is generally not a good idea, because the names of the latest templates may change as new template bundles are released, and if you allow the user to choose the template used, they might not pick the latest template.
If you decide to accept template properties from the user, it should be the os/workload/flavor labels.
Just FYI, we are currently working on some things that were mentioned here:
- Default template for each OS - https://issues.redhat.com/browse/CNV-6153
- Having fixed template names - https://issues.redhat.com/browse/CNV-6624
mansam
force-pushed
the
vsphere-template-finder-nil-workload
branch
from
555043c
to
2a9f90d
Compare
mansam
changed the title
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
mansam
force-pushed
the
vsphere-template-finder-nil-workload
branch
2 times, most recently
from
839c27e
to
378fcde
Compare
|
Not sure why there's an ovirt test failing, none of that code's been touched by this PR. /retest |
pkliczewski
added
the
release-note-none
kubevirt-bot
removed
the
do-not-merge/release-note-label-needed
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jakub-dzon, mansam, masayag, pkliczewski The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The vSphere provider FindTemplate method should pass a nil workload to the template provider, since it can't get the workload from the vSphere API and setting a default can result in failing to find a valid template.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1890486
Signed-off-by: Sam Lucidi slucidi@redhat.com