-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for disabling serivce / system and disk offerings #9546
base: main
Are you sure you want to change the base?
Changes from 3 commits
6932fb0
cfe4c6a
ea1c41d
6601b38
6ed1b60
fdb1f45
3785bab
bd7ff55
9040055
d34a916
1f3d2c9
29eea23
bb8a660
31f5c9b
a9f1b0a
590dc48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,6 @@ | |
// under the License. | ||
package org.apache.cloudstack.api.command.user.offering; | ||
|
||
import static com.cloud.offering.ServiceOffering.State.Active; | ||
|
||
import org.apache.cloudstack.api.APICommand; | ||
import org.apache.cloudstack.api.ApiConstants; | ||
import org.apache.cloudstack.api.BaseListProjectAndAccountResourcesCmd; | ||
|
@@ -28,7 +26,6 @@ | |
import org.apache.cloudstack.api.response.UserVmResponse; | ||
import org.apache.cloudstack.api.response.ZoneResponse; | ||
import org.apache.commons.lang3.EnumUtils; | ||
import org.apache.commons.lang3.StringUtils; | ||
|
||
import com.cloud.offering.ServiceOffering.State; | ||
|
||
|
@@ -157,14 +154,7 @@ | |
} | ||
|
||
public State getState() { | ||
if (StringUtils.isBlank(serviceOfferingState)) { | ||
return Active; | ||
} | ||
State state = EnumUtils.getEnumIgnoreCase(State.class, serviceOfferingState); | ||
if (!serviceOfferingState.equalsIgnoreCase("all") && state == null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just made this uniform to how it works with listing network and vpc offerings ; wherein when the state isn't passed, we list all offerings - so we do not explicitly need "all" |
||
throw new IllegalArgumentException("Invalid state value: " + serviceOfferingState); | ||
} | ||
return state; | ||
return EnumUtils.getEnumIgnoreCase(State.class, serviceOfferingState); | ||
Check warning on line 157 in api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java Codecov / codecov/patchapi/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java#L157
|
||
} | ||
|
||
public Long getTemplateId() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,6 +99,7 @@ | |
protected SearchBuilder<VMInstanceVO> StartingWithNoHostSearch; | ||
protected SearchBuilder<VMInstanceVO> NotMigratingSearch; | ||
protected SearchBuilder<VMInstanceVO> BackupSearch; | ||
protected SearchBuilder<VMInstanceVO> ServiceOfferingSearch; | ||
protected SearchBuilder<VMInstanceVO> LastHostAndStatesSearch; | ||
protected SearchBuilder<VMInstanceVO> VmsNotInClusterUsingPool; | ||
|
||
|
@@ -325,6 +326,10 @@ | |
VmsNotInClusterUsingPool.join("hostSearch2", hostSearch2, hostSearch2.entity().getId(), VmsNotInClusterUsingPool.entity().getHostId(), JoinType.INNER); | ||
VmsNotInClusterUsingPool.and("vmStates", VmsNotInClusterUsingPool.entity().getState(), Op.IN); | ||
VmsNotInClusterUsingPool.done(); | ||
|
||
ServiceOfferingSearch = createSearchBuilder(); | ||
ServiceOfferingSearch.and("serviceOfferingId", ServiceOfferingSearch.entity().getServiceOfferingId(), Op.EQ); | ||
BackupSearch.done(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't it be ServiceOfferingSearch.done here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right |
||
} | ||
|
||
@Override | ||
|
@@ -1069,4 +1074,11 @@ | |
List<VMInstanceVO> uniqueVms = vms.stream().distinct().collect(Collectors.toList()); | ||
return new Pair<>(uniqueVms, uniqueVms.size()); | ||
} | ||
|
||
@Override | ||
public List<VMInstanceVO> listByOfferingId(long offeringId) { | ||
SearchCriteria<VMInstanceVO> sc = ServiceOfferingSearch.create(); | ||
sc.setParameters("serviceOfferingId", offeringId); | ||
return search(sc, null); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3430,7 +3430,7 @@ | |
diskOfferingSearch.and("computeOnly", diskOfferingSearch.entity().isComputeOnly(), Op.EQ); | ||
|
||
if (state != null) { | ||
diskOfferingSearch.and("state", diskOfferingSearch.entity().getState(), Op.EQ); | ||
diskOfferingSearch.and("state", diskOfferingSearch.entity().getState(), Op.IN); | ||
} | ||
|
||
// Keeping this logic consistent with domain specific zones | ||
|
@@ -3547,6 +3547,8 @@ | |
|
||
if (state != null) { | ||
sc.setParameters("state", state); | ||
} else { | ||
sc.setParameters("state", Arrays.asList(ServiceOffering.State.Active, ServiceOffering.State.Inactive)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in case we don't specify what offering is required , this would list offerings in both active and inactive states There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you do not set the state condition (AKA |
||
} | ||
|
||
if (keyword != null) { | ||
|
@@ -3756,7 +3758,7 @@ | |
serviceOfferingSearch.select(null, Func.DISTINCT, serviceOfferingSearch.entity().getId()); // select distinct | ||
|
||
if (state != null) { | ||
serviceOfferingSearch.and("state", serviceOfferingSearch.entity().getState(), Op.EQ); | ||
serviceOfferingSearch.and("state", serviceOfferingSearch.entity().getState(), Op.IN); | ||
} | ||
|
||
if (vmId != null) { | ||
|
@@ -3913,8 +3915,7 @@ | |
} | ||
|
||
serviceOfferingSearch.join("diskOfferingSearch", diskOfferingSearch, JoinBuilder.JoinType.INNER, JoinBuilder.JoinCondition.AND, | ||
serviceOfferingSearch.entity().getDiskOfferingId(), diskOfferingSearch.entity().getId(), | ||
serviceOfferingSearch.entity().setString("Active"), diskOfferingSearch.entity().getState()); | ||
serviceOfferingSearch.entity().getDiskOfferingId(), diskOfferingSearch.entity().getId()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this a query that is executed if vmid != null? if so, should we list inactive offerings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this PR aims to support disabling service offerings - to make it similar to the way network offerings work, it would make sense to list active and inactive offerings There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method is too long (500+ lines...) for me to judge what it is exactly doing and where it is being done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Al, that's a fair point.. Probably reverting to it listing Active by default would be a better option. Thanks. |
||
} | ||
|
||
if (cpuNumber != null) { | ||
|
@@ -4043,6 +4044,8 @@ | |
SearchCriteria<ServiceOfferingVO> sc = serviceOfferingSearch.create(); | ||
if (state != null) { | ||
sc.setParameters("state", state); | ||
} else { | ||
sc.setParameters("state", Arrays.asList(ServiceOffering.State.Active, ServiceOffering.State.Inactive)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, if you do not set this parameter, both active and inactive offerings will be queried. |
||
} | ||
|
||
if (vmId != null) { | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -230,7 +230,6 @@ def test_02_deploy_vm_account_limit_reached(self): | |||
self.testdata["service_offering_multiple_cores"] | ||||
) | ||||
# Adding to cleanup list after execution | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
self.cleanup.append(self.service_offering) | ||||
|
||||
self.debug("Setting up account and domain hierarchy") | ||||
self.setupAccounts(account_limit=6, domain_limit=8) | ||||
|
@@ -242,8 +241,10 @@ def test_02_deploy_vm_account_limit_reached(self): | |||
self.debug("Deploying instance with account: %s" % | ||||
self.child_do_admin.name) | ||||
|
||||
self.createInstance(account=self.child_do_admin, | ||||
self.vm1 = self.createInstance(account=self.child_do_admin, | ||||
service_off=self.service_offering, api_client=api_client_admin) | ||||
self.cleanup.append(self.vm1) | ||||
self.cleanup.append(self.service_offering) | ||||
DaanHoogland marked this conversation as resolved.
Show resolved
Hide resolved
DaanHoogland marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
|
||||
self.debug("Deploying instance when CPU limit is reached in account") | ||||
|
||||
|
@@ -305,8 +306,6 @@ def test_04_deployVm__account_limit_reached(self): | |||
self.apiclient, | ||||
self.testdata["service_offering_multiple_cores"] | ||||
) | ||||
# Adding to cleanup list after execution | ||||
self.cleanup.append(self.service_offering) | ||||
|
||||
self.debug("Setting up account and domain hierarchy") | ||||
self.setupAccounts(account_limit=5, domain_limit=5, project_limit=5) | ||||
|
@@ -317,9 +316,11 @@ def test_04_deployVm__account_limit_reached(self): | |||
|
||||
self.debug("Deploying instance with account: %s" % | ||||
self.child_do_admin.name) | ||||
self.createInstance(account=self.child_do_admin, | ||||
self.vm2 = self.createInstance(account=self.child_do_admin, | ||||
service_off=self.service_offering, api_client=api_client_admin) | ||||
|
||||
# Adding to cleanup list after execution | ||||
self.cleanup.append(self.vm2) | ||||
self.cleanup.append(self.service_offering) | ||||
DaanHoogland marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
self.debug("Deploying instance in project when CPU limit is reached in account") | ||||
|
||||
with self.assertRaises(Exception): | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these checks not relevant anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just made this uniform to how it works with listing network and vpc offerings ; wherein when the state isn't passed, we list all offerings - so we do not explicitly need "all"