[Hanarion]

Description

When using Cloudstack, when creating instance with a lot of offerings, it could be hard to differentiate between all of the offerings. For that, categories could be useful.

This pull request introduces the concept of "Service Offering Categories" to the API, allowing service offerings to be grouped, managed, and queried by category. It adds a new interface for categories, updates the API to support creating, updating, deleting, and listing categories, and enables associating service offerings with a category. The changes also extend existing API commands and responses to work with categories.

Service Offering Category API Support:

• Added new API commands for service offering categories: CreateServiceOfferingCategoryCmd, DeleteServiceOfferingCategoryCmd, UpdateServiceOfferingCategoryCmd, and ListServiceOfferingCategoriesCmd, enabling full CRUD operations and listing for categories.
• Updated ConfigurationService interface to include methods for creating, deleting, and updating service offering categories.
• Added a new interface ServiceOfferingCategory that defines category properties and behaviors.

API Parameter and Response Enhancements:

• Introduced new API parameters and constants for category ID (SERVICE_OFFERING_CATEGORY_ID) in ApiConstants, and updated related commands (CreateServiceOfferingCmd, UpdateServiceOfferingCmd, ListServiceOfferingsCmd) to accept or filter by category. The category name is returned in responses under the category key.
• Extended ResponseGenerator to support generating responses for service offering categories.

Service Offering Model Update:

• Added a getCategoryId() method to the ServiceOffering interface, allowing offerings to be associated with a specific category.

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)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

On my dev environment, through the API and cloudmonkey

How did you try to break this feature and the system with this change?

Those changes should not break any features as it is only adding a new column to serviceoffering and adding a new table, it is only a way to filter and categorize.

--
I'm sorry if the formatting isn't perfect, i couldn't get the pre-commit to work, and it is my first code PR.
Warning, i put the SQL where i thought it made sense, but i think you will want to move it where it really should be.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 3.52%. Comparing base (d0a9cbe) to head (c0f70c2).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##            main   #12144      +/-   ##
=========================================
- Coverage   3.53%    3.52%   -0.01%     
=========================================
  Files        487      487              
  Lines      41867    41921      +54     
  Branches    7913     7922       +9     
=========================================
  Hits        1479     1479              
- Misses     40173    40227      +54     
  Partials     215      215              

Flag	Coverage Δ
uitests	3.52% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[daviftorres]

Dear @Hanarion,

This is a nice feature to have. Are you considering extending it to allow a domain or account to be limited to one (or a specific list) of categories?

For example, we distinguish between Core (higher-performance hardware) and Essentials (lower-performance hardware), in a tiered fashion (with different costs, of course). With "Offering Category", we could define which groups of offerings a client (domain/account) is allowed to use.

What do you think?

[Hanarion]

@daviftorres it could be interesting yes.
An association between accounts, domains and the categories ? And if none are found, we show everything ?
But that would mean that we would need to add a check for "permission" to use the specific service offering category when creating a VM if we want to do things right.
What do you think ?

[daviftorres]

@daviftorres it could be interesting yes. An association between accounts, domains and the categories ? And if none are found, we show everything ? But that would mean that we would need to add a check for "permission" to use the specific service offering category when creating a VM if we want to do things right. What do you think ?

Sounds great!

[rajujith]

@blueorangutan package

[blueorangutan]

@rajujith 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.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15873

[rajujith]

@Hanarion I added service offering to a new category but it gets added the default one although the API has the correct one.

[Hanarion]

@rajujith thanks for the review.
Can you give me a bit more detail about that ? Is it showing the default one in the list UI, detail or edit ?
Or do you mean that the return object isn't updated with the new category on the edit service offering call ?

I'll check today in order to fix this issue

[rajujith]

@Hanarion

Screen.Recording.2025-12-03.at.10.03.17.AM.mov

[Copilot]

🟡 Changes recommended

It introduces several correctness issues (API version annotations, duplicated UI config keys overriding behavior, and robustness/permission handling gaps) that should be fixed before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

Previously missed (3) — in code that hasn't changed since the last review.

ui/src/config/section/offering.js:46

• The object defines filters twice; the later filters: ['active', 'inactive'] overrides the role-based filters() function, so non-admin users will incorrectly see the admin filters (and any future changes to filters() will be ignored). Remove the duplicate filters property and keep a single definition.
  ui/src/views/compute/DeployVM.vue:2342
• This always calls listServiceOfferingCategories during VM deploy. For roles that don’t have the API, this will consistently log an error and show a meaningless single "All" option. Guard the call with an API-availability check and initialize the selected category to -1 when categories are loaded.
  ui/src/views/compute/DeployVnfAppliance.vue:2714
• This always calls listServiceOfferingCategories on load; if the current role doesn’t have that API, it will always hit the error path and log to console. Add an API-availability guard to avoid noisy errors and to skip the call when unsupported.

ui/src/views/compute/wizard/ComputeOfferingSelection.vue:36

• The new .radio-option styles won’t apply because the slot content doesn’t render an element with class="radio-option"; currently it only outputs text. Wrap the slot content so the CSS is actually used (and so long names can be ellipsized as intended).
  api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:298
• The new categoryid parameter is marked as since = "24.0", but this PR’s schema upgrade targets 4.23.0.0 and related commands use since = "4.23.0". This should be 4.23.0 for consistent API versioning/annotations.

            entityType = ServiceOfferingCategoryResponse.class,
            required = false,
            description = "the ID of the service offering category to associate with this offering",
            since = "24.0")
    private Long categoryId;

engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql:35

• The migration inserts the default category with an unconditional INSERT ... VALUES (1, ...), which will fail if the upgrade script is re-run (or if the table already exists for any reason). Also, uuid is nullable even though it’s used as the API identifier (unique constraints allow multiple NULLs in MySQL). Make uuid NOT NULL and guard the default-row insert so upgrades remain robust.

CREATE TABLE IF NOT EXISTS `cloud`.`service_offering_category` (
 `id` bigint unsigned NOT NULL auto_increment,
 `name` varchar(255) NOT NULL,
 `uuid` varchar(40),
 `sort_key` int NOT NULL DEFAULT 0,
 PRIMARY KEY  (`id`),
 CONSTRAINT `uc_service_offering_category__uuid` UNIQUE (`uuid`),
 CONSTRAINT `uc_service_offering_category__name` UNIQUE (`name`)
) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8;

INSERT INTO `cloud`.`service_offering_category` (id, name, uuid) VALUES (1, 'Default', UUID());

ALTER TABLE `cloud`.`service_offering` ADD COLUMN `category_id` bigint unsigned NOT NULL DEFAULT 1;
ALTER TABLE `cloud`.`service_offering` ADD CONSTRAINT `fk_service_offering__category_id` FOREIGN KEY (`category_id`) REFERENCES `cloud`.`service_offering_category` (`id`) ON DELETE RESTRICT ON UPDATE CASCADE;

• Files reviewed: 43/43 changed files
• Comments generated: 1
• Review effort level: Lite

[Hanarion]

@DaanHoogland yes still interested

I just pushed a commit applying your 24.0 suggestion to the rest of the category API so it's consistent, and went through the remaining copilot comments : clone was ignoring the categoryid given on the command, the schema upgrade is now idempotent, and i fixed the missing category state in DeployVM.vue. I also added unit tests on the category validation.

[Copilot]

🟡 Changes recommended

Several introduced API/version metadata and UI/API-availability handling issues should be corrected to avoid inaccurate API docs and runtime errors for users lacking the new API permission.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql:30

• service_offering_category.uuid is nullable, but the object implements Identity and the table enforces a UNIQUE constraint. In MySQL, UNIQUE allows multiple NULLs, so this doesn’t actually guarantee uniqueness unless the column is NOT NULL. Consider making uuid NOT NULL to enforce invariants at the DB layer.

api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java:295

