Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Adding memoryallocatedpercentage & memoryallocatedbytes to HostsResponse & HostsForMigrationResponse#4478

Merged
DaanHoogland merged 1 commit intoapache:4.14apache/cloudstack:4.14from
shapeblue:fix-host-respshapeblue/cloudstack:fix-host-respCopy head branch name to clipboard
Nov 20, 2020
Merged

Adding memoryallocatedpercentage & memoryallocatedbytes to HostsResponse & HostsForMigrationResponse#4478
DaanHoogland merged 1 commit intoapache:4.14apache/cloudstack:4.14from
shapeblue:fix-host-respshapeblue/cloudstack:fix-host-respCopy head branch name to clipboard

Conversation

@davidjumani
Copy link
Contributor

@davidjumani davidjumani commented Nov 18, 2020 •
edited
Loading

Description

Fixes #4460

Adds memoryallocatedpercentage & memoryallocatedbytes to HostsResponse as well as HostsForMigrationResponse and deprecates memoryallocated

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • 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)

How Has This Been Tested?

(localhost) 🐱 > list hosts filter=cpuallocated,memoryallocated,memoryallocatedbytes,memoryallocatedpercentage, type=Routing
{
  "count": 2,
  "host": [
    {
      "cpuallocated": "6.25%",
      "memoryallocated": 2415919104,
      "memoryallocatedbytes": 2415919104,
      "memoryallocatedpercentage": "28.12%"
    },
    {
      "cpuallocated": "1.56%",
      "memoryallocated": 536870912,
      "memoryallocatedbytes": 536870912,
      "memoryallocatedpercentage": "6.25%"
    }
  ]
}
(localhost) 🐱 > find hostsformigration virtualmachineid=e3c2c89a-26b3-427e-bc4a-34caeedf466a filter=cpuallocated,memoryallocated,memoryallocatedbytes,memoryallocatedpercentage, type=Routing
{
  "count": 2,
  "host": [
    {
      "cpuallocated": "1.56%",
      "memoryallocated": "6.25%",
      "memoryallocatedbytes": 536870912,
      "memoryallocatedpercentage": "6.25%"
    }
  ]
}

@davidjumani
Copy link
Contributor Author

davidjumani commented Nov 18, 2020

@blueorangutan package

@davidjumani davidjumani added this to the 4.14.1.0 milestone Nov 18, 2020
@davidjumani
Copy link
Contributor Author

davidjumani commented Nov 18, 2020

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Nov 18, 2020

@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@davidjumani
Copy link
Contributor Author

davidjumani commented Nov 18, 2020

@weizhouapache Is it fine that the memory is numeric but CPU is in percentage ? Or would it be better to have new memoryallocaedpercentage & cpuallocaedpercentage fields ?

@blueorangutan
Copy link

blueorangutan commented Nov 18, 2020

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2400

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Nov 18, 2020

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Nov 18, 2020

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@weizhouapache
Copy link
Contributor

weizhouapache commented Nov 18, 2020

@weizhouapache Is it fine that the memory is numeric but CPU is in percentage ? Or would it be better to have new memoryallocaedpercentage & cpuallocaedpercentage fields ?

@davidjumani it would be better. but it might break backwards compatibility (hope few users will be impacted).

@blueorangutan
Copy link

blueorangutan commented Nov 18, 2020

Trillian test result (tid-3198)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30749 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4478-t3198-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Smoke tests completed. 83 look OK, 0 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File

yadvr
yadvr reviewed Nov 18, 2020
View reviewed changes
server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java
hostResponse.setMemWithOverprovisioning(decimalFormat.format(memWithOverprovisioning));
hostResponse.setMemoryAllocated(decimalFormat.format((float) mem / memWithOverprovisioning * 100.0f) +"%");
hostResponse.setMemoryAllocated(mem);

Copy link
Member

@yadvr yadvr Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this cause regression?

Copy link
Contributor Author

@davidjumani davidjumani Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will but we can introduce two new fields memoryallocaedpercentage & cpuallocaedpercentage to compensate.
This will ensure that the response for both HostResponse and HostForMigrationResponse will be the same. Sounds good @rhtyd ?

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Nov 19, 2020

€0,02: we can add responce parameters that are alligned for both -precentages and -values and leave these alone alltogether. @weizhouapache @rhtyd @davidjumani ?

@weizhouapache
Copy link
Contributor

weizhouapache commented Nov 19, 2020

€0,02: we can add responce parameters that are alligned for both -precentages and -values and leave these alone alltogether. @weizhouapache @rhtyd @davidjumani ?

@DaanHoogland good to me. and mark the cpuallocated and memoryallocated as deprecated ?

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Nov 19, 2020

€0,02: we can add responce parameters that are alligned for both -precentages and -values and leave these alone alltogether. @weizhouapache @rhtyd @davidjumani ?

@DaanHoogland good to me. and mark the cpuallocated and memoryallocated as deprecated ?

yes

@davidjumani
Copy link
Contributor Author

davidjumani commented Nov 19, 2020

€0,02: we can add responce parameters that are alligned for both -precentages and -values and leave these alone alltogether. @weizhouapache @rhtyd @davidjumani ?

@DaanHoogland good to me. and mark the cpuallocated and memoryallocated as deprecated ?

yes

Sounds good! I'll make the changes

@davidjumani davidjumani changed the title hostformigrationresponse: Setting memory allocated to long Adding memoryallocatedpercentage & memoryallocatedbytes to HostsResponse & HostsForMigrationResponse Nov 20, 2020
@davidjumani
Copy link
Contributor Author

davidjumani commented Nov 20, 2020

@DaanHoogland @weizhouapache Added memoryallocatedpercentage & memoryallocatedbytes and deprecated memoryallocated

@DaanHoogland DaanHoogland mentioned this pull request Nov 20, 2020
Closed
@DaanHoogland
Copy link
Contributor

DaanHoogland commented Nov 20, 2020

yes 👍 don't bother for now but I'm creating a ticket do implement the same for cpu #4488

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Nov 20, 2020

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Nov 20, 2020

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

blueorangutan commented Nov 20, 2020

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2413

@DaanHoogland DaanHoogland mentioned this pull request Nov 20, 2020
Closed
@DaanHoogland
Copy link
Contributor

DaanHoogland commented Nov 20, 2020

I don't think we need smoke tests for this. restarted two of the travis runs

@DaanHoogland DaanHoogland merged commit d79d242 into apache:4.14 Nov 20, 2020
@DaanHoogland DaanHoogland deleted the fix-host-resp branch November 20, 2020 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr approved these changes

@DaanHoogland DaanHoogland DaanHoogland approved these changes

@weizhouapache weizhouapache Awaiting requested review from weizhouapache

Assignees

No one assigned

Labels

Severity:Minor type:bug

Projects

None yet

Milestone

4.14.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@davidjumani @blueorangutan @DaanHoogland @weizhouapache @yadvr
Morty Proxy This is a proxified and sanitized view of the page, visit original site.