Add flavor extra_specs by chess-knight · Pull Request #817 · k-orc/openstack-resource-controller

chess-knight · 2026-06-11T10:10:23Z

Add Flavor extra_specs.

Closes #738

eshulman2

Thanks for the PR, generally LGTM except for the missing interface aliases.

eshulman2 · 2026-06-15T08:58:16Z

+	return deletes
+}
+
+func (actuator flavorActuator) GetResourceReconcilers(ctx context.Context, orcObject orcObjectPT, osResource *osResourceT, controller generic.ResourceController) ([]resourceReconciler, progress.ReconcileStatus) {


flavorActuator now implements ReconcileResourceActuator via GetResourceReconcilers, but the type alias and compile-time assertion are missing. Compare securitygroup/actuator.go which has:

reconcileResourceActuator = interfaces.ReconcileResourceActuator[orcObjectPT, orcObjectT, osResourceT] // ... var _ reconcileResourceActuator = securityGroupActuator{}

The resourceReconciler alias was added, but reconcileResourceActuator wasn't. Please add:

reconcileResourceActuator = generic.ReconcileResourceActuator[orcObjectPT, orcObjectT, osResourceT]

var _ reconcileResourceActuator = flavorActuator{}

To be fair, this was not a big problem. This compile-time assertions are there to help developers and provide quicker feedback loop to ensure the actuator satisfies the interface. It has no functional use.

eshulman2 · 2026-06-15T09:05:05Z

 	}
 }
