server: Fix some cpuspeed issues while create service offering by ustcweizhou · Pull Request #4376 · apache/cloudstack

ustcweizhou · 2020-10-06T07:23:24Z

Description

Only the following offerings are accepted

(1) fixed offering:
cpunumber, memory, must be specified.
cpuspeed should be specified

example: create serviceoffering displaytext=offering-fix name=offering-fix cpunumber=1 memory=1024 cpuspeed=1000

(should not work)
create serviceoffering displaytext=offering-fix name=offering-fix cpunumber=1 memory=1024 (but works in 4.14, fixed by this commit)

(2) unconstrained offering:

cpunumber, memory must be null
cpuspeed must be null

example: create serviceoffering displaytext=offering-unconstrained name=offering-unconstrained

(should not work)
create serviceoffering displaytext=offering-unconstrained name=offering-unconstrained cpunumber=1
create serviceoffering displaytext=offering-unconstrained name=offering-unconstrained memory=1024
create serviceoffering displaytext=offering-unconstrained name=offering-unconstrained cpuspeed=1000 (but works in 4.14, fixed by this commit)

(3) constrained offering:

cpunumber, memory must be null
mincpunumber, maxcpunumber, minmemory, maxmemory, cpuspeed must be specified

example:create serviceoffering displaytext=offering-constrained name=offering-constrained mincpunumber=1 maxcpunumber=2 minmemory=1024 maxmemory=2048 cpuspeed=1000

(should not work)
create serviceoffering displaytext=offering-constrained name=offering-constrained mincpunumber=1 maxcpunumber=2 minmemory=1024 maxmemory=2048 (but works in 4.14, fixed by this commit)

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)

Screenshots (if appropriate):

How Has This Been Tested?

Only the following offerings are accepted (1) fixed offering: cpunumber, memory, must be specified. cpuspeed should be specified example: create serviceoffering displaytext=offering-fix name=offering-fix cpunumber=1 memory=1024 cpuspeed=1000 (should not work) create serviceoffering displaytext=offering-fix name=offering-fix cpunumber=1 memory=1024 (but works in 4.14, fixed by this commit) (2) unconstrained offering: cpunumber, memory must be null cpuspeed must be null example: create serviceoffering displaytext=offering-unconstrained name=offering-unconstrained (should not work) create serviceoffering displaytext=offering-unconstrained name=offering-unconstrained cpunumber=1 create serviceoffering displaytext=offering-unconstrained name=offering-unconstrained memory=1024 create serviceoffering displaytext=offering-unconstrained name=offering-unconstrained cpuspeed=1000 (but works in 4.14, fixed by this commit) (3) constrained offering: cpunumber, memory must be null mincpunumber, maxcpunumber, minmemory, maxmemory, cpuspeed must be specified example:create serviceoffering displaytext=offering-constrained name=offering-constrained mincpunumber=1 maxcpunumber=2 minmemory=1024 maxmemory=2048 cpuspeed=1000 (should not work) create serviceoffering displaytext=offering-constrained name=offering-constrained mincpunumber=1 maxcpunumber=2 minmemory=1024 maxmemory=2048 (but works in 4.14, fixed by this commit)

DaanHoogland

code lgtm

DaanHoogland · 2020-10-06T11:15:19Z

@blueorangutan package

blueorangutan · 2020-10-06T11:15:54Z

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

blueorangutan · 2020-10-06T12:33:25Z

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

DaanHoogland · 2020-10-06T12:48:28Z

@blueorangutan test

blueorangutan · 2020-10-06T12:49:00Z

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

blueorangutan · 2020-10-06T21:51:13Z

Trillian test result (tid-2899)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30521 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4376-t2899-kvm-centos7.zip
Smoke tests completed. 83 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

shwstppr

LGTM

sureshanaparti · 2020-10-09T07:21:46Z

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

+                    throw new InvalidParameterValueException("For creating a custom compute offering min/max cpu and min/max memory/cpu speed should all be null or all specified");
                }
            } else {
                if (cpuSpeed != null && (cpuSpeed.intValue() < 0 || cpuSpeed.longValue() > Integer.MAX_VALUE)) {


cpuSpeed null check not required here, it is always not null as per the above changes.

@sureshanaparti indeed, I will remove it.

sureshanaparti · 2020-10-09T07:28:51Z

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

                throw new InvalidParameterValueException("Failed to create service offering " + offeringName + ": specify the cpu number value between 1 and " + Integer.MAX_VALUE);
            }
-            if (cpuSpeed != null && (cpuSpeed.intValue() < 0 || cpuSpeed.longValue() > Integer.MAX_VALUE)) {
+            if (cpuSpeed == null || (cpuSpeed.intValue() < 0 || cpuSpeed.longValue() > Integer.MAX_VALUE)) {


@ustcweizhou similar cond check applicable for cpuNumber and memory if these are mandatory params? check and update accordingly.

@sureshanaparti cpuNumber and memory are not mandatory for dynamic offerings.
There are 3 types of offerings
(1) fixed offering, cpu number, cpu speed, memory are mandatory
(2) dynamic offering,
(2.1) unconstrained offering, cpu number, cpu speed, memory are not required
(2.2) constrained offerings, min/max cpu, min/max memory, and cpu speed are mandatory.

@weizhouapache thanks for confirming

DaanHoogland approved these changes Oct 6, 2020

View reviewed changes

DaanHoogland added this to the 4.14.1.0 milestone Oct 6, 2020

shwstppr approved these changes Oct 9, 2020

View reviewed changes

sureshanaparti reviewed Oct 9, 2020

View reviewed changes

apache#4376 Remove unnecessary null check on cpuSpeed

5f2f58c

sureshanaparti approved these changes Oct 12, 2020

View reviewed changes

DaanHoogland merged commit 94f9f89 into apache:4.14 Oct 13, 2020

DaanHoogland added the type:bug label Oct 21, 2020

DaanHoogland added the Severity:Minor label Nov 10, 2020

Conversation

ustcweizhou commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

DaanHoogland commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

weizhouapache Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

weizhouapache Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ustcweizhou commented Oct 6, 2020 •

edited

Loading