Skip to content

feat(castellum): Add castellum and Manila share resources#250

Open
munchtoast wants to merge 3 commits into
SAP-cloud-infrastructure:masterfrom
munchtoast:add-castellum
Open

feat(castellum): Add castellum and Manila share resources#250
munchtoast wants to merge 3 commits into
SAP-cloud-infrastructure:masterfrom
munchtoast:add-castellum

Conversation

@munchtoast

@munchtoast munchtoast commented Jun 2, 2026

Copy link
Copy Markdown
  • Add sci_castellum_resource_v1 resource for configuring Castellum autoscaling policies on OpenStack project resources.
  • Upgrade gophercloud-sapcc/v2 to fork version containing the castellum/v1/resources client.
  • bump gophercloud-sapcc to 2.1.0

@cla-assistant

cla-assistant Bot commented Jun 2, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@kayrus

kayrus commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Hi, thanks for the PR. Could you please explain what is the reason for adding the sci_share_v2 resource? Does it make sense to extend this resource in upstream https://github.com/terraform-provider-openstack/terraform-provider-openstack?

arc and automation

these resources will be removed within #248 PR

- Add sci_castellum_resource_v1 resource for managing Castellum autoscaling
  configuration on a per-project, per-resource-type basis.
- Register castellum client via gophercloud-sapcc/v2 v2.1.0.
@munchtoast

Copy link
Copy Markdown
Author

Hi, thanks for the PR. Could you please explain what is the reason for adding the sci_share_v2 resource? Does it make sense to extend this resource in upstream https://github.com/terraform-provider-openstack/terraform-provider-openstack?

arc and automation

these resources will be removed within #248 PR

sci_share removed as openstack_sharedfilesystem_share_v2 accomplishes the same thing upstream.. I have also rebased the branch with the new deprecation changes.

@kayrus kayrus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my new comments. and could you add a corresponding documentation with a couple of examples?

Comment thread sci/sci_castellum_resource_v1.go Outdated
@@ -0,0 +1,5 @@
// SPDX-FileCopyrightText: 2020-2026 SAP SE or an SAP affiliate company

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file can be removed

Comment thread sci/resource_sci_autoscaling_v1.go
return opts
}

func castellumSCICastellumResourceV1ExpandThreshold(raw any) *castellum.Threshold {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function (and other functions like it) should return Option[T] instead of *T to avoid useless legwork converting between both formats at the callsite.

Comment on lines +352 to +356
if v, ok := sc.Minimum.Unpack(); ok {
m["minimum"] = int(v)
} else {
m["minimum"] = 0
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This, and other instance of this pattern, can be simplified into:

Suggested change
if v, ok := sc.Minimum.Unpack(); ok {
m["minimum"] = int(v)
} else {
m["minimum"] = 0
}
m["minimum"] = int(sc.Minimum.UnwrapOr(0))

Comment on lines +357 to +361
if v, ok := sc.Maximum.Unpack(); ok {
m["maximum"] = int(v)
} else {
m["maximum"] = 0
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an incorrect default value. maximum = None is wildly different from maximum = 0. In fact, it's the polar opposite.

Comment on lines +386 to +390
parts := strings.SplitN(id, "/", 2)
if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
return "", "", fmt.Errorf("invalid sci_castellum_resource_v1 ID %q: expected <project_id>/<resource_type>", id)
}
return parts[0], parts[1], nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use strings.Cut instead.

Comment on lines +399 to +403
if t, ok := resource.LowThreshold.Unpack(); ok {
_ = d.Set("low_threshold", castellumSCICastellumResourceV1FlattenThreshold(t))
} else {
_ = d.Set("low_threshold", []map[string]any{})
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if t, ok := resource.LowThreshold.Unpack(); ok {
_ = d.Set("low_threshold", castellumSCICastellumResourceV1FlattenThreshold(t))
} else {
_ = d.Set("low_threshold", []map[string]any{})
}
_ = d.Set("low_threshold", options.Map(resource.LowThreshold, castellumSCICastellumResourceV1FlattenThreshold).UnwrapOr([]map[string]any{}))

Same for the other similar instances below.

Comment on lines +288 to +297
m, ok := list[0].(map[string]any)
if !ok {
return nil
}
return &castellum.Threshold{
UsagePercent: castellum.UsageValues{
castellum.SingularUsageMetric: m["usage_percent"].(float64),
},
DelaySeconds: uint32(m["delay_seconds"].(int)),
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation only works for resources that have a single usage metric (like Manila shares), but not for ones with multiple usage metrics (e.g. server groups that scale both on CPU and RAM usage). I'm not asking that this functionality be included in this PR, but the structure of the threshold configuration should be declared in a way that allows the natural expansion to multidimensional usage thresholds later.

For example, we could check m["metric"]here and, instead of pickinglist[0], pick an element from listthat has the key"metric": castellum.SingularUsageMetric`.

@kayrus

kayrus commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

@munchtoast

could you add a corresponding documentation with a couple of examples?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants