You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "DaanHoogland (via GitHub)" <gi...@apache.org> on 2024/06/10 05:36:00 UTC

[PR] add to cpu speed parameter a description of the cgroup2 case [cloudstack]

DaanHoogland opened a new pull request, #9191:
URL: https://github.com/apache/cloudstack/pull/9191

   ### Description
   
   This PR adds to the `cpuSpeed` parameter of `CreateServiceOfferingCmd` a description how it will be interpreted in the case cgroups are being used on KVM.
   
   Fixes: #6743
   
   <!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
   
   <!-- For new features, provide link to FS, dev ML discussion etc. -->
   <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
   
   <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
   <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
   
   <!--- ******************************************************************************* -->
   <!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
   <!--- PLEASE PUT AN 'X' in only **ONE** box -->
   <!--- ******************************************************************************* -->
   
   ### 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)
   - [x] Cleanup (Code refactoring and cleanup, that may add test cases)
   - [ ] build/CI
   
   ### Feature/Enhancement Scale or Bug Severity
   
   #### Feature/Enhancement Scale
   
   - [ ] Major
   - [x] Minor
   
   #### Bug Severity
   
   - [ ] BLOCKER
   - [ ] Critical
   - [ ] Major
   - [ ] Minor
   - [x] 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 -->
   
   #### How did you try to break this feature and the system with this change?
   
   <!-- 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

Re: [PR] add to cpu speed parameter a description of the cgroup2 case [cloudstack]

Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #9191:
URL: https://github.com/apache/cloudstack/pull/9191#discussion_r1634367237


