-
Notifications
You must be signed in to change notification settings - Fork 12
sap_vm_provision/kubevirt_vm: Make usage of hugepages configurable #147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sap_vm_provision/kubevirt_vm: Make usage of hugepages configurable #147
Conversation
marcelmamula
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review done with few changes to be made
roles/sap_vm_provision/tasks/platform_ansible/kubevirt_vm/execute_provision.yml
Outdated
Show resolved
Hide resolved
marcelmamula
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last name change before LGTM
roles/sap_vm_provision/tasks/platform_ansible/kubevirt_vm/execute_provision.yml
Outdated
Show resolved
Hide resolved
…vision_fact_memory_definition
…ook endpoint name waiting
|
Hi @marcelmamula, i've renamed the var's as suggested, please take a look. |
|
@newkit Rename makes change, but comments are not aligned with it now, because key name is hypervisor, but comments says it is for VM inside of it. I would see it like, this: Clear and does not confuse with having 2 keys with same name. kubevirt_vm_cpu_cores: 2
kubevirt_vm_memory_gib: 24
# Defines size of huge pages for kubervirt hypervisor (e.g. '2Gi')
# This can be disabled by setting value at 'false'. The default value is '1Gi'.
kubevirt_vm_memory_hypervisor_hugepages: 1Gi@sean-freeman It would better to continue in one thread instead of jumping between PR and Issue. |
…id CPU/NUMA configuration
|
Done @marcelmamula |
marcelmamula
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@newkit Add review comments and also adding @sean-freeman into reviewers as he is involved.
roles/sap_vm_provision/tasks/platform_ansible/kubevirt_vm/execute_provision.yml
Outdated
Show resolved
Hide resolved
roles/sap_vm_provision/tasks/platform_ansible/kubevirt_vm/execute_provision.yml
Show resolved
Hide resolved
roles/sap_vm_provision/tasks/platform_ansible/kubevirt_vm/execute_main.yml
Outdated
Show resolved
Hide resolved
|
@newkit Any progress on fixing this task I reported last week? Its message needs improving. - name: Stop execution of playbook here if an error has occurred
ansible.builtin.fail:
msg: "Stop entire playbook since something when wrong." |
@marcelmamula I've updated the error message. |
marcelmamula
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This addresses the enhancement described here:
When creating a VM, currently the usage of hugepages is mandatory, see here.
Let's add a flag in to the VM definition to switch off the usage of hugepages:
When omitted / by default hugepages should be on to be backwards compatible.