+
+func TestExtraSpecUpdates(t *testing.T) {


Nice to have: would be great to add a test covering the entire logic of the update function eg. what happens if update succeed but delete fails, etc

eshulman2 · 2026-06-15T09:34:29Z

+	// +kubebuilder:validation:MaxItems:=128
+	// +listType=atomic
+	// +optional
+	ExtraSpecs []ExtraSpec `json:"extraSpecs,omitempty"`


The []ExtraSpec slice has no uniqueness constraint on name. If a user specifies the same name twice, extraSpecsToMap silently uses the last value - no error or warning at admission time.

A CEL validation rule on the field would catch this at admission time:

// +kubebuilder:validation:XValidation:rule="self.all(x, self.filter(y, y.name == x.name).size() == 1)",message="extraSpecs names must be unique" ExtraSpecs []ExtraSpec `json:"extraSpecs,omitempty"`

Thinking about it, we should probably change the the list type to map.

// extraSpecs is a map of key-value pairs that define extra specifications for the flavor. // +kubebuilder:validation:MaxItems:=128 // +listType=map // +listMapKey=name // +optional ExtraSpecs []ExtraSpec `json:"extraSpecs,omitempty"`

eshulman2 · 2026-06-15T09:34:53Z

 	// +listType=atomic
 	// +optional
-	ExtraSpecs []VolumeTypeExtraSpec `json:"extraSpecs,omitempty"`
+	ExtraSpecs []ExtraSpec `json:"extraSpecs,omitempty"`


same as above:

The []ExtraSpec slice has no uniqueness constraint on name. If a user specifies the same name twice, extraSpecsToMap silently uses the last value - no error or warning at admission time.

A CEL validation rule on the field would catch this at admission time:

// +kubebuilder:validation:XValidation:rule="self.all(x, self.filter(y, y.name == x.name).size() == 1)",message="extraSpecs names must be unique" ExtraSpecs []ExtraSpec `json:"extraSpecs,omitempty"`

There too, this should be change to a map list type, but this should be done in a separate PR.

We'll still need to revert the changes and keep the VolumeTypeExtraSpec type.

chess-knight · 2026-06-17T12:23:26Z

Thanks for the PR, generally LGTM except for the missing interface aliases.

Thank you for the review, I added everything requested

eshulman2 · 2026-06-23T08:07:34Z

@mandre can you please approve the workflow, then I'll re-review

mandre · 2026-06-24T06:20:44Z

@chess-knight apologies for the lack of input on your PR, I was quite busy lately. I'll review very soon, stay tuned.

mandre

Oof, this was much more work than I originally expected due to the fact this is actually a separate API call. There are a few critical changes we need to make (use a separate FlavorExtraSpecs stuct, and drop the change from CreateResource) before we can merge.

mandre · 2026-06-24T07:09:21Z

 	// description contains a free form description of the flavor.
 	// +kubebuilder:validation:MinLength:=1
 	// +kubebuilder:validation:MaxLength:=65535
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="description is immutable"


Just noting that the description should in theory be mutable (https://docs.openstack.org/api-ref/compute/#update-flavor-description) but this is clearly out of scope for this PR.

Yes, I know about mutable description, but as you said i didn't want to change it with this PR.

mandre · 2026-06-24T07:41:16Z

+	if len(resource.ExtraSpecs) > 0 {
+		extraSpecs := extraSpecsToMap(resource.ExtraSpecs)
+
+		_, err = actuator.osClient.CreateFlavorExtraSpecs(ctx, osResource.ID, flavors.ExtraSpecsOpts(extraSpecs))


We can't create the extra spec in CreateResource because this is a different API call that might fail. This should be handled in a separate reconciliation, and we should rely on updateResource instead (or better even, call it reconcileExtraSpecs).

mandre · 2026-06-24T08:04:16Z

 // +kubebuilder:validation:MaxLength:=64
 type KeystoneName string
+
+type ExtraSpec struct {


I'm unsure about making the ExtraSpec a common type. The different projects might enforce different rules.

For instance, nova allows spaces in flavors extraspec keys while cinder does not for volumetypes extraspec keys.

BTW, we should add CEL validations for those.

Should we add this validations as part of this PR? or we will be fine with MaxLength:=255 for now

In this PR please. We'll tighten up the validation for the volumetype in a separate PR.

mandre · 2026-06-24T08:07:04Z

+	// +kubebuilder:validation:MaxItems:=128
+	// +listType=atomic
+	// +optional
+	ExtraSpecs []ExtraSpec `json:"extraSpecs,omitempty"`


Thinking about it, we should probably change the the list type to map.

// extraSpecs is a map of key-value pairs that define extra specifications for the flavor. // +kubebuilder:validation:MaxItems:=128 // +listType=map // +listMapKey=name // +optional ExtraSpecs []ExtraSpec `json:"extraSpecs,omitempty"`

mandre · 2026-06-24T08:10:23Z

 	// +listType=atomic
 	// +optional
-	ExtraSpecs []VolumeTypeExtraSpec `json:"extraSpecs,omitempty"`
+	ExtraSpecs []ExtraSpec `json:"extraSpecs,omitempty"`


There too, this should be change to a map list type, but this should be done in a separate PR.

We'll still need to revert the changes and keep the VolumeTypeExtraSpec type.

mandre · 2026-06-24T08:29:08Z

 	return progress.WrapError(actuator.osClient.DeleteFlavor(ctx, flavor.ID))
 }

+func (actuator flavorActuator) updateResource(ctx context.Context, obj orcObjectPT, osResource *osResourceT) progress.ReconcileStatus {


Let's rename this function to reconcileExtraSpecs, as this is a single concern reconciler.

mandre · 2026-06-24T08:40:21Z

+	}
+
+	if len(updates) > 0 {
+		_, err := actuator.osClient.CreateFlavorExtraSpecs(


OK, it seems (from the gophercloud comment at least) that nova does an upsert.

The nova doc doesn't state it explicitly.

yes, if you look into the code create and update are basically the same.

mandre

Nice work! just a small nit, then I believe this is ready to merge.

mandre · 2026-06-24T15:37:34Z

+	resource := obj.Spec.Resource
+	if resource == nil {
+		// Should have been caught by API validation
+		return progress.WrapError(


Let's just return nil in this case, this would be more consistent with other reconcile functions:

openstack-resource-controller/internal/controllers/trunk/actuator.go

Lines 307 to 309 in 9b8ee08

if resource == nil {

return nil

}

mandre · 2026-06-24T15:39:45Z

 	"errors"
 	"fmt"
 	"iter"
+	reflect "reflect"


I'm surprised this isn't flagged by the linter.

Suggested change

reflect "reflect"

"reflect"

mandre · 2026-06-24T15:41:18Z

+
+type FlavorExtraSpecStatus struct {
+	// name is the name of the extraspec
+	// +kubebuilder:validation:Pattern="^[a-zA-Z0-9-_:. ]+$"


We don't really care about enforcing a pattern on the status, we'll just return what OpenStack gives us. Not a big deal, though...

Signed-off-by: Roman Hros <roman.hros@dnation.cloud>

chess-knight force-pushed the flavor-extra-specs branch from d67e89a to 8de585f Compare June 11, 2026 10:20

github-actions Bot added the semver:major Breaking change label Jun 11, 2026

eshulman2 suggested changes Jun 15, 2026

View reviewed changes

eshulman2 reviewed Jun 15, 2026

View reviewed changes

chess-knight force-pushed the flavor-extra-specs branch from 8de585f to 597bac6 Compare June 17, 2026 12:21

mandre reviewed Jun 24, 2026

View reviewed changes

chess-knight force-pushed the flavor-extra-specs branch from 597bac6 to 38b177f Compare June 24, 2026 14:14

chess-knight requested a review from mandre June 24, 2026 14:14

mandre reviewed Jun 24, 2026

View reviewed changes

Add flavor extra_specs

0b914d0

Signed-off-by: Roman Hros <roman.hros@dnation.cloud>

chess-knight force-pushed the flavor-extra-specs branch from 38b177f to 0b914d0 Compare June 25, 2026 06:23

mandre approved these changes Jun 25, 2026

View reviewed changes

mandre added this pull request to the merge queue Jun 25, 2026

Merged via the queue into k-orc:main with commit 4c6aef7 Jun 25, 2026
10 checks passed

Uh oh!

Conversation

chess-knight commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eshulman2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chess-knight commented Jun 17, 2026

Uh oh!

eshulman2 commented Jun 23, 2026

Uh oh!

mandre commented Jun 24, 2026

Uh oh!

mandre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mandre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chess-knight commented Jun 11, 2026 •

edited

Loading