You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "rohityadavcloud (via GitHub)" <gi...@apache.org> on 2023/08/03 11:12:47 UTC
[GitHub] [cloudstack] rohityadavcloud opened a new pull request, #7810: Issue workaround
rohityadavcloud opened a new pull request, #7810:
URL: https://github.com/apache/cloudstack/pull/7810
This makes the KVM guru pass the max memory to KVM agent by picking the max. memory from the VM transfer object. This lets the cluster/agent decide if VM should start with the min or max memory depending on whether vm.memballoon.disable is false (default) or true.
When vm.memballoon.disable is true, VMs start with max. memory. Right now the max memory isn't sent to the KVM agent causing the regression. This feature is defined at https://github.com/apache/cloudstack/blob/main/agent/conf/agent.properties#L220
Also fixes https://github.com/apache/cloudstack/issues/7430 if user sets `vm.memballoon.disable` to true in agent.properties.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665189695
@DaanHoogland a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1664294148
@rohityadavcloud a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1663916691
I would suggest not to change the default value. @rohityadavcloud
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1284154489
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2800,11 +2795,17 @@ protected GuestResourceDef createGuestResourceDef(VirtualMachineTO vmTO){
}
protected long getCurrentMemAccordingToMemBallooning(VirtualMachineTO vmTO, long maxRam) {
- if (_noMemBalloon) {
- s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ if (_noMemBalloon || (vmTO != null && vmTO.getType() != VirtualMachine.Type.User)) {
Review Comment:
ok, looks good and let's merge. I'll test that scenario anyway in the env I am building right now; https://github.com/apache/cloudstack/pull/7810#issuecomment-1665188783
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1664921031
<b>[SF] Trillian test result (tid-7253)</b>
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server r8
Total time taken: 50690 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7810-t7253-vmware-67u3.zip
Smoke tests completed. 103 look OK, 5 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_add_primary_storage_disabled_host | `Error` | 0.23 | test_primary_storage.py
test_03_migration_options_storage_tags | `Error` | 0.02 | test_primary_storage.py
test_02_list_snapshots_with_removed_data_store | `Error` | 10.63 | test_snapshots.py
test_02_list_snapshots_with_removed_data_store | `Error` | 10.63 | test_snapshots.py
test_01_non_strict_host_anti_affinity | `Failure` | 138.13 | test_nonstrict_affinity_group.py
test_02_non_strict_host_affinity | `Error` | 100.08 | test_nonstrict_affinity_group.py
test_01_deploy_vm_on_specific_host | `Error` | 0.11 | test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster | `Error` | 0.12 | test_vm_deployment_planner.py
test_03_live_migrate_VM_with_two_data_disks | `Error` | 59.98 | test_vm_life_cycle.py
test_08_migrate_vm | `Error` | 0.06 | test_vm_life_cycle.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1284106377
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java:
##########
@@ -1238,7 +1238,7 @@ public String getMemBalloonStatsPeriod() {
@Override
public String toString() {
StringBuilder memBalloonBuilder = new StringBuilder();
- memBalloonBuilder.append("<memballoon model='" + memBalloonModel + "' autodeflate='on'>\n");
+ memBalloonBuilder.append("<memballoon model='" + memBalloonModel + "'>\n");
Review Comment:
@harikrishna-patnala , yes as shown by
https://github.com/apache/cloudstack/pull/7810#issuecomment-1664869520
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1284098812
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java:
##########
@@ -1238,7 +1238,7 @@ public String getMemBalloonStatsPeriod() {
@Override
public String toString() {
StringBuilder memBalloonBuilder = new StringBuilder();
- memBalloonBuilder.append("<memballoon model='" + memBalloonModel + "' autodeflate='on'>\n");
+ memBalloonBuilder.append("<memballoon model='" + memBalloonModel + "'>\n");
Review Comment:
Possibly - we need to check/test cc @DaanHoogland
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1664292430
I did whole lot of testing; the VM ballooning feature isn't dynamic or automatic like I thought earlier and the only feasible feature is to have dynamic scaling of VMs (that's manual); so as a best compromise this PR tries to do the following;
1. Pass the maxMem to KVMGuru that sends the right max. memory to KVM agent
2. When memory ballooning is disable or when the VM isn't a User VM, then use the maxMemory (new/tweaked behaviour)
3. When memory balloning is enabled and VM is a User VM type, then use min memory (old behaviour)
Ideally I think the ballooning feature should be disabled esp if overcommit is used; and use should log or document such as case for operators/users to be aware of (i.e. what to expect from the memory ballooning feature in case of KVM, and its behaviour with memory overcommit enabled).
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1666666191
<b>[SF] Trillian test result (tid-7287)</b>
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37829 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7810-t7287-xenserver-71.zip
Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1663941433
@blueorangutan test matrix keepEnv
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665912785
<b>[SF] Trillian test result (tid-7257)</b>
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38977 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7810-t7257-xenserver-71.zip
Smoke tests completed. 101 look OK, 7 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_05_stop_ssvm | `Error` | 941.73 | test_ssvm.py
test_07_reboot_ssvm | `Error` | 914.43 | test_ssvm.py
test_09_reboot_ssvm_forced | `Error` | 949.60 | test_ssvm.py
ContextSuite context=TestIsolatedNetworksPasswdServer>:setup | `Error` | 0.00 | test_password_server.py
ContextSuite context=TestUnmanageVM>:setup | `Error` | 0.00 | test_vm_lifecycle_unmanage_import.py
ContextSuite context=TestCreateVolume>:setup | `Error` | 0.00 | test_volumes.py
ContextSuite context=TestVPCRedundancy>:setup | `Error` | 0.00 | test_vpc_redundant.py
ContextSuite context=TestVPCNics>:setup | `Error` | 0.00 | test_vpc_router_nics.py
ContextSuite context=TestRVPCSite2SiteVpn>:setup | `Error` | 0.00 | test_vpc_vpn.py
ContextSuite context=TestVPCSite2SiteVPNMultipleOptions>:setup | `Error` | 0.00 | test_vpc_vpn.py
ContextSuite context=TestVpcRemoteAccessVpn>:setup | `Error` | 0.00 | test_vpc_vpn.py
ContextSuite context=TestVpcSite2SiteVpn>:setup | `Error` | 0.00 | test_vpc_vpn.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1284376590
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2800,11 +2795,17 @@ protected GuestResourceDef createGuestResourceDef(VirtualMachineTO vmTO){
}
protected long getCurrentMemAccordingToMemBallooning(VirtualMachineTO vmTO, long maxRam) {
- if (_noMemBalloon) {
- s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ if (_noMemBalloon || (vmTO != null && vmTO.getType() != VirtualMachine.Type.User)) {
+ if (_noMemBalloon) {
+ s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ } else {
+ s_logger.warn(String.format("Setting System VM's [%s] current memory as max memory [%s].", vmTO.toString(), maxRam));
+ }
return maxRam;
} else {
- return ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ long minRam = ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ s_logger.debug(String.format("Setting VM's [%s] current memory as min memory [%s] due to memory ballooning is enabled.", vmTO.toString(), minRam));
+ return minRam;
}
}
Review Comment:
ok, i'll be a dare devil (we can always revert)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665613267
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1666688086
<b>[SF] Trillian test result (tid-7288)</b>
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server r8
Total time taken: 43166 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7810-t7288-vmware-67u3.zip
Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland merged pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland merged PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1283122951
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java:
##########
@@ -214,11 +214,11 @@ public void testDiskDef() {
assertEquals(bus, disk.getBusType());
assertEquals(DiskDef.DeviceType.DISK, disk.getDeviceType());
- String resultingXml = disk.toString();
+ String xmlDef = disk.toString();
String expectedXml = "<disk device='disk' type='file'>\n<driver name='qemu' type='" + type.toString() + "' cache='" + cacheMode.toString() + "' />\n" +
"<source file='" + filePath + "'/>\n<target dev='" + diskLabel + "' bus='" + bus.toString() + "'/>\n</disk>\n";
- assertEquals(expectedXml, resultingXml);
+ assertEquals(xmlDef, expectedXml);
Review Comment:
```suggestion
assertEquals(expectedXml, xmlDef);
```
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java:
##########
@@ -356,29 +356,29 @@ public void testDiskDefWithBurst() {
"<read_bytes_sec_max_length>"+bytesReadRateMaxLength+"</read_bytes_sec_max_length>\n<write_bytes_sec_max_length>"+bytesWriteRateMaxLength+"</write_bytes_sec_max_length>\n" +
"<read_iops_sec_max_length>"+iopsReadRateMaxLength+"</read_iops_sec_max_length>\n<write_iops_sec_max_length>"+iopsWriteRateMaxLength+"</write_iops_sec_max_length>\n</iotune>\n</disk>\n";
- assertEquals(expectedXml, resultingXml);
+ assertEquals(xmlDef, expectedXml);
Review Comment:
```suggestion
assertEquals(expectedXml, xmlDef);
```
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java:
##########
@@ -356,29 +356,29 @@ public void testDiskDefWithBurst() {
"<read_bytes_sec_max_length>"+bytesReadRateMaxLength+"</read_bytes_sec_max_length>\n<write_bytes_sec_max_length>"+bytesWriteRateMaxLength+"</write_bytes_sec_max_length>\n" +
"<read_iops_sec_max_length>"+iopsReadRateMaxLength+"</read_iops_sec_max_length>\n<write_iops_sec_max_length>"+iopsWriteRateMaxLength+"</write_iops_sec_max_length>\n</iotune>\n</disk>\n";
- assertEquals(expectedXml, resultingXml);
+ assertEquals(xmlDef, expectedXml);
}
@Test
public void memBalloonDefTestNone() {
- String expectedXml = "<memballoon model='none' autodeflate='on'>\n</memballoon>";
+ String expectedXml = "<memballoon model='none'>\n</memballoon>";
MemBalloonDef memBalloonDef = new MemBalloonDef();
memBalloonDef.defNoneMemBalloon();
- String resultingXml = memBalloonDef.toString();
+ String xmlDef = memBalloonDef.toString();
- assertEquals(expectedXml, resultingXml);
+ assertEquals(xmlDef, expectedXml);
}
@Test
public void memBalloonDefTestVirtio() {
- String expectedXml = "<memballoon model='virtio' autodeflate='on'>\n<stats period='60'/>\n</memballoon>";
+ String expectedXml = "<memballoon model='virtio'>\n<stats period='60'/>\n</memballoon>";
MemBalloonDef memBalloonDef = new MemBalloonDef();
memBalloonDef.defVirtioMemBalloon("60");
- String resultingXml = memBalloonDef.toString();
+ String xmlDef = memBalloonDef.toString();
- assertEquals(expectedXml, resultingXml);
+ assertEquals(xmlDef, expectedXml);
Review Comment:
```suggestion
assertEquals(expectedXml, xmlDef);
```
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java:
##########
@@ -453,6 +453,6 @@ public void testSCSIDef() {
"<address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>\n" +
"<driver queues='4'/>\n" +
"</controller>\n";
- assertEquals(expected, str);
+ assertEquals(str, expected);
Review Comment:
```suggestion
assertEquals(expected, str);
```
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java:
##########
@@ -356,29 +356,29 @@ public void testDiskDefWithBurst() {
"<read_bytes_sec_max_length>"+bytesReadRateMaxLength+"</read_bytes_sec_max_length>\n<write_bytes_sec_max_length>"+bytesWriteRateMaxLength+"</write_bytes_sec_max_length>\n" +
"<read_iops_sec_max_length>"+iopsReadRateMaxLength+"</read_iops_sec_max_length>\n<write_iops_sec_max_length>"+iopsWriteRateMaxLength+"</write_iops_sec_max_length>\n</iotune>\n</disk>\n";
- assertEquals(expectedXml, resultingXml);
+ assertEquals(xmlDef, expectedXml);
}
@Test
public void memBalloonDefTestNone() {
- String expectedXml = "<memballoon model='none' autodeflate='on'>\n</memballoon>";
+ String expectedXml = "<memballoon model='none'>\n</memballoon>";
MemBalloonDef memBalloonDef = new MemBalloonDef();
memBalloonDef.defNoneMemBalloon();
- String resultingXml = memBalloonDef.toString();
+ String xmlDef = memBalloonDef.toString();
- assertEquals(expectedXml, resultingXml);
+ assertEquals(xmlDef, expectedXml);
Review Comment:
```suggestion
assertEquals(expectedXml, xmlDef);
```
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java:
##########
@@ -413,11 +413,11 @@ public void testRngDef() {
int bytes = 2048;
LibvirtVMDef.RngDef def = new LibvirtVMDef.RngDef(path, backendModel, bytes, period);
- assertEquals(path, def.getPath());
- assertEquals(backendModel, def.getRngBackendModel());
- assertEquals(LibvirtVMDef.RngDef.RngModel.VIRTIO, def.getRngModel());
- assertEquals(bytes, def.getRngRateBytes());
- assertEquals(period, def.getRngRatePeriod());
+ assertEquals(def.getPath(), path);
+ assertEquals(def.getRngBackendModel(), backendModel);
+ assertEquals(def.getRngModel(), LibvirtVMDef.RngDef.RngModel.VIRTIO);
+ assertEquals(def.getRngRateBytes(), bytes);
+ assertEquals(def.getRngRatePeriod(), period);
Review Comment:
```suggestion
assertEquals(path, def.getPath());
assertEquals(backendModel, def.getRngBackendModel());
assertEquals(LibvirtVMDef.RngDef.RngModel.VIRTIO, def.getRngModel());
assertEquals(bytes, def.getRngRateBytes());
assertEquals(period, def.getRngRatePeriod());
```
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java:
##########
@@ -441,8 +441,8 @@ public void testWatchDogDef() {
LibvirtVMDef.WatchDogDef.WatchDogAction action = LibvirtVMDef.WatchDogDef.WatchDogAction.RESET;
LibvirtVMDef.WatchDogDef def = new LibvirtVMDef.WatchDogDef(action, model);
- assertEquals(model, def.getModel());
- assertEquals(action, def.getAction());
+ assertEquals(def.getModel(), model);
+ assertEquals(def.getAction(), action);
Review Comment:
```suggestion
assertEquals(model, def.getModel());
assertEquals(action, def.getAction());
```
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java:
##########
@@ -236,7 +236,7 @@ public void testDiskDefWithEncryption() {
"<secret type='passphrase' uuid='" + passphraseUuid + "' />\n" +
"</encryption>\n" +
"</disk>\n";
- assertEquals(expectedXML, disk.toString());
+ assertEquals(disk.toString(), expectedXML);
Review Comment:
```suggestion
assertEquals(expectedXML, disk.toString());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1663941091
Okay @weizhouapache @DaanHoogland I've address code comments and revert the change of balloon behaviour; if memory ballooning feature works then I think it might just work as earlier we weren't passing the right max ram value;
but let's test
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1663938670
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 6648
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665614089
@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665892250
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1664778343
<b>[SF] Trillian test result (tid-7252)</b>
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38201 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7810-t7252-xenserver-71.zip
Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1284151087
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2800,11 +2795,17 @@ protected GuestResourceDef createGuestResourceDef(VirtualMachineTO vmTO){
}
protected long getCurrentMemAccordingToMemBallooning(VirtualMachineTO vmTO, long maxRam) {
- if (_noMemBalloon) {
- s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ if (_noMemBalloon || (vmTO != null && vmTO.getType() != VirtualMachine.Type.User)) {
Review Comment:
@DaanHoogland https://github.com/apache/cloudstack/issues/7430 is addressed here :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1284230855
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2800,11 +2795,17 @@ protected GuestResourceDef createGuestResourceDef(VirtualMachineTO vmTO){
}
protected long getCurrentMemAccordingToMemBallooning(VirtualMachineTO vmTO, long maxRam) {
- if (_noMemBalloon) {
- s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ if (_noMemBalloon || (vmTO != null && vmTO.getType() != VirtualMachine.Type.User)) {
+ if (_noMemBalloon) {
+ s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ } else {
+ s_logger.warn(String.format("Setting System VM's [%s] current memory as max memory [%s].", vmTO.toString(), maxRam));
+ }
return maxRam;
} else {
- return ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ long minRam = ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ s_logger.debug(String.format("Setting VM's [%s] current memory as min memory [%s] due to memory ballooning is enabled.", vmTO.toString(), minRam));
+ return minRam;
}
}
Review Comment:
this might break unit tests as-is; feel free to push changes :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665872641
<b>[SF] Trillian test result (tid-7258)</b>
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server r8
Total time taken: 36963 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7810-t7258-vmware-67u3.zip
Smoke tests completed. 99 look OK, 9 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_deploy_vm_on_specific_host | `Error` | 13.63 | test_vm_deployment_planner.py
test_02_deploy_vm_on_specific_cluster | `Error` | 6.42 | test_vm_deployment_planner.py
test_03_deploy_vm_on_specific_pod | `Error` | 4.41 | test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster | `Error` | 7.53 | test_vm_deployment_planner.py
test_05_deploy_vm_on_cluster_override_pod | `Error` | 3.35 | test_vm_deployment_planner.py
ContextSuite context=TestDeployVM>:setup | `Error` | 0.00 | test_vm_life_cycle.py
test_01_offline_migrate_VM_and_root_volume | `Error` | 10.69 | test_vm_life_cycle.py
test_02_offline_migrate_VM_with_two_data_disks | `Error` | 12.63 | test_vm_life_cycle.py
test_03_live_migrate_VM_with_two_data_disks | `Error` | 8.53 | test_vm_life_cycle.py
test_04_migrate_detached_volume | `Error` | 7.49 | test_vm_life_cycle.py
ContextSuite context=TestVAppsVM>:setup | `Error` | 2.71 | test_vm_life_cycle.py
ContextSuite context=TestVMLifeCycle>:setup | `Error` | 15.84 | test_vm_life_cycle.py
test_01_unmanage_vm_cycle | `Error` | 10.44 | test_vm_lifecycle_unmanage_import.py
ContextSuite context=TestUnmanageVM>:teardown | `Error` | 10.47 | test_vm_lifecycle_unmanage_import.py
test_change_service_offering_for_vm_with_snapshots | `Error` | 13.65 | test_vm_snapshots.py
ContextSuite context=TestVmSnapshot>:setup | `Error` | 35.05 | test_vm_snapshots.py
ContextSuite context=TestCreateVolume>:setup | `Error` | 0.00 | test_volumes.py
ContextSuite context=TestVolumes>:setup | `Error` | 0.22 | test_volumes.py
ContextSuite context=TestIpv6Vpc>:setup | `Error` | 0.00 | test_vpc_ipv6.py
ContextSuite context=TestVPCRedundancy>:setup | `Error` | 0.00 | test_vpc_redundant.py
ContextSuite context=TestVPCNics>:setup | `Error` | 0.00 | test_vpc_router_nics.py
ContextSuite context=TestRVPCSite2SiteVpn>:setup | `Error` | 0.00 | test_vpc_vpn.py
ContextSuite context=TestVPCSite2SiteVPNMultipleOptions>:setup | `Error` | 0.00 | test_vpc_vpn.py
ContextSuite context=TestVpcRemoteAccessVpn>:setup | `Error` | 0.00 | test_vpc_vpn.py
ContextSuite context=TestVpcSite2SiteVpn>:setup | `Error` | 0.00 | test_vpc_vpn.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1666404224
<b>[SF] Trillian test result (tid-7285)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 45691 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7810-t7285-kvm-centos7.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_migrate_VM_and_root_volume | `Error` | 85.85 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 59.47 | test_vm_life_cycle.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1666708668
<b>[SF] Trillian test result (tid-7289)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48809 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7810-t7289-kvm-centos7.zip
Smoke tests completed. 106 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_list_vms_metrics_admin | `Error` | 3607.61 | test_metrics_api.py
test_list_vms_metrics_history | `Error` | 5.42 | test_metrics_api.py
test_list_volumes_metrics_history | `Error` | 5.38 | test_metrics_api.py
test_01_migrate_VM_and_root_volume | `Error` | 76.77 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 53.67 | test_vm_life_cycle.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1666775756
> Thanks for testing @DaanHoogland I suppose that's true if the vm divide setting is enabled for user-vms; for systemvms the memory would be consistently the same (max memory).
Yes @rohityadavcloud , I can test that as well, but we already showed that SVMs start which is what is what matters. Errors in the kvm version seem unrelated, merging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1663942695
@DaanHoogland a [SF] Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] codecov[bot] commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1664035485
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/7810?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#7810](https://app.codecov.io/gh/apache/cloudstack/pull/7810?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (037bcbe) into [4.18](https://app.codecov.io/gh/apache/cloudstack/commit/c86684fbdd0f4461838f4b6d534bff7986b75c0d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c86684f) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## 4.18 #7810 +/- ##
============================================
- Coverage 13.02% 13.02% -0.01%
+ Complexity 9031 9028 -3
============================================
Files 2720 2720
Lines 257010 257007 -3
Branches 40083 40081 -2
============================================
- Hits 33472 33466 -6
- Misses 219336 219341 +5
+ Partials 4202 4200 -2
```
| [Files Changed](https://app.codecov.io/gh/apache/cloudstack/pull/7810?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ervisor/kvm/resource/LibvirtComputingResource.java](https://app.codecov.io/gh/apache/cloudstack/pull/7810?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9oeXBlcnZpc29ycy9rdm0vc3JjL21haW4vamF2YS9jb20vY2xvdWQvaHlwZXJ2aXNvci9rdm0vcmVzb3VyY2UvTGlidmlydENvbXB1dGluZ1Jlc291cmNlLmphdmE=) | `18.30% <100.00%> (-0.02%)` | :arrow_down: |
| [...om/cloud/hypervisor/kvm/resource/LibvirtVMDef.java](https://app.codecov.io/gh/apache/cloudstack/pull/7810?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9oeXBlcnZpc29ycy9rdm0vc3JjL21haW4vamF2YS9jb20vY2xvdWQvaHlwZXJ2aXNvci9rdm0vcmVzb3VyY2UvTGlidmlydFZNRGVmLmphdmE=) | `67.13% <100.00%> (ø)` | |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/cloudstack/pull/7810/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1664451429
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 6655
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] harikrishna-patnala commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "harikrishna-patnala (via GitHub)" <gi...@apache.org>.
harikrishna-patnala commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1284089612
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java:
##########
@@ -1238,7 +1238,7 @@ public String getMemBalloonStatsPeriod() {
@Override
public String toString() {
StringBuilder memBalloonBuilder = new StringBuilder();
- memBalloonBuilder.append("<memballoon model='" + memBalloonModel + "' autodeflate='on'>\n");
+ memBalloonBuilder.append("<memballoon model='" + memBalloonModel + "'>\n");
Review Comment:
@rohityadavcloud does this mean, the blocker issue https://github.com/apache/cloudstack/issues/7794 is fixed here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1284160928
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2800,11 +2795,17 @@ protected GuestResourceDef createGuestResourceDef(VirtualMachineTO vmTO){
}
protected long getCurrentMemAccordingToMemBallooning(VirtualMachineTO vmTO, long maxRam) {
- if (_noMemBalloon) {
- s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ if (_noMemBalloon || (vmTO != null && vmTO.getType() != VirtualMachine.Type.User)) {
+ if (_noMemBalloon) {
+ s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ } else {
+ s_logger.warn(String.format("Setting System VM's [%s] current memory as max memory [%s].", vmTO.toString(), maxRam));
+ }
return maxRam;
} else {
- return ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ long minRam = ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ s_logger.debug(String.format("Setting VM's [%s] current memory as min memory [%s] due to memory ballooning is enabled.", vmTO.toString(), minRam));
+ return minRam;
}
}
Review Comment:
for simplification and readability i'd make this
```suggestion
protected long getCurrentMemAccordingToMemBallooning(VirtualMachineTO vmTO, long maxRam) {
long retVal = maxRam;
if (_noMemBalloon) {
s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
} else if (vmTO != null && vmTO.getType() != VirtualMachine.Type.User) {
s_logger.warn(String.format("Setting System VM's [%s] current memory as max memory [%s].", vmTO.toString(), maxRam));
} else {
long minRam = ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
s_logger.debug(String.format("Setting VM's [%s] current memory as min memory [%s] due to memory ballooning is enabled.", vmTO.toString(), minRam));
retVal = minRam;
}
return retVal;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1284376590
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2800,11 +2795,17 @@ protected GuestResourceDef createGuestResourceDef(VirtualMachineTO vmTO){
}
protected long getCurrentMemAccordingToMemBallooning(VirtualMachineTO vmTO, long maxRam) {
- if (_noMemBalloon) {
- s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ if (_noMemBalloon || (vmTO != null && vmTO.getType() != VirtualMachine.Type.User)) {
+ if (_noMemBalloon) {
+ s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ } else {
+ s_logger.warn(String.format("Setting System VM's [%s] current memory as max memory [%s].", vmTO.toString(), maxRam));
+ }
return maxRam;
} else {
- return ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ long minRam = ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ s_logger.debug(String.format("Setting VM's [%s] current memory as min memory [%s] due to memory ballooning is enabled.", vmTO.toString(), minRam));
+ return minRam;
}
}
Review Comment:
ok, i'll be a dare devil (we can always revert)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665895465
@rohityadavcloud a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1666838127
Cheers thanks @DaanHoogland
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1666539858
Thanks for testing @DaanHoogland I suppose that's true if the vm divide setting is enabled for user-vms; for systemvms the memory would be consistently the same (max memory).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1663927907
@weizhouapache in that case let me grab Daan's workaround to not apply min memory for systemvms
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1664321157
Packaging result [SF]: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: el9 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 6654
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665249761
Thanks @DaanHoogland if you see my comment, we need not re-open the previous issue. Ask @weizhouapache to review and lgtm/advise;
This PR handles the following cases:
1. Doc PR addresses what to expect, what/how to configure KVM host env when overcommiting memory - https://github.com/apache/cloudstack-documentation/pull/337
2. For non-User VMs, we use max memory and in KVM guru the correct max memory is passed (fixes #7443)
3. Fixes #7794 as centos7/el7 smoketests passing now https://github.com/apache/cloudstack/pull/7810#issuecomment-1664869520
4. Reverts/ensures we don't disable memory balloon feature for KVM
I think this covers most of the cases and doc PR handles advising users the best practice they deem fit for what they're doing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1663857483
@rohityadavcloud a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1664869520
<b>[SF] Trillian test result (tid-7254)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 45466 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7810-t7254-kvm-centos7.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_migrate_VM_and_root_volume | `Error` | 79.72 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 53.40 | test_vm_life_cycle.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665187521
I think we should merge this and re-open #7430 / re-implement #7663 if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1664363696
@rohityadavcloud a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1666183454
<b>[SF] Trillian test result (tid-7269)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 45398 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7810-t7269-kvm-centos7.zip
Smoke tests completed. 104 look OK, 4 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
ContextSuite context=TestCpuCapServiceOfferings>:setup | `Error` | 0.00 | test_service_offerings.py
ContextSuite context=TestServiceOfferings>:setup | `Error` | 0.20 | test_service_offerings.py
ContextSuite context=TestRouterIpTablesPolicies>:setup | `Error` | 0.00 | test_routers_iptables_default_policy.py
ContextSuite context=TestVPCIpTablesPolicies>:setup | `Error` | 0.00 | test_routers_iptables_default_policy.py
ContextSuite context=TestVMWareStoragePolicies>:setup | `Error` | 0.00 | test_storage_policy.py
test_01_migrate_VM_and_root_volume | `Error` | 82.84 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 56.55 | test_vm_life_cycle.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1666520260
@DaanHoogland a [SF] Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1284151087
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2800,11 +2795,17 @@ protected GuestResourceDef createGuestResourceDef(VirtualMachineTO vmTO){
}
protected long getCurrentMemAccordingToMemBallooning(VirtualMachineTO vmTO, long maxRam) {
- if (_noMemBalloon) {
- s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ if (_noMemBalloon || (vmTO != null && vmTO.getType() != VirtualMachine.Type.User)) {
Review Comment:
@DaanHoogland https://github.com/apache/cloudstack/issues/7430 is addressed here :) so we need not re-open the bug
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665724929
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 6666
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665931446
<b>[SF] Trillian test result (tid-7259)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 39820 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7810-t7259-kvm-centos7.zip
Smoke tests completed. 102 look OK, 6 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_migrate_VM_and_root_volume | `Error` | 82.20 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 54.75 | test_vm_life_cycle.py
test_06_download_detached_volume | `Error` | 299.76 | test_volumes.py
test_13_migrate_volume_and_change_offering | `Error` | 121.89 | test_volumes.py
ContextSuite context=TestIpv6Vpc>:setup | `Error` | 0.00 | test_vpc_ipv6.py
ContextSuite context=TestVPCRedundancy>:setup | `Error` | 0.00 | test_vpc_redundant.py
ContextSuite context=TestVPCNics>:setup | `Error` | 0.00 | test_vpc_router_nics.py
ContextSuite context=TestRVPCSite2SiteVpn>:setup | `Error` | 0.00 | test_vpc_vpn.py
ContextSuite context=TestVPCSite2SiteVPNMultipleOptions>:setup | `Error` | 0.00 | test_vpc_vpn.py
ContextSuite context=TestVpcRemoteAccessVpn>:setup | `Error` | 0.00 | test_vpc_vpn.py
ContextSuite context=TestVpcSite2SiteVpn>:setup | `Error` | 0.00 | test_vpc_vpn.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1283166526
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2780,15 +2780,10 @@ protected GuestResourceDef createGuestResourceDef(VirtualMachineTO vmTO){
grd.setMemBalloning(!_noMemBalloon);
- long maxRam = ByteScaleUtils.bytesToKibibytes(vmTO.getMaxRam());
- long currRam = vmTO.getType() == VirtualMachine.Type.User ? getCurrentMemAccordingToMemBallooning(vmTO, maxRam) : maxRam;
-
- if (s_logger.isTraceEnabled()) {
- s_logger.trace(String.format("memory values for VM %s are %d/%d",vmTO.getName(),maxRam, currRam));
- }
+ Long maxRam = ByteScaleUtils.bytesToKibibytes(vmTO.getMaxRam());
grd.setMemorySize(maxRam);
- grd.setCurrentMem(currRam);
+ grd.setCurrentMem(getCurrentMemAccordingToMemBallooning(vmTO, maxRam));
Review Comment:
I just realised the maxRam seetting is fixed as in the KVMGuru the correct maxRam is passed now.
cc @DaanHoogland @weizhouapache let's test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1284378162
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2800,11 +2795,17 @@ protected GuestResourceDef createGuestResourceDef(VirtualMachineTO vmTO){
}
protected long getCurrentMemAccordingToMemBallooning(VirtualMachineTO vmTO, long maxRam) {
- if (_noMemBalloon) {
- s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ if (_noMemBalloon || (vmTO != null && vmTO.getType() != VirtualMachine.Type.User)) {
+ if (_noMemBalloon) {
+ s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ } else {
+ s_logger.warn(String.format("Setting System VM's [%s] current memory as max memory [%s].", vmTO.toString(), maxRam));
+ }
return maxRam;
} else {
- return ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ long minRam = ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ s_logger.debug(String.format("Setting VM's [%s] current memory as min memory [%s] due to memory ballooning is enabled.", vmTO.toString(), minRam));
+ return minRam;
}
}
Review Comment:
not accepted by github, will leave for prosterity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#discussion_r1284451055
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2800,11 +2795,17 @@ protected GuestResourceDef createGuestResourceDef(VirtualMachineTO vmTO){
}
protected long getCurrentMemAccordingToMemBallooning(VirtualMachineTO vmTO, long maxRam) {
- if (_noMemBalloon) {
- s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ if (_noMemBalloon || (vmTO != null && vmTO.getType() != VirtualMachine.Type.User)) {
+ if (_noMemBalloon) {
+ s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ } else {
+ s_logger.warn(String.format("Setting System VM's [%s] current memory as max memory [%s].", vmTO.toString(), maxRam));
+ }
return maxRam;
} else {
- return ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ long minRam = ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ s_logger.debug(String.format("Setting VM's [%s] current memory as min memory [%s] due to memory ballooning is enabled.", vmTO.toString(), minRam));
+ return minRam;
}
}
Review Comment:
ran it locally and pushed, couldn ´t leave it
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2800,11 +2795,17 @@ protected GuestResourceDef createGuestResourceDef(VirtualMachineTO vmTO){
}
protected long getCurrentMemAccordingToMemBallooning(VirtualMachineTO vmTO, long maxRam) {
- if (_noMemBalloon) {
- s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ if (_noMemBalloon || (vmTO != null && vmTO.getType() != VirtualMachine.Type.User)) {
+ if (_noMemBalloon) {
+ s_logger.warn(String.format("Setting VM's [%s] current memory as max memory [%s] due to memory ballooning is disabled. If you are using a custom service offering, verify if memory ballooning really should be disabled.", vmTO.toString(), maxRam));
+ } else {
+ s_logger.warn(String.format("Setting System VM's [%s] current memory as max memory [%s].", vmTO.toString(), maxRam));
+ }
return maxRam;
} else {
- return ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ long minRam = ByteScaleUtils.bytesToKibibytes(vmTO.getMinRam());
+ s_logger.debug(String.format("Setting VM's [%s] current memory as min memory [%s] due to memory ballooning is enabled.", vmTO.toString(), minRam));
+ return minRam;
}
}
Review Comment:
ran it locally and pushed, couldn ´t leave it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1666518930
@rohityadavcloud, #7430 is fixed in this PR. When memory overprovisioning is set to 3 on the cluster:
```
<maxMemory slots='16' unit='KiB'>524288</maxMemory>
<memory unit='KiB'>174080</memory>
<currentMemory unit='KiB'>174080</currentMemory>
```
when set to 5
```
<maxMemory slots='16' unit='KiB'>524288</maxMemory>
<memory unit='KiB'>104448</memory>
<currentMemory unit='KiB'>104448</currentMemory>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1666520011
@blueorangutan test matrix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1663855395
I think here's how we deal with three issues:
1. the centos7 blocker hopefully will be fixed if the revert committed caused that regression
2. by default it passes the VM max memory via KVM guru, so libvirt computing resource uses the max VM memory when VM ballooning is disabled; and in env where VM ballooning somehow works the setting is not disabled and VM start with min. memory
3. we change the default behaviour to have VM ballooning feature disable by default when the setting isn't defined. Existing env when they upgrade will continue to honour the setting in agent.properties; but users not reading docs will not get affected if they use commit memory in KVM env.
please review and advise - @DaanHoogland @weizhouapache @harikrishna-patnala
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665038468
@blueorangutan test matrix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665039729
@rohityadavcloud a [SF] Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665188783
@blueorangutan test keepEnv
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1664361039
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7810: Allow KVM overcommit to work without reducing minimum VM memory when vm ballooning is disabled
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7810:
URL: https://github.com/apache/cloudstack/pull/7810#issuecomment-1665167914
@DaanHoogland @weizhouapache I've raised a doc PR pl help review if along with the doc PR we cover all bases - https://github.com/apache/cloudstack-documentation/pull/337
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org