feat: add space_guarantee_requested field to create_lun - #181
Merged
Conversation
dbtinsley
requested review from
Hardikl,
cgrinds and
rahulguptajss
as code owners
July 13, 2026 16:58
|
Thank you for your interest in contributing to the ontap-mcp project! We require contributors to sign our Corporate contributor license agreement (CCLA), and we don"t have the user(s) @dbtinsley on file. In order for us to review and merge your code, please follow the instructions in step 6 of creating a pull request. |
There was a problem hiding this comment.
Pull request overview
Adds a typed input for requesting space-guaranteed (thick-provisioned) LUNs at creation time, and refactors REST mutation handling to properly support ONTAP’s 202 Accepted async job flow.
Changes:
- Add
space_guarantee_requestedto thecreate_luntool input and propagate it into the ONTAP REST body (space.guarantee.requested). - Extend ONTAP LUN models with
LUNSpaceGuaranteenested underLUNSpace. - Refactor
rest.Client.handleJobusage to capture response bodies and correctly poll async 202 jobs; add unit tests around job polling behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tool/tool.go | Adds SpaceGuaranteeRequested to the typed LUNCreate input schema. |
| ontap/ontap.go | Introduces LUNSpaceGuarantee and nests it into LUNSpace for REST serialization. |
| server/lun.go | Wires the new tool field into the outgoing ONTAP LUN create payload. |
| server/lun_typed_fields_test.go | Adds coverage for LUN create JSON mapping (needs an additional false/omit case). |
| rest/client.go | Refactors handleJob to distinguish sync vs async responses and adds jobPollInterval for test control. |
| rest/lun.go | Captures response body and routes LUN create through handleJob for async 202 support. |
| rest/lunmap.go | Captures response body and routes LUN map create through handleJob. |
| rest/nvme.go | Captures response body and routes NVMe namespace/subsystem-map creates through handleJob. |
| rest/igroup.go | Captures response body and routes igroup create through handleJob. |
| rest/qospolicy.go | Routes QoS create/delete through handleJob with response body capture. |
| rest/job_wait_test.go | Adds unit tests for handleJob and validates async polling across scoped mutation helpers (currently contains a Go compile error). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+10
to
+31
| func TestCreateLUNMapsRequestedSpaceGuarantee(t *testing.T) { | ||
| got, err := newCreateLUN(tool.LUNCreate{ | ||
| SVM: "genio", | ||
| Volume: "genio-data", | ||
| Name: "genio-lun", | ||
| Size: "1GB", | ||
| OsType: "linux", | ||
| SpaceGuaranteeRequested: true, | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("newCreateLUN() error = %v", err) | ||
| } | ||
|
|
||
| body, err := json.Marshal(got) | ||
| if err != nil { | ||
| t.Fatalf("json.Marshal() error = %v", err) | ||
| } | ||
| want := `{"svm":{"name":"genio"},"name":"/vol/genio-data/genio-lun","space":{"size":1073741824,"guarantee":{"requested":true}},"os_type":"linux"}` | ||
| if string(body) != want { | ||
| t.Errorf("LUN create body = %s, want %s", body, want) | ||
| } | ||
| } |
| if req.Method != http.MethodGet { | ||
| t.Fatalf("job poll method = %s, want GET", req.Method) | ||
| } | ||
| *jobCalls++ |
dbtinsley
added a commit
to dbtinsley/ontap-mcp
that referenced
this pull request
Jul 13, 2026
Asserts that when SpaceGuaranteeRequested is false (the zero value), the serialized LUN create body does not include the guarantee field, preserving default thin-provisioning behavior for existing callers. Addresses review comment on NetApp#181. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@cla-bot check Note for the ontap-mcp team: @dbtinsley is a NetApp employee contributing this work as part of a NetApp-sponsored project (NetApp CPOC). We believe the internal employment agreement covers IP assignment and that the external CCLA process does not apply, but please let us know if there is an internal process we should follow instead. |
|
Thank you for your interest in contributing to the ontap-mcp project! We require contributors to sign our Corporate contributor license agreement (CCLA), and we don"t have the user(s) @dbtinsley on file. In order for us to review and merge your code, please follow the instructions in step 6 of creating a pull request. |
|
The cla-bot has been summoned, and re-checked this pull request! |
| if req.Method != http.MethodGet { | ||
| t.Fatalf("job poll method = %s, want GET", req.Method) | ||
| } | ||
| *jobCalls++ |
Hardikl
reviewed
Jul 15, 2026
| Name string `json:"lun_name" jsonschema:"LUN name"` | ||
| Size string `json:"size" jsonschema:"size of the LUN (e.g., '10GB', '1TB')"` | ||
| OsType string `json:"os_type" jsonschema:"OS type (e.g., linux, windows, windows_2008, windows_gpt, aix, esxi, hyper_v, solaris, vmware, xen)"` | ||
| SpaceGuaranteeRequested bool `json:"space_guarantee_requested,omitzero" jsonschema:"request space guarantee for the LUN"` |
| out.SVM = ontap.NameAndUUID{Name: in.SVM} | ||
| out.Name = lunPath(in.Volume, in.Name) | ||
| out.Space = ontap.LUNSpace{Size: size} | ||
| out.Space = ontap.LUNSpace{ |
Contributor
There was a problem hiding this comment.
First validate the guarantee.requested exist and then assign.
| @@ -0,0 +1,54 @@ | |||
| package server | |||
Contributor
There was a problem hiding this comment.
remove this file and add/update tests in this file here: https://github.com/NetApp/ontap-mcp/blob/main/integration/test/lun_test.go
| return jsonResponse(req, http.StatusOK, `{"state":"success"}`), nil | ||
| }) | ||
|
|
||
| err := client.handleJob(context.Background(), http.StatusCreated, *bytes.NewBufferString(`{"uuid":"volume-1"}`)) |
Contributor
There was a problem hiding this comment.
If this file have test case for handleJob function, then it's not required. API related tests also not required as object level tests are added in this folder : https://github.com/NetApp/ontap-mcp/tree/main/integration/test
Enables thick LUN provisioning by exposing ONTAP's space.guarantee.requested field through the create_lun tool: tool struct changes: - LUNCreate gains SpaceGuaranteeRequested bool with JSON tag space.guarantee.requested (ONTAP dot-path notation) and omitzero so the field is omitted from the REST body when false (thin provisioning default) ontap/ontap.go: - LUNSpaceGuarantee.Requested changed from bool to *bool so zero value is omitted via omitzero rather than serialized as false server/lun.go: - newCreateLUN only populates the Guarantee struct when SpaceGuaranteeRequested is true, avoiding an unwanted false in the REST body for thin-provisioned LUNs Also includes rest.Client.handleJob fix (201 vs 202 polling) shared across the typed-field PR set. integration/test/lun_test.go: two new prompt test cases covering thick-provisioned LUN create and cleanup; standalone server/lun_typed_fields_test.go and rest/job_wait_test.go removed per maintainer guidance Co-authored-by: Cursor <cursoragent@cursor.com>
dbtinsley
force-pushed
the
cpoc/feat/lun-space-guarantee
branch
from
July 15, 2026 16:15
fd6ec4c to
f3694b3
Compare
Contributor
Author
|
Round 2 update — rebased on main (post-#157), addressed all maintainer and Copilot feedback: Shared changes across all four PRs:
PR-specific changes:
|
| Name string `json:"lun_name" jsonschema:"LUN name"` | ||
| Size string `json:"size" jsonschema:"size of the LUN (e.g., '10GB', '1TB')"` | ||
| OsType string `json:"os_type" jsonschema:"OS type (e.g., linux, windows, windows_2008, windows_gpt, aix, esxi, hyper_v, solaris, vmware, xen)"` | ||
| SpaceGuaranteeRequested bool `json:"space.guarantee.requested,omitzero" jsonschema:"set to true to request thick provisioning (space guarantee) for the LUN"` |
| @@ -41,28 +42,32 @@ type credentials struct { | |||
| } | |||
|
|
|||
| func (c *Client) handleJob(ctx context.Context, statusCode int, buf bytes.Buffer) error { | |||
Comment on lines
+65
to
+70
| { | ||
| name: "Create thick-provisioned LUN", | ||
| input: ClusterStr + "create a 10MB thick-provisioned lun named " + rn("lundocthick") + " with space guarantee in volume " + rn("doc") + " on the " + rn("marketing") + " svm with os type linux", | ||
| expectedOntapErr: "", | ||
| verifyAPI: ontapVerifier{api: "api/storage/luns?name=/vol/" + rn("doc") + "/" + rn("lundocthick") + "&svm.name=" + rn("marketing"), validationFunc: createObject}, | ||
| }, |
Contributor
There was a problem hiding this comment.
This needs to be resolved.
…files - Change handleJob signature to accept *bytes.Buffer to avoid copying a potentially large buffer by value; update all call sites in rest/ - Fix import grouping in rest/lun.go and rest/nvme.go to separate stdlib from third-party packages (goimports convention) Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
Round 3 update — lun-space-guarantee (#181)Changes pushed in this update ( REST / infra (same as other PRs)
No server or tool logic changes on this branch — the only open feedback was the formatting/infra items also shared with the other three PRs. |
Comment on lines
234
to
+236
| type LUNSpace struct { | ||
| Size int64 `json:"size,omitempty" jsonschema:"size of the LUN"` | ||
| Size int64 `json:"size,omitempty" jsonschema:"size of the LUN"` | ||
| Guarantee LUNSpaceGuarantee `json:"guarantee,omitzero"` |
| Name string `json:"lun_name" jsonschema:"LUN name"` | ||
| Size string `json:"size" jsonschema:"size of the LUN (e.g., '10GB', '1TB')"` | ||
| OsType string `json:"os_type" jsonschema:"OS type (e.g., linux, windows, windows_2008, windows_gpt, aix, esxi, hyper_v, solaris, vmware, xen)"` | ||
| SpaceGuaranteeRequested bool `json:"space.guarantee.requested,omitzero" jsonschema:"set to true to request thick provisioning (space guarantee) for the LUN"` |
The tool/tool.go input structs represent the MCP tool schema visible to callers; they use flat underscore names (volume_name, lun_name, size) for consistency and clarity. The ONTAP REST body mapping (space.guarantee.requested) is handled separately in the ontap.LUNSpaceGuarantee struct. Using a dotted key in the tool input schema was inconsistent and potentially surprising. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
Round 4 update — lun-space-guarantee (#181)Changes pushed in this update ( Fixed
Stale / already addressed in round 3
|
Comment on lines
+44
to
+50
| func (c *Client) handleJob(ctx context.Context, statusCode int, buf *bytes.Buffer) error { | ||
| if err := c.checkStatus(statusCode); err != nil { | ||
| return err | ||
| } | ||
| if statusCode != http.StatusAccepted { | ||
| return nil | ||
| } |
Comment on lines
+65
to
+70
| { | ||
| name: "Create thick-provisioned LUN", | ||
| input: ClusterStr + "create a 10MB thick-provisioned lun named " + rn("lundocthick") + " with space guarantee in volume " + rn("doc") + " on the " + rn("marketing") + " svm with os type linux", | ||
| expectedOntapErr: "", | ||
| verifyAPI: ontapVerifier{api: "api/storage/luns?name=/vol/" + rn("doc") + "/" + rn("lundocthick") + "&svm.name=" + rn("marketing"), validationFunc: createObject}, | ||
| }, |
Contributor
Author
Round 5 update — lun-space-guarantee (#181)No code changes in this round. Addressing the two new comments: Pre-existing / out of scope
Acknowledged limitation
|
Add verifyLUNSpaceGuarantee — modelled on the existing verifyDNSConfig / verifyQoSAdaptiveFields pattern — which GETs the LUN with fields=space.guarantee and asserts that space.guarantee.requested matches the expected value. Wire it into the "Create thick-provisioned LUN" test case so the test now verifies the guarantee flag was actually set on the created LUN, not just that the LUN exists. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
Update — lun-space-guarantee (#181)Fixed in commit Added The "Create thick-provisioned LUN" test case now verifies that |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
rest/client.go:60
- The PR description calls out new/updated tests for the
handleJobrefactor (e.g.rest/job_wait_test.go), but no such test file appears to be present. Given the behavior change around 201 vs 202 handling and job UUID parsing, this refactor should be covered by unit tests (at least: 201 returns immediately, 202 polls, and missing/invalid UUID yields a clear error).
func (c *Client) handleJob(ctx context.Context, statusCode int, buf *bytes.Buffer) error {
if err := c.checkStatus(statusCode); err != nil {
return err
}
if statusCode != http.StatusAccepted {
return nil
}
var pj ontap.PostJob
if err := json.Unmarshal(buf.Bytes(), &pj); err != nil {
return fmt.Errorf("failed to decode async job response: %w", err)
}
if strings.TrimSpace(pj.Job.UUID) == "" {
return fmt.Errorf("async job response is missing job UUID")
}
return c.waitForJob(ctx, `/api/cluster/jobs/`+pj.Job.UUID, 3*time.Minute)
Comment on lines
172
to
+176
| out.Space = ontap.LUNSpace{Size: size} | ||
| if in.SpaceGuaranteeRequested { | ||
| t := true | ||
| out.Space.Guarantee = ontap.LUNSpaceGuarantee{Requested: &t} | ||
| } |
rahulguptajss
approved these changes
Jul 20, 2026
TestNewCreateLUN_SpaceGuarantee covers the two key behaviors of the SpaceGuaranteeRequested field: - false (default): Space.Guarantee.Requested remains nil so the field is omitted from the ONTAP REST body, preserving thin provisioning - true: Space.Guarantee.Requested is set to &true for thick provisioning Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
Update — lun-space-guarantee (#181)Two changes pushed in commit Added: server-side unit test (
|
Hardikl
reviewed
Jul 21, 2026
Hardikl
left a comment
Hardikl
left a comment
Contributor
There was a problem hiding this comment.
File .worktrees/genio-typed-fields may not required.
Comment on lines
+9
to
+15
| func TestNewCreateLUN_SpaceGuarantee(t *testing.T) { | ||
| base := tool.LUNCreate{ | ||
| SVM: "vs1", | ||
| Volume: "vol1", | ||
| Name: "lun1", | ||
| Size: "10GB", | ||
| OsType: "linux", |
Contributor
There was a problem hiding this comment.
This test may not be needed to validate server files.
Server-side unit test coverage is not required here; the integration test in integration/test/lun_test.go with verifyLUNSpaceGuarantee provides end-to-end verification that space_guarantee_requested propagates correctly to space.guarantee.requested on the created LUN. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
Removed The field-level coverage for |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
server/lun.go:176
- PR description and file list mention a new
server/lun_typed_fields_test.gotyped-field propagation unit test, but this file is not present underserver/in this branch. Either add the referenced unit test (to match the description) or update the PR description/file list to reflect that coverage is currently provided via the integration test only.
if in.SpaceGuaranteeRequested {
t := true
out.Space.Guarantee = ontap.LUNSpaceGuarantee{Requested: &t}
}
rest/client.go:61
- PR description mentions
rest/job_wait_test.goupdates for thehandleJobrefactor, but there is nojob_wait_test.go(or any job-wait unit test) underrest/in this branch. Consider adding unit tests covering 201 vs 202 behavior (and the newjobPollIntervaloverride) or update the PR description to avoid pointing reviewers to a non-existent file.
func (c *Client) handleJob(ctx context.Context, statusCode int, buf *bytes.Buffer) error {
if err := c.checkStatus(statusCode); err != nil {
return err
}
if statusCode != http.StatusAccepted {
return nil
}
var pj ontap.PostJob
if err := json.Unmarshal(buf.Bytes(), &pj); err != nil {
return fmt.Errorf("failed to decode async job response: %w", err)
}
if strings.TrimSpace(pj.Job.UUID) == "" {
return fmt.Errorf("async job response is missing job UUID")
}
return c.waitForJob(ctx, `/api/cluster/jobs/`+pj.Job.UUID, 3*time.Minute)
}
Hardikl
approved these changes
Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #176
What this PR does
Adds
space_guarantee_requested booltoLUNCreateso callers can request thick provisioning (space.guarantee.requested=true) at LUN creation time. The field is omitted from the REST body when false, preserving the current default (thin-provisioned) behavior for existing callers.Without this field, all LUNs are created thin-provisioned regardless of the caller's intent and there is no way to request space-guaranteed LUNs through the typed MCP surface.
Supporting change:
rest.Client.handleJobrefactorIncludes the same
handleJobrefactor as PR #178, and additionally updatesCreateLUN,CreateLunMap,CreateNVMeNamespace, andCreateNVMeSubsystemMapto usehandleJobwith response body capture so they handle ONTAP 202 Accepted async responses correctly. Only needs to be merged once across the four companion PRs.Files changed
tool/tool.goSpaceGuaranteeRequested booltoLUNCreateontap/ontap.goLUNSpaceGuaranteestruct; extendLUNSpaceserver/lun.gonewCreateLUNserver/lun_typed_fields_test.gorest/client.go+rest/*.go+rest/job_wait_test.gohandleJobrefactor (same as #178)Raised by NetApp CPOC.
Made with Cursor