Add a new smb admin API sub-package #1100

phlogistonjohn · 2025-04-07T19:20:21Z

Add a new smb admin api.
This should be compatible with the recently added smb mgr module in ceph.

Note that the smb module on the ceph side supports a declarative style api where resource specs are the full power interface to use. There are some imperative style apis but we don't need to implement those in this library.

Checklist

Added tests for features and functional changes
Public functions and types are documented
Standard formatting is applied to Go code
Is this a new API? Added a new file that begins with //go:build ceph_preview
Ran make api-update to record new APIs

New or infrequent contributors may want to review the go-ceph Developer's Guide including the section on how we track API Status and the API Stability Plan.

The go-ceph project uses mergify. View the mergify command guide for information on how to interact with mergify. Add a comment with @Mergifyio rebase to rebase your PR when github indicates that the PR is out of date with the base branch.

phlogistonjohn · 2025-04-16T13:41:17Z

I was trying to remember why I did not check off the Ran make api-update checklist item. After a minute of thought I remembered that the ceph_main build tag prevents the api from being added normally. What I mean to do is leave this in draft until there's at least a pre-tentacle test job available and then rework the build tags so that this code will apply on tentacle.

phlogistonjohn · 2025-04-30T17:54:57Z

@anoopcs9 @ansiwen now that Anoop has added a pre-tentacle job I figured it would be finally time to finish this up. Please review. While it's no rush I know that at least one cloud vendor is interested.

anoopcs9

This looks really good and well structured, thanks.

I only have few minor comments with a suggestion to place the commit admin/smb: add resources.go containing resource management funcs before admin/smb: add result.go for the apply result types as I had to jump to check for resourceEntry from a further commit.

common/admin/smb/cluster.go

anoopcs9 · 2025-05-05T05:09:58Z

common/admin/smb/cluster.go

+	IntentValue       Intent            `json:"intent"`
+	ClusterID         string            `json:"cluster_id"`
+	AuthMode          ClusterAuthMode   `json:"auth_mode"`
+	DomainSettings    *DomainSettings   `json:"domain_settings,omitempty"`
+	UserGroupSettings []UserGroupSource `json:"user_group_settings,omitempty"`
+	CustomDNS         []string          `json:"custom_dns,omitempty"`
+	Placement         Placement         `json:"placement,omitempty"`
+	Clustering        Clustering        `json:"clustering,omitempty"`
+	PublicAddrs       []PublicAddress   `json:"public_addrs,omitempty"`


DomainSettings *DomainSettings . . . Placement Placement Clustering Clustering

I hope we don't have any generic guideline to not use the same name for field and its type.

No, I think this is fairly common in Go, but if you think I'm mistaken about that or it just bothers you a lot, let me know.

If you happen to update the patch set again I would like to suggest a change to name it slightly different to avoid a first look confusion. Feel free to ignore if you think its not worth it.

I think it's preferable and I think it is a fairly common convention but I can't find too many links discussing this exact point. I'm going to leave it as-is and wait to see what @ansiwen thinks about this.

I don't have a strong opinion, I just know that I sometime also use identical names and types, because it feels awkward otherwise, but I also get the point @anoopcs9 is talking about. Usually I would go whatever we do otherwise in go-ceph. Sorry, I'm a bad tie-breaker in this case.

common/admin/smb/share.go

common/admin/smb/cluster_test.go

common/admin/smb/join_auth_test.go

common/admin/smb/ref.go

common/admin/smb/share.go

common/admin/smb/resources.go

phlogistonjohn · 2025-05-05T12:33:19Z

I fixed a lot of the low hanging fruit items that you pointed out, thanks! I will fold/squash the temp commits later after I spend a few more minutes thinking about the remaining items. In the meantime I will let the tests run to find any mistakes in these fixes :-)

anoopcs9 · 2025-05-05T13:40:20Z

We see CI failure on main because the regression reported via #1107 got fixed on Ceph side.

phlogistonjohn · 2025-05-05T15:36:33Z

fixes and udates squashed, branch rebased

common/admin/smb/share.go

common/admin/smb/share_test.go

common/admin/smb/users_groups_test.go

common/admin/smb/cluster_test.go

phlogistonjohn · 2025-05-07T15:12:34Z

Thanks for finding all my lazy copy-and-pasteoid errors that I missed. :-D

anoopcs9

lgtm, thanks.

mergify · 2025-05-17T15:12:18Z

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

anoopcs9 · 2025-05-18T08:47:33Z

@Mergifyio rebase

mergify · 2025-05-18T08:47:40Z

rebase

✅ Branch has been successfully rebased

mergify · 2025-05-28T08:49:48Z

anoopcs9 · 2025-05-28T09:36:16Z

@Mergifyio requeue

mergify · 2025-05-28T09:36:54Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

Part of the ceph command api supports commands with additional data in a buffer. Add interfaces that match these function calls in the go-ceph rados package. Signed-off-by: John Mulligan <[email protected]>

Signed-off-by: John Mulligan <[email protected]>

The admin_test.go module is the place where we actually test the smb module communicating with our go-ceph smb subpackage. Signed-off-by: John Mulligan <[email protected]>

Add a small commentary on how the smb module is expected to be used. Signed-off-by: John Mulligan <[email protected]>

Signed-off-by: John Mulligan <[email protected]>

Pull request has been modified.

Signed-off-by: John Mulligan <[email protected]>

ansiwen · 2025-06-04T15:54:43Z

common/admin/smb/cluster.go

+
+// MarshalJSON supports marshalling a cluster to JSON.
+func (cluster *Cluster) MarshalJSON() ([]byte, error) {
+	type vCluster Cluster


It's not clear to me, what v and w stands for here. Just like x and y?

I think I started off with v for virtual, and then needed another placeholder so I just chose w to follow on with v. These types only exist to wrangle the json so I don't think I put a lot of thought into the naming here.

ansiwen · 2025-06-04T16:10:52Z

common/admin/smb/enums.go

+
+const (
+	// Present resources will be created or updated.
+	Present = Intent("present")


Probably has no real-world effect, but want to mention it anyway: like errors.New() using a pointer to a string instead of the string itself makes the switch-cases faster and allows identical string in different enums.

I don't think I would want to change it now, but I will keep that in mind for the future.

ansiwen · 2025-06-04T17:08:46Z

common/admin/smb/resources.go

+	return false
+}
+
+var rl = []rune("bcdfghjklmnpqrstvwxyz")


Why are some characters omitted?

So that it makes junk strings and not words. I saw some other project doing that (k8s?) and kinda copied the idea.

ansiwen · 2025-06-04T17:12:37Z

common/admin/smb/resources.go

+// errorMerge is a helper function for combining a lower level error from
+// the api with a resultGroup, treating the result group as an error if it
+// is not successful.
+func errorMerge(results ResultGroup, err error) error {


I think "merge" is misleading, it is rather selecting.

Far enough, it's a private function so we can change it whenever. Because I am anxious to land this PR after it's been open for almost 2 months I'm going to leave it as-is but I might come back in a follow up PR to adjust this.

No great alternatives are jumping to mind, let me know if you have any suggestions.

I thought that was obvious from my wording: errorSelect, but I actually think errorPick is even better!

ansiwen

I left a few comments, but no blockers and otherwise LGTM as far as I can tell without knowing more about the business logic. I set do-not-merge to give a chance to react to the comments. Remove it at your own convenience.

It better reflects what the function does. Per the discussion in ceph#1100 (comment) and subsequent comments. Signed-off-by: John Mulligan <[email protected]>

It better reflects what the function does. Per the discussion in #1100 (comment) and subsequent comments. Signed-off-by: John Mulligan <[email protected]>

phlogistonjohn force-pushed the jjm-admin-smb branch from 266c1dd to 5d2b819 Compare April 7, 2025 20:03

phlogistonjohn changed the title ~~[DNM] Add a new smb admin API sub-package~~ [DNM][post-v033] Add a new smb admin API sub-package Apr 7, 2025

phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Apr 8, 2025

phlogistonjohn force-pushed the jjm-admin-smb branch from 96c6345 to 0561b58 Compare April 15, 2025 22:48

phlogistonjohn changed the title ~~[DNM][post-v033] Add a new smb admin API sub-package~~ [DNM] Add a new smb admin API sub-package Apr 16, 2025

phlogistonjohn force-pushed the jjm-admin-smb branch from 0561b58 to 4f395da Compare April 30, 2025 14:15

phlogistonjohn marked this pull request as ready for review April 30, 2025 17:53

phlogistonjohn requested review from anoopcs9 and ansiwen April 30, 2025 17:53

phlogistonjohn changed the title ~~[DNM] Add a new smb admin API sub-package~~ Add a new smb admin API sub-package Apr 30, 2025

anoopcs9 requested changes May 5, 2025

View reviewed changes

phlogistonjohn force-pushed the jjm-admin-smb branch from 8585c6b to 7f33e31 Compare May 5, 2025 15:25

anoopcs9 requested changes May 7, 2025

View reviewed changes

common/admin/smb/share.go Show resolved Hide resolved

common/admin/smb/share_test.go Outdated Show resolved Hide resolved

common/admin/smb/users_groups_test.go Outdated Show resolved Hide resolved

common/admin/smb/cluster_test.go Outdated Show resolved Hide resolved

phlogistonjohn force-pushed the jjm-admin-smb branch from 7f33e31 to b92b4b3 Compare May 7, 2025 15:11

anoopcs9 previously approved these changes May 7, 2025

View reviewed changes

anoopcs9 force-pushed the jjm-admin-smb branch from b92b4b3 to 42935b0 Compare May 18, 2025 08:47

mergify bot force-pushed the jjm-admin-smb branch from 42935b0 to 287e510 Compare May 28, 2025 08:49

commands: add new interfaces for mon,mgr commands with buffers

587ac2d

Part of the ceph command api supports commands with additional data in a buffer. Add interfaces that match these function calls in the go-ceph rados package. Signed-off-by: John Mulligan <[email protected]>

phlogistonjohn added 10 commits May 28, 2025 11:19

admin/smb: add cluster_test.go for cluster resource tests

0df0fc3

Signed-off-by: John Mulligan <[email protected]>

admin/smb: add join_auth_test.go for join auth tests

5d29189

Signed-off-by: John Mulligan <[email protected]>

admin/smb: add users_groups_test.go for users & groups tests

e367ce2

Signed-off-by: John Mulligan <[email protected]>

admin/smb: add share_test.go for share tests

c8f79eb

Signed-off-by: John Mulligan <[email protected]>

admin/smb: add result_test.go for result type tests

7851278

Signed-off-by: John Mulligan <[email protected]>

admin/smb: add resources_test.go for resource tests

e374ddb

Signed-off-by: John Mulligan <[email protected]>

admin/smb: add admin_test.go for functional tests

6e5ee2d

The admin_test.go module is the place where we actually test the smb module communicating with our go-ceph smb subpackage. Signed-off-by: John Mulligan <[email protected]>

admin/smb: expand the general smb package doc comment

01eab96

Add a small commentary on how the smb module is expected to be used. Signed-off-by: John Mulligan <[email protected]>

makefile: add smb package to list of test bins

93343a1

Signed-off-by: John Mulligan <[email protected]>

docs: generate api status data for admin/smb

f4ebd17

Signed-off-by: John Mulligan <[email protected]>

phlogistonjohn force-pushed the jjm-admin-smb branch from 287e510 to f4ebd17 Compare May 28, 2025 15:19

revive.toml: skip package name checks

d7e38da

Signed-off-by: John Mulligan <[email protected]>

anoopcs9 approved these changes May 28, 2025

View reviewed changes

This was referenced May 30, 2025

ci: fix meaningless package name check #1120

Closed

rbd: make Image.Write() and Image.WriteAt() propagate errors #1125

Merged

ansiwen reviewed Jun 4, 2025

View reviewed changes

ansiwen added the do-not-merge label Jun 4, 2025

ansiwen approved these changes Jun 4, 2025

View reviewed changes

phlogistonjohn removed the do-not-merge label Jun 4, 2025

mergify bot merged commit a1fcb9c into ceph:master Jun 4, 2025
14 of 16 checks passed

phlogistonjohn deleted the jjm-admin-smb branch June 4, 2025 21:27

phlogistonjohn mentioned this pull request Jun 4, 2025

admin/smb: rename errorMerge to errorPick #1126

Merged

5 tasks

mergify bot pushed a commit that referenced this pull request Jun 5, 2025

admin/smb: rename errorMerge to errorPick

c705072

It better reflects what the function does. Per the discussion in #1100 (comment) and subsequent comments. Signed-off-by: John Mulligan <[email protected]>

Add a new smb admin API sub-package #1100

Add a new smb admin API sub-package #1100

Uh oh!

Conversation

phlogistonjohn commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

phlogistonjohn commented Apr 16, 2025

Uh oh!

phlogistonjohn commented Apr 30, 2025

Uh oh!

anoopcs9 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phlogistonjohn commented May 5, 2025

Uh oh!

anoopcs9 commented May 5, 2025

Uh oh!

phlogistonjohn commented May 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phlogistonjohn commented May 7, 2025

Uh oh!

anoopcs9 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented May 17, 2025

Uh oh!

anoopcs9 commented May 18, 2025

Uh oh!

mergify bot commented May 18, 2025

✅ Branch has been successfully rebased

Uh oh!

mergify bot commented May 28, 2025

Uh oh!

anoopcs9 commented May 28, 2025

Uh oh!

mergify bot commented May 28, 2025

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

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

phlogistonjohn commented Apr 7, 2025 •

edited

Loading