##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:
##########
@@ -56,7 +56,7 @@ public class CreateServiceOfferingCmd extends BaseCmd {
     @Parameter(name = ApiConstants.CPU_NUMBER, type = CommandType.INTEGER, required = false, description = "the CPU number of the service offering")
     private Integer cpuNumber;
 
-    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz.")
+    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz. In the case cgroups are used on KVM, this is interpreted to calculate the maximum number of shares.")

Review Comment:
   How about this @weizhouapache : 
   ```suggestion
       @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz. In the case cgroups are used on KVM, this is interpreted to calculate the number of shares. see also https://libvirt.org/formatdomain.html#cpu-tuning")
   ```



-- 
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

Re: [PR] add to cpu speed parameter a description of the cgroup2 case [cloudstack]

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #9191:
URL: https://github.com/apache/cloudstack/pull/9191#discussion_r1634379120


##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:
##########
@@ -56,7 +56,7 @@ public class CreateServiceOfferingCmd extends BaseCmd {
     @Parameter(name = ApiConstants.CPU_NUMBER, type = CommandType.INTEGER, required = false, description = "the CPU number of the service offering")
     private Integer cpuNumber;
 
-    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz.")
+    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz. In the case cgroups are used on KVM, this is interpreted to calculate the maximum number of shares.")

Review Comment:
   that's fine !
   @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

Re: [PR] add to cpu speed parameter a description of the cgroup2 case [cloudstack]

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #9191:
URL: https://github.com/apache/cloudstack/pull/9191#discussion_r1633138413


##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:
##########
@@ -56,7 +56,7 @@ public class CreateServiceOfferingCmd extends BaseCmd {
     @Parameter(name = ApiConstants.CPU_NUMBER, type = CommandType.INTEGER, required = false, description = "the CPU number of the service offering")
     private Integer cpuNumber;
 
-    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz.")
+    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz. In the case cgroups are used on KVM, this is interpreted to calculate the maximum number of shares.")

Review Comment:
   according to https://libvirt.org/formatdomain.html#cpu-tuning
   
   `
   The optional shares element specifies the proportional weighted share for the domain. If this is omitted, it defaults to the OS provided defaults. NB, There is no unit for the value, it's a relative measure based on the setting of other VM, e.g. A VM configured with value 2048 will get twice as much CPU time as a VM configured with value 1024. The value should be in range [2, 262144] using cgroups v1, [1, 10000] using cgroups v2. Since 0.9.0
   `
   I am not sure if "the maximum number of shares" is correct.
   maybe we could also add the link above in the description



-- 
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

Re: [PR] add to cpu speed parameter a description of the cgroup2 case [cloudstack]

Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #9191:
URL: https://github.com/apache/cloudstack/pull/9191#discussion_r1637586046


##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:
##########
@@ -56,7 +56,7 @@ public class CreateServiceOfferingCmd extends BaseCmd {
     @Parameter(name = ApiConstants.CPU_NUMBER, type = CommandType.INTEGER, required = false, description = "the CPU number of the service offering")
     private Integer cpuNumber;
 
-    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz.")
+    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz. In the case cgroups are used on KVM, this is interpreted to calculate the number of shares. see also https://libvirt.org/formatdomain.html#cpu-tuning")

Review Comment:
   I updated your suggestion @BryanMLima  (and I was really surprised that I was allowed to???)
   
   I added the more explicit mention of VMware and Xen. Hope you don't mind.



-- 
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

Re: [PR] add to cpu speed parameter a description of the cgroup2 case [cloudstack]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #9191:
URL: https://github.com/apache/cloudstack/pull/9191#issuecomment-2157314778

   ## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/9191?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   All modified and coverable lines are covered by tests :white_check_mark:
   > Project coverage is 4.30%. Comparing base [(`c779b1c`)](https://app.codecov.io/gh/apache/cloudstack/commit/c779b1c616471b8fea833c4b090a7f3bc2dc6ab7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`ea6f75c`)](https://app.codecov.io/gh/apache/cloudstack/commit/ea6f75cdcb64707ffb5feb6e662945a62722a203?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   
   
   <details><summary>Additional details and impacted files</summary>
   
   
   ```diff
   @@             Coverage Diff              @@
   ##               4.19   #9191       +/-   ##
   ============================================
   - Coverage     14.96%   4.30%   -10.67%     
   ============================================
     Files          5373     363     -5010     
     Lines        469248   29302   -439946     
     Branches      58967    5116    -53851     
   ============================================
   - Hits          70220    1261    -68959     
   + Misses       391253   27898   -363355     
   + Partials       7775     143     -7632     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/9191/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/9191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.30% <ø> (ø)` | |
   | [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/9191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/9191?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?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

Re: [PR] add to cpu speed parameter a description of the cgroup2 case [cloudstack]

Posted by "BryanMLima (via GitHub)" <gi...@apache.org>.
BryanMLima commented on code in PR #9191:
URL: https://github.com/apache/cloudstack/pull/9191#discussion_r1637011378


##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:
##########
@@ -56,7 +56,7 @@ public class CreateServiceOfferingCmd extends BaseCmd {
     @Parameter(name = ApiConstants.CPU_NUMBER, type = CommandType.INTEGER, required = false, description = "the CPU number of the service offering")
     private Integer cpuNumber;
 
-    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz.")
+    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz. In the case cgroups are used on KVM, this is interpreted to calculate the number of shares. see also https://libvirt.org/formatdomain.html#cpu-tuning")

Review Comment:
   @DaanHoogland, I think it would benefit the users if we can add more detail on this parameter description. Something like below, what do you think?
   ```suggestion
       @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz. For the KVM hypervisor," +
               " the values of the parameters cpuSpeed and cpuNumber will be used to calculate the `shares` value. This value is used by the KVM hypervisor to calculate how much time" +
               " the VM will have access to the host's CPU. The `shares` value does not have a unit, and its purpose is being a weight value for the host to compare between its guest" +
               " VMs. For more information, see https://libvirt.org/formatdomain.html#cpu-tuning.")
   ```



-- 
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

Re: [PR] add to cpu speed parameter a description of the cgroup2 case [cloudstack]

Posted by "BryanMLima (via GitHub)" <gi...@apache.org>.
BryanMLima commented on code in PR #9191:
URL: https://github.com/apache/cloudstack/pull/9191#discussion_r1637011378


##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:
##########
@@ -56,7 +56,7 @@ public class CreateServiceOfferingCmd extends BaseCmd {
     @Parameter(name = ApiConstants.CPU_NUMBER, type = CommandType.INTEGER, required = false, description = "the CPU number of the service offering")
     private Integer cpuNumber;
 
-    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz.")
+    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz. In the case cgroups are used on KVM, this is interpreted to calculate the number of shares. see also https://libvirt.org/formatdomain.html#cpu-tuning")

Review Comment:
   @DaanHoogland, I think it would benefit the users if we can add more detail on this parameter description. Something like below, what do you think?
   ```suggestion
       @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "For VMware and Xen based hypervisors this is the CPU speed of the service offering in MHz.\n" +
               "For the KVM hypervisor," +
               " the values of the parameters cpuSpeed and cpuNumber will be used to calculate the `shares` value. This value is used by the KVM hypervisor to calculate how much time" +
               " the VM will have access to the host's CPU. The `shares` value does not have a unit, and its purpose is being a weight value for the host to compare between its guest" +
               " VMs. For more information, see https://libvirt.org/formatdomain.html#cpu-tuning.")
   ```



-- 
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

Re: [PR] add to cpu speed parameter a description of the cgroup2 case [cloudstack]

Posted by "BryanMLima (via GitHub)" <gi...@apache.org>.
BryanMLima commented on code in PR #9191:
URL: https://github.com/apache/cloudstack/pull/9191#discussion_r1637011378


##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:
##########
@@ -56,7 +56,7 @@ public class CreateServiceOfferingCmd extends BaseCmd {
     @Parameter(name = ApiConstants.CPU_NUMBER, type = CommandType.INTEGER, required = false, description = "the CPU number of the service offering")
     private Integer cpuNumber;
 
-    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz.")
+    @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz. In the case cgroups are used on KVM, this is interpreted to calculate the number of shares. see also https://libvirt.org/formatdomain.html#cpu-tuning")

Review Comment:
   @DaanHoogland, I think it would benefit the users if we can add more detail on this parameter description. Something like below, what do you think?
   ```suggestion
       @Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER, required = false, description = "the CPU speed of the service offering in MHz. For the KVM hypervisor," +
               " the values of the parameters cpuSpeed and cpuNumber will be used to calculate the `shares` value. This value is used by the KVM hypervisor to calculate how much time" +
               " the VM will have access to the host's CPU. The `shares` value does not have a unit, and its purpose is be a weight value for the host to compare between its guest" +
               " VMs. For more information, see https://libvirt.org/formatdomain.html#cpu-tuning.")
   ```



-- 
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

Re: [PR] add to cpu speed parameter a description of the cgroup2 case [cloudstack]

Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud merged PR #9191:
URL: https://github.com/apache/cloudstack/pull/9191


-- 
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

Re: [PR] add to cpu speed parameter a description of the cgroup2 case [cloudstack]

Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #9191:
URL: https://github.com/apache/cloudstack/pull/9191#issuecomment-2160735274

   I don't think this needs a lot of testing but a second review is very welcome, @BryanMLima !


-- 
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