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.

Also, create common type ExtraSpec and use it for the Flavor and VolumeType.

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.

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

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.

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.

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

Add flavor extra_specs

597bac6

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

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

Conversation

chess-knight commented Jun 11, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants