Manila: Share network by dlaw4608 · Pull Request #691 · k-orc/openstack-resource-controller

dlaw4608 · 2026-02-24T18:05:44Z

Add support for Openstack Manila Service Share Network resource
Closes: #687

github-actions · 2026-02-24T18:06:03Z

Failed to assess the semver bump. See logs for details.

winiciusallan

Hey @dlaw4608, I left a few comments, but I need some more time to take a look at the other test scenarios. For now, let me know what you think about what it is commented.

winiciusallan · 2026-03-01T18:41:13Z

api/v1alpha1/sharenetwork_types.go

 	// +optional
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="networkRef is immutable"
 	NetworkRef *KubernetesNameRef `json:"networkRef,omitempty"`

 	// subnetRef is a reference to the ORC Subnet which this resource is associated with.
 	// +optional
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="subnetRef is immutable"
 	SubnetRef *KubernetesNameRef `json:"subnetRef,omitempty"`


Looks like both NetworkRef and SubnetRef are required according to the docs.

Specify both a neutron network and a neutron subnet that belongs to that neutron network.

Also, you've specified these two fields in the create-minimal, so I'm assuming that the claim above is true. Isn't it better to define these two fields as required and remove the omitempty tag?

Interesting because when i look at the API docs it says that the fields are optional, now in what scenario are you not including a neutron_id or neutron_subnet when creating a share_network resource I'm not quite sure 🤷, but I guess I was more just reflecting what the fields were shown as in the API and gophercloud. I am curious though to the documentation you refer to above, so please share a link if you can 👍 , happy to make the change if needed.

For the moment though I just stripped the dependencies from the create-minimal assertion in step 0, and left the dependencies as optional.

Sorry, I wasn't too clear. I meant that network and subnet should both be passed together and I thought they were required, but this is wrong.

Ella has already pointed out a comment to use a validation to ensure that these two fields will be passed together in the request, so you can consider this as resolved :)

winiciusallan · 2026-03-01T18:52:21Z

internal/controllers/sharenetwork/status.go

-const ShareNetworkStatusInUse     = "in-use"
-const ShareNetworkStatusDeleting  = "deleting"
+
+const (


I believe we're not using any of these statuses throughout the code, and it looks like we don't have a status field in the ShareNetwork struct on Gophercloud's side. Is this useful?

Maybe do we need to create an issue to address this?

Yes you are correct they are not necessary here I removed them, thanks 👍

winiciusallan · 2026-03-01T19:00:19Z

internal/controllers/sharenetwork/tests/sharenetwork-create-full/00-assert.yaml

+    - celExpr: "sharenetwork.status.resource.name == 'sharenetwork-create-full-override'"
+    - celExpr: "sharenetwork.status.resource.description == 'ShareNetwork from \"create full\" test'"


We don't need this check since KUTTL already matches fields from declaration vs status.

Thanks for the Feedback @winiciusallan , i could of sworn i had already left these comments last week but clearly never actually clicked the big green button ack.

It happens 😄

eshulman2

Generally looks good, dded a few comments and question, please LMK what you think.

eshulman2 · 2026-03-09T09:44:19Z

.github/workflows/e2e.yaml

        enable_workaround_docker_io: 'false'
        branch: ${{ matrix.openstack_version }}
-        enabled_services: "openstack-cli-server,neutron-trunk"
+        enabled_services: "openstack-cli-server,neutron-trunk,manila,m-api,m-sch,m-shr,m-dat"


This comment is not specific to this patch but I believe we should consider a coherent approach to the services we enable in the gates and possibly making specific gates with specific services enabled for certain controllers like being done in gophercloud for loadbalancers for example. @mandre WDYT?

Agreed. At some point we'll have to split our jobs as they grow bigger (gophercloud is a good example).

eshulman2 · 2026-03-09T09:48:53Z

api/v1alpha1/sharenetwork_types.go

+	// networkRef is a reference to the ORC Network which this resource is associated with.
+	// +optional
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="networkRef is immutable"
+	NetworkRef *KubernetesNameRef `json:"networkRef"`


Should there be a rule here that make sure that if a network is set the subnet is also set? from the api referance:

The UUID of a neutron network when setting up or updating a share network subnet with neutron. Specify both a neutron network and a neutron subnet that belongs to that neutron network.

I agree! , I created a new validation rule

// +kubebuilder:validation:XValidation:rule="has(self.networkRef) == has(self.subnetRef)",message="networkRef and subnetRef must be specified together"

also added a test file in the apivalidations directory here, I used securitygroup_test.go and network_test.go as my main sources of reference (both in the apivalidations dir),

The CEL rule is a boolean equality and tests 4 combinations, both networkRef and subnetRef present = valid, both networkRef and subnetRef absent = valid aswell, but if either network Ref or subnetRef is missing or vice versa it is flagged as invalid.
The idea is to follow the API Ref statement creating a both or neither configuration rule.

The UUID of a neutron network when setting up or updating a share network subnet with neutron. Specify both a neutron network and a neutron subnet that belongs to that neutron network.

please lmk what you think 👍

eshulman2 · 2026-03-09T09:50:40Z

api/v1alpha1/sharenetwork_types.go

+
+	// networkRef is a reference to the ORC Network which this resource is associated with.
+	// +optional
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="networkRef is immutable"


As far as I can tell from the API docs it is possible to change this https://docs.openstack.org/api-ref/shared-file-system/#update-share-network any specific reason for making it immutable?

Nope, you are correct the validation test is not required here, with the two dependencies being mutable according to the API, Thanks 👍

That's true. But it's also true that it's not a problem leaving a field immutable in the first implementation as writing a controller that supports all the fields, with dependency and mutability can sometimes be quite a lot of work.
I prefer to have a partially implemented controller that is robust than a half baked fully implemented controller.

eshulman2 · 2026-03-09T09:51:18Z

api/v1alpha1/sharenetwork_types.go

+
+	// subnetRef is a reference to the ORC Subnet which this resource is associated with.
+	// +optional
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="subnetRef is immutable"


same as in https://github.com/k-orc/openstack-resource-controller/pull/691/changes#r2904348085

Thanks again, this validation rule was generated by the scaffolding tool both have been removed.

eshulman2

regarding the gate failures

eshulman2 · 2026-03-09T11:57:31Z

internal/controllers/sharenetwork/tests/sharenetwork-create-minimal/00-assert.yaml

+      ref: sharenetwork
+assertAll:
+    - celExpr: "sharenetwork.status.id != ''"
+    - celExpr: "sharenetwork.status.resource.neutronNetID == '"


seems like there is something wrong with the test here:

harness.go:395: failed to prepare expression evaluation: failed to load expression(s): failed to build CEL program from expression "sharenetwork.status.resource.neutronNetID == '": type-check error: ERROR: <input>:1:46: Syntax error: token recognition error at: ''' | sharenetwork.status.resource.neutronNetID == ' | .............................................^ ERROR: <input>:1:47: Syntax error: mismatched input '<EOF>' expecting {'[', '{', '(', '.', '-', '!', 'true', 'false', 'null', NUM_FLOAT, NUM_INT, NUM_UINT, STRING, BYTES, IDENTIFIER} | sharenetwork.status.resource.neutronNetID == ' | ..............................................^

I suspect it is a small typo yo have only a single ' instead of what I assumed you mean which is ' ' but I might be wrong

Good Spot, thanks for the help @eshulman2 👍

github-actions · 2026-03-09T17:04:19Z

Failed to assess the semver bump. See logs for details.

go run ./cmd/scaffold-controller -interactive=false \ -kind=ShareNetwork \ -gophercloud-client=NewSharedFilesystemV2 \ -gophercloud-module=github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/sharenetworks \ -gophercloud-type=ShareNetwork \ -openstack-json-object=share_network \ -optional-create-dependency=Network \ -optional-create-dependency=Subnet Signed-off-by: Daniel Lawton <dlawton@redhat.com>

Implements ShareNetwork controller to manage Manila share networks. - E2E tests included - API configured - Manila enabled in CI Signed-off-by: Daniel Lawton <dlawton@redhat.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

mandre

Just an initial pass for now, I believe we should make the dependencies immutable unless we have more test coverage proving it works as expected, and that the resources that are no longer dependencies no longer have a finalizer.

mandre · 2026-03-10T15:15:46Z

.github/workflows/e2e.yaml

        enable_workaround_docker_io: 'false'
        branch: ${{ matrix.openstack_version }}
-        enabled_services: "openstack-cli-server,neutron-trunk"
+        enabled_services: "openstack-cli-server,neutron-trunk,manila,m-api,m-sch,m-shr,m-dat"


Agreed. At some point we'll have to split our jobs as they grow bigger (gophercloud is a good example).

mandre · 2026-03-10T15:28:10Z

api/v1alpha1/sharenetwork_types.go

+
+	// networkRef is a reference to the ORC Network which this resource is associated with.
+	// +optional
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="networkRef is immutable"


That's true. But it's also true that it's not a problem leaving a field immutable in the first implementation as writing a controller that supports all the fields, with dependency and mutability can sometimes be quite a lot of work.
I prefer to have a partially implemented controller that is robust than a half baked fully implemented controller.

mandre · 2026-03-10T16:34:57Z

internal/controllers/sharenetwork/tests/sharenetwork-create-full/00-assert.yaml

  resource:
    name: sharenetwork-create-full-override
    description: ShareNetwork from "create full" test
-    # TODO(scaffolding): Add all fields the resource supports


What about SegmentationID, CIRD, NetworkType, ProjectID, etc. ? We should test their value, or lack thereof.

mandre · 2026-03-10T16:50:05Z

internal/controllers/sharenetwork/tests/sharenetwork-create-minimal/00-assert.yaml

 status:
  resource:
    name: sharenetwork-create-minimal
-    # TODO(scaffolding): Add all fields the resource supports


There too, we should make sure the resulting resource is exactly as we expect it to be, including for absence of fields in its status.

mandre · 2026-03-10T16:51:29Z

internal/controllers/sharenetwork/tests/sharenetwork-create-minimal/00-create-resource.yaml

  managementPolicy: managed
-  # TODO(scaffolding): Only add the mandatory fields. It's possible the resource
-  # doesn't have mandatory fields, in that case, leave it empty.
  resource: {}


mandre · 2026-03-10T16:58:18Z

api/v1alpha1/sharenetwork_types.go

+package v1alpha1
+
+// ShareNetworkResourceSpec contains the desired state of the resource.
+type ShareNetworkResourceSpec struct {


Did you forget to run the generator with -optional-create-dependency=Project or was it intentional to leave it out in the first iteration?

mandre · 2026-03-10T17:06:55Z

internal/controllers/sharenetwork/tests/sharenetwork-update/01-assert.yaml

  resource:
    name: sharenetwork-update-updated
    description: sharenetwork-update-updated
-    # TODO(scaffolding): match all fields that were modified


We need to validate that updating networkRef and subnetRef had any effect.

mandre · 2026-03-10T17:10:20Z

internal/controllers/sharenetwork/actuator.go

 	handleNameUpdate(&updateOpts, obj, osResource)
 	handleDescriptionUpdate(&updateOpts, resource, osResource)

-	// TODO(scaffolding): add handler for all fields supporting mutability


We should add a dependence on network and subnet here, and ensure we set the updateOpts correctly.

We may also need to remove the finalizer on the old dependencies, that might be a tricky one to handle. I would suggest you make the dependencies immutable for now.

dlaw4608 force-pushed the share_network branch from 4f9c96e to 46a1765 Compare February 24, 2026 18:13

github-actions bot added the semver:major Breaking change label Feb 24, 2026

dlaw4608 force-pushed the share_network branch 2 times, most recently from 79282c8 to 08867bf Compare February 24, 2026 19:10

dlaw4608 marked this pull request as ready for review February 25, 2026 10:21

winiciusallan reviewed Mar 1, 2026

View reviewed changes

dlaw4608 force-pushed the share_network branch 2 times, most recently from b5292e8 to dacb270 Compare March 4, 2026 18:29

eshulman2 reviewed Mar 9, 2026

View reviewed changes

dlaw4608 force-pushed the share_network branch from dacb270 to 07f8a13 Compare March 9, 2026 17:04

github-actions bot removed the semver:major Breaking change label Mar 9, 2026

dlaw4608 force-pushed the share_network branch from 07f8a13 to c9974c3 Compare March 9, 2026 17:35

github-actions bot added the semver:major Breaking change label Mar 9, 2026

Added ShareNetwork Controller for Manila

bc22e2b

Implements ShareNetwork controller to manage Manila share networks. - E2E tests included - API configured - Manila enabled in CI Signed-off-by: Daniel Lawton <dlawton@redhat.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

dlaw4608 force-pushed the share_network branch from c9974c3 to bc22e2b Compare March 10, 2026 13:10

mandre reviewed Mar 10, 2026

View reviewed changes

		- celExpr: "sharenetwork.status.resource.name == 'sharenetwork-create-full-override'"
		- celExpr: "sharenetwork.status.resource.description == 'ShareNetwork from \"create full\" test'"

Conversation

dlaw4608 commented Feb 24, 2026

Uh oh!

github-actions bot commented Feb 24, 2026

Uh oh!

winiciusallan 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!

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!

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!

eshulman2 left a comment

Choose a reason for hiding this comment

Uh oh!

eshulman2 Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 9, 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

eshulman2 Mar 9, 2026 •

edited

Loading