Add support for disabling serivce / system and disk offerings #9546
Conversation
boring-cyborg
bot
added
component:api
component:marvin
component:UI
Python
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9546 +/- ##
===========================================
Coverage 15.81% 15.81%
- Complexity 12539 12554 +15
===========================================
Files 5623 5629 +6
Lines 491606 492041 +435
Branches 59582 61524 +1942
===========================================
+ Hits 77725 77817 +92
- Misses 405562 405901 +339
- Partials 8319 8323 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@blueorangutan package |
|
@Pearl1594 a [SL] 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10698 |
| return Active; | ||
| } | ||
| State state = EnumUtils.getEnumIgnoreCase(State.class, diskOfferingState); | ||
| if (!diskOfferingState.equalsIgnoreCase("all") && state == null) { |
There was a problem hiding this comment.
Are these checks not relevant anymore?
There was a problem hiding this comment.
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"
| return Active; | ||
| } | ||
| State state = EnumUtils.getEnumIgnoreCase(State.class, serviceOfferingState); | ||
| if (!serviceOfferingState.equalsIgnoreCase("all") && state == null) { |
There was a problem hiding this comment.
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"
| } else { | ||
| sc.setParameters("state", Arrays.asList(ServiceOffering.State.Active, ServiceOffering.State.Inactive)); |
There was a problem hiding this comment.
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.
If you do not set the state condition (AKA where state in x), both active and inactive will be listed.
|
I am not sure if should allow deletion of offerings. This could potentially cause some issues when the offering being deleted is still in use by some other resource (e.g. CKS, autoscaling group, etc.) which could then result in NPE or some other errors. |
…ort-disable-offerings
@vishesh92 Checks have been added to prevent deletion of offerings if there are any VMs or volumes using them |
|
@blueorangutan package |
|
@Pearl1594 a [SL] 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. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10715 |
|
@blueorangutan package |
|
@Pearl1594 a [SL] 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10716 |
|
|
||
| ServiceOfferingSearch = createSearchBuilder(); | ||
| ServiceOfferingSearch.and("serviceOfferingId", ServiceOfferingSearch.entity().getServiceOfferingId(), Op.EQ); | ||
| BackupSearch.done(); |
There was a problem hiding this comment.
Shouldn't it be ServiceOfferingSearch.done here?
| } else { | ||
| sc.setParameters("state", Arrays.asList(ServiceOffering.State.Active, ServiceOffering.State.Inactive)); |
There was a problem hiding this comment.
If you do not set the state condition (AKA where state in x), both active and inactive will be listed.
| @@ -3913,8 +3915,7 @@ private Pair<List<Long>, Integer> searchForServiceOfferingIdsAndCount(ListServic | |||
| } | |||
|
|
|||
| 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.
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.
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.
This method is too long (500+ lines...) for me to judge what it is exactly doing and where it is being done.
I just want to make sure that when creating VMs we don't list inactive offerings. If this change does not cause that, then I'm ok.
There was a problem hiding this comment.
Al, that's a fair point.. Probably reverting to it listing Active by default would be a better option. Thanks.
| } else { | ||
| sc.setParameters("state", Arrays.asList(ServiceOffering.State.Active, ServiceOffering.State.Inactive)); |
There was a problem hiding this comment.
Same here, if you do not set this parameter, both active and inactive offerings will be queried.
|
@blueorangutan package |
|
[SF] Trillian test result (tid-11263)
|
|
[SF] Trillian test result (tid-11262)
|
|
[SF] Trillian test result (tid-11264)
|
|
Seems like quite a few of the test failures weren't related to the changes of the PR |
|
@blueorangutan package |
|
@DaanHoogland a [SL] 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10905 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-11285)
|
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
Most test failures seems not related to this PR:
|
…ort-disable-offerings
|
@blueorangutan package |
|
@Pearl1594 a [SL] 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11086 |
|
@blueorangutan test |
|
@Pearl1594 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-11442)
|
| List<VolumeVO> volumesUsingOffering = _volumeDao.findByDiskOfferingId(diskOfferingId); | ||
| if (!volumesUsingOffering.isEmpty()) { | ||
| throw new InvalidParameterValueException(String.format("Unable to delete disk offering: %s [%s] because there are volumes using it", offering.getUuid(), offering.getName())); | ||
| } |
There was a problem hiding this comment.
How is this related to the main change? Why should we start blocking this now?
| List<VMInstanceVO> vmsUsingOffering = _vmInstanceDao.listByOfferingId(offeringId); | ||
| if (!vmsUsingOffering.isEmpty()) { | ||
| throw new CloudRuntimeException(String.format("Unable to delete service offering %s as it is in use", offering.getUuid())); | ||
| } |
There was a problem hiding this comment.
How is this related to the main change? Why should we start blocking this now?
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
Description
This PR addresses: #8440
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?