• The new category fields are marked with since = "24.0", which doesn’t match the CloudStack version format used elsewhere in this codebase (e.g. 4.22.0, 4.23.0). This affects API documentation/versioning metadata; please update to the correct release version for this feature (likely 4.23.0).

    @SerializedName("categoryid")
    @Param(description = "the ID of the service offering category", since = "24.0")
    private String categoryId;

    @SerializedName("category")
    @Param(description = "the name of the service offering category", since = "24.0")
    private String categoryName;

api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCategoryCmd.java:35

• since = "24.0" is inconsistent with the CloudStack API version strings used throughout the project (e.g. 4.23.0). Update this to the correct CloudStack release version for the new command so API docs remain accurate.

@APICommand(name = "createServiceOfferingCategory",
        description = "Creates a service offering category.",
        responseObject = ServiceOfferingCategoryResponse.class,
        since = "24.0",
        requestHasSensitiveInfo = false,
        responseHasSensitiveInfo = false)

api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:298

• The new categoryid parameter is annotated with since = "24.0", which doesn’t match the CloudStack version format used elsewhere. Please update to the correct release version (likely 4.23.0) so the API metadata stays consistent.

    @Parameter(name = ApiConstants.SERVICE_OFFERING_CATEGORY_ID,
            type = CommandType.UUID,
            entityType = ServiceOfferingCategoryResponse.class,
            required = false,
            description = "the ID of the service offering category to associate with this offering",
            since = "24.0")
    private Long categoryId;

api/src/main/java/org/apache/cloudstack/api/command/admin/offering/UpdateServiceOfferingCmd.java:118

• The new categoryid parameter is annotated with since = "24.0", which doesn’t match the CloudStack version format used elsewhere. Please update to the correct release version (likely 4.23.0) so the API metadata stays consistent.

    @Parameter(name = ApiConstants.SERVICE_OFFERING_CATEGORY_ID,
            type = CommandType.UUID,
            entityType = ServiceOfferingCategoryResponse.class,
            required = false,
            description = "the ID of the service offering category to associate",
            since = "24.0")
    private Long categoryId;

ui/src/views/compute/wizard/ComputeOfferingSelection.vue:36

• The added .radio-option / .ellipsis styles aren’t applied because the slot content only renders {{ item.name }}. As-is, these new styles are dead code and long names won’t ellipsize. Either remove the unused CSS or wrap the slot content to use the classes.

• Files reviewed: 44/44 changed files
• Comments generated: 7
• Review effort level: Lite

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 44 out of 44 changed files in this pull request and generated 4 comments.

Suppressed comments (3)

ui/src/components/offering/ComputeOfferingForm.vue:1

• fetchCategories() unconditionally calls listServiceOfferingCategories. In the same PR, other UI flows explicitly guard this call when the API isn’t available (older management server / plugin set), but this form doesn’t—so the create/edit offering UI can break in environments where the new API command isn’t registered. Add an API-availability guard (like checking ('listServiceOfferingCategories' in this.$store.getters.apis)) and handle failures by keeping categories = [] (and optionally hiding the selector).
  ui/src/components/offering/ComputeOfferingForm.vue:1
• fetchCategories() unconditionally calls listServiceOfferingCategories. In the same PR, other UI flows explicitly guard this call when the API isn’t available (older management server / plugin set), but this form doesn’t—so the create/edit offering UI can break in environments where the new API command isn’t registered. Add an API-availability guard (like checking ('listServiceOfferingCategories' in this.$store.getters.apis)) and handle failures by keeping categories = [] (and optionally hiding the selector).
  server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java:1
• These new mock methods return null/false with TODOs. That can cause hard-to-debug NPEs if a test starts invoking them indirectly. Prefer throwing UnsupportedOperationException (fails fast) or implementing a minimal deterministic stub consistent with existing mock behavior.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 44 out of 44 changed files in this pull request and generated 3 comments.

Suppressed comments (4)

ui/src/components/offering/ComputeOfferingForm.vue:1

• The category <a-select-option> does not set an explicit :value, which can result in the selected form.categoryid being undefined or not matching the intended category ID depending on Ant Design Vue behavior. Also, fetchCategories() unconditionally calls listServiceOfferingCategories without checking API availability (unlike DeployVM/DeployVnfAppliance), which will error against older backends; add the same API-existence guard and handle rejected promises (e.g., set empty list and stop loading).
  ui/src/components/offering/ComputeOfferingForm.vue:1
• The category <a-select-option> does not set an explicit :value, which can result in the selected form.categoryid being undefined or not matching the intended category ID depending on Ant Design Vue behavior. Also, fetchCategories() unconditionally calls listServiceOfferingCategories without checking API availability (unlike DeployVM/DeployVnfAppliance), which will error against older backends; add the same API-existence guard and handle rejected promises (e.g., set empty list and stop loading).
  server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java:1
• These mock methods silently return null/false, which can mask errors (or cause later NPEs) if any test hits these paths. In test mocks, prefer failing fast with UnsupportedOperationException (or implement minimal deterministic behavior) so unintended calls are immediately visible.
  ui/src/views/compute/DeployVM.vue:1
• This category-fetching logic is duplicated (also in DeployVnfAppliance.vue) with identical behavior and error handling. Consider extracting a shared helper/mixin/composable (or a store action) to avoid divergence and keep UI behavior consistent when the API contract changes.

[DaanHoogland]

@Hanarion , I think you can apply some of these co-pilot comments. I didn’t go through all of them as there is one must-change, the DB upgrade file.

[Hanarion]

@DaanHoogland applied the stubs and made the category uuid not null.

For SERVICE_OFFERING_CATEGORY_NAME, copilot is right that it's missing but the fix is on the description side. The name is never sent as a parameter, it's only returned in the response as category (@SerializedName("category") in ServiceOfferingResponse), so a constant for it would be dead code. I removed it earlier for that reason and the description was never updated. Fixed the description instead.

For the FK i'd rather keep it as is, IDEMPOTENT_ADD_FOREIGN_KEY hardcodes ON DELETE CASCADE and we need RESTRICT, otherwise deleting a category deletes all its offerings. It's already idempotent with the IDEMPOTENT_DROP_FOREIGN_KEY above.

For the schema file, there is no 23 -> 24 file yet from what i see, the chain still ends at Upgrade42210to42300. Do you want me to create it here or does it come with a version bump PR?

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 44 out of 44 changed files in this pull request and generated 3 comments.

Suppressed comments (3)

ui/src/components/offering/ComputeOfferingForm.vue:1

• The form always renders the category selector for non-system offerings and always calls listServiceOfferingCategories, but other UI code paths explicitly guard for older management servers that don’t expose this API. This can cause avoidable API errors and a broken/empty selector on older deployments. Consider conditionally showing the field and/or skipping fetchCategories() unless listServiceOfferingCategories exists in $store.getters.apis (and/or handling the catch to keep the UI stable).
  ui/src/components/offering/ComputeOfferingForm.vue:1
• The form always renders the category selector for non-system offerings and always calls listServiceOfferingCategories, but other UI code paths explicitly guard for older management servers that don’t expose this API. This can cause avoidable API errors and a broken/empty selector on older deployments. Consider conditionally showing the field and/or skipping fetchCategories() unless listServiceOfferingCategories exists in $store.getters.apis (and/or handling the catch to keep the UI stable).
  ui/src/views/compute/wizard/ComputeOfferingSelection.vue:1
• The form-item field name serviceofferingcategoryid doesn’t align with the API parameter name used elsewhere (categoryid) and the prop/event naming (selectedServiceOfferingCategoryId, service-offering-category-change). Aligning the name with the API parameter (or with the component props) will reduce confusion and make it easier to trace/filter form state consistently.

[nvazquez]

Hi @Hanarion can you please address the review comments?

[Copilot]

🟡 Changes recommended

Critical schema migration gaps and missing authorization on category management and listing APIs could break upgrades, fresh deployments, and access to the deployment wizard.

41 open issues

• Add category schema to fresh installs and the correct migration · New
• Recreate the service offering view in the upgrade migration · New
• Restrict global category management to administrators · New
• The Filter order-by field is set to \"sortKey\", but the underlying DB column is sort_key…
• This upgrade adds DDL/DML directly (CREATE/ALTER/INSERT) in a schema upgrade file that already uses…
• The schema upgrade adds a default row and new column/constraint in a way that is not restart-safe:…
• selectedServiceOfferingCategoryId and options.serviceOfferingCategories are referenced here…
• selectedServiceOfferingCategoryId, options.serviceOfferingCategories, and…
• The FK constraint is added before the default category row is inserted. During upgrade, existing…
• Authorize listing categories for deployment roles · New
• Link category action events to the affected resource · New
• Render category labels in the overflow dropdown · New
• The hard-coded default category ID (1L) is a fragile “magic number” that tightly couples runtime…
• Hardcoding the default category as id=1 is brittle (schema or deployments could evolve, and it’s…
• The FK creation uses a raw ALTER TABLE ... ADD CONSTRAINT, which is not idempotent. Since the…
• getCategoryId() is defined as a primitive long, but the category association is treated as…
• fetchServiceOfferingCategories() calls listServiceOfferingCategories unconditionally. In the…
• fetchServiceOfferingCategories() calls listServiceOfferingCategories unconditionally. In the…
• Hard-coding the default category id as 1L bakes a schema assumption into application logic. A…
• DeployVM passes options.serviceOfferingCategories / selectedServiceOfferingCategoryId into…

And 21 more that still need to be addressed.

4 resolved since last review

• The migration uses a raw ALTER TABLE ... ADD CONSTRAINT which is not idempotent; re-running the…
• The since = "24.0" metadata looks inconsistent with the existing versioning scheme in the…
• The uuid column is nullable while also being declared UNIQUE. This allows multiple rows with…
• SERVICE_OFFERING_CATEGORY_NAME is introduced but never referenced anywhere in this PR (no…

_{Review effort: Lite}

[Copilot]

🟡 Changes recommended

The migration has unresolved idempotency and view-recreation issues that can break retries and cause existing installations' service-offering queries to fail.

40 open issues

• Recreate the service offering view in the upgrade · New
• Restrict global category management to administrators
• Recreate the service offering view in the upgrade migration
• Add category schema to fresh installs and the correct migration
• The Filter order-by field is set to \"sortKey\", but the underlying DB column is sort_key…
• This upgrade adds DDL/DML directly (CREATE/ALTER/INSERT) in a schema upgrade file that already uses…
• The schema upgrade adds a default row and new column/constraint in a way that is not restart-safe:…
• selectedServiceOfferingCategoryId and options.serviceOfferingCategories are referenced here…
• selectedServiceOfferingCategoryId, options.serviceOfferingCategories, and…
• The FK constraint is added before the default category row is inserted. During upgrade, existing…
• Make foreign-key migration idempotent · New
• Link category action events to the affected resource
• The hard-coded default category ID (1L) is a fragile “magic number” that tightly couples runtime…
• Hardcoding the default category as id=1 is brittle (schema or deployments could evolve, and it’s…
• The FK creation uses a raw ALTER TABLE ... ADD CONSTRAINT, which is not idempotent. Since the…
• fetchServiceOfferingCategories() calls listServiceOfferingCategories unconditionally. In the…
• fetchServiceOfferingCategories() calls listServiceOfferingCategories unconditionally. In the…
• Hard-coding the default category id as 1L bakes a schema assumption into application logic. A…
• DeployVM passes options.serviceOfferingCategories / selectedServiceOfferingCategoryId into…
• cloneServiceOffering always passes the source offering category (sourceOffering.getCategoryId())…

And 20 more that still need to be addressed.

3 resolved since last review

• Render category labels in the overflow dropdown
• Authorize listing categories for deployment roles
• getCategoryId() is defined as a primitive long, but the category association is treated as…

_{Review effort: Lite}