From 9089091d08f86a3bb6b228fff4ffbdcb6ad413c3 Mon Sep 17 00:00:00 2001 From: vsoch Date: Wed, 23 Aug 2023 08:25:46 -0600 Subject: [PATCH] refactor go binding module/client to use struct Problem: we currently pass around a ctx to interact with all go binding module or client functions, and provide the developer user with context that we are using the clients via function names (e.g., ReapiCliMatchAllocate). Solution: we can save the context alongside a struct that is returned with XNew(), and then rename the functions to not include the extra information (e.g., cli.MatchAllocate() implies MatchAllocate for the ReapiClient{ctx:ctx} that it is created from. This also removes the defer from the Go bindings, which should not be needed as the CString without new is allocated to the stack. Signed-off-by: vsoch --- README.md | 8 +- .../bindings/go/src/fluxcli/reapi_cli.go | 96 +++++++++---------- .../bindings/go/src/fluxcli/reapi_cli_test.go | 4 +- .../go/src/fluxmodule/reapi_module.go | 44 +++++---- .../go/src/fluxmodule/reapi_module_test.go | 10 +- resource/reapi/bindings/go/src/test/main.go | 36 +++---- 6 files changed, 102 insertions(+), 96 deletions(-) diff --git a/README.md b/README.md index ee3bc499d..072892759 100644 --- a/README.md +++ b/README.md @@ -119,7 +119,7 @@ export WITH_GO=yes make ``` -To run just one test, you can sd into t +To run just one test, you can cd into t ```bash $ ./t9001-golang-basic.t @@ -129,6 +129,12 @@ ok 2 - match allocate 2 slots: 2 sockets: 5 cores 1 gpu 6 memory 1..2 ``` +To run full tests (more robust and mimics what happens in CI) you can do: + +```bash +make check +``` + ##### Flux Instance The examples below walk through exercising functioning flux-sched modules (i.e., diff --git a/resource/reapi/bindings/go/src/fluxcli/reapi_cli.go b/resource/reapi/bindings/go/src/fluxcli/reapi_cli.go index d46c32fff..e6a2c80ec 100644 --- a/resource/reapi/bindings/go/src/fluxcli/reapi_cli.go +++ b/resource/reapi/bindings/go/src/fluxcli/reapi_cli.go @@ -16,25 +16,25 @@ package fluxcli import "C" import ( "fmt" - "unsafe" ) type ( ReapiCtx C.struct_reapi_cli_ctx_t + + // ReapiClient is a flux resource API client + // it holds a context that is required for most interactinos + ReapiClient struct { + ctx *ReapiCtx + } ) // NewReapiCli creates a new resource API client // reapi_cli_ctx_t *reapi_cli_new (); -func NewReapiCli() *ReapiCtx { - return (*ReapiCtx)(C.reapi_cli_new()) +func NewReapiClient() *ReapiClient { + ctx := (*ReapiCtx)(C.reapi_cli_new()) + return &ReapiClient{ctx: ctx} } -// // ReapiCliGetNode retruns a resource node -// This function is currently not being used -// func ReapiCliGetNode(ctx *ReapiCtx) string { -// return C.GoString(C.reapi_cli_get_node((*C.struct_reapi_cli_ctx)(ctx))) -// } - // Given an integer return code, convert to go error // Also provide a meaningful string to the developer user func retvalToError(code int, message string) error { @@ -44,31 +44,33 @@ func retvalToError(code int, message string) error { return fmt.Errorf(message+" %d", code) } -// ReapiCliDestroy destroys a resource API context +// HasContext exposes the private ctx, telling the caller if it is set +func (cli *ReapiClient) HasContext() bool { + return cli.ctx != nil +} + +// Destroy destroys a resource API context // void reapi_cli_destroy (reapi_cli_ctx_t *ctx); -func ReapiCliDestroy(ctx *ReapiCtx) { - C.reapi_cli_destroy((*C.struct_reapi_cli_ctx)(ctx)) +func (cli *ReapiClient) Destroy() { + C.reapi_cli_destroy((*C.struct_reapi_cli_ctx)(cli.ctx)) } -// ReapiCliInit initializes a new resource API context +// InitContext initializes a new resource API context // int reapi_cli_initialize (reapi_cli_ctx_t *ctx, const char *jgf); -func ReapiCliInit(ctx *ReapiCtx, jgf string, options string) (err error) { +func (cli *ReapiClient) InitContext(jgf string, options string) (err error) { jobgraph := C.CString(jgf) - defer C.free(unsafe.Pointer(jobgraph)) - opts := C.CString(options) - defer C.free(unsafe.Pointer(opts)) fluxerr := (int)( C.reapi_cli_initialize( - (*C.struct_reapi_cli_ctx)(ctx), jobgraph, (opts), + (*C.struct_reapi_cli_ctx)(cli.ctx), jobgraph, (opts), ), ) return retvalToError(fluxerr, "issue initializing resource api client") } -// ReapiCliMatchAllocate matches a jobspec to the best resources, either +// MatchAllocate matches a jobspec to the best resources, either // allocating or reserved them. The best resources are determined by the // match policy. // @@ -94,18 +96,14 @@ func ReapiCliInit(ctx *ReapiCtx, jgf string, options string) (err error) { // const char *jobspec, const uint64_t jobid, // bool *reserved, // char **R, int64_t *at, double *ov); -func ReapiCliMatchAllocate( - ctx *ReapiCtx, +func (cli *ReapiClient) MatchAllocate( orelse_reserve bool, jobspec string, ) (reserved bool, allocated string, at int64, overhead float64, jobid uint64, err error) { var r = C.CString("") - defer C.free(unsafe.Pointer(r)) - spec := C.CString(jobspec) - defer C.free(unsafe.Pointer(spec)) - fluxerr := (int)(C.reapi_cli_match_allocate((*C.struct_reapi_cli_ctx)(ctx), + fluxerr := (int)(C.reapi_cli_match_allocate((*C.struct_reapi_cli_ctx)(cli.ctx), (C.bool)(orelse_reserve), spec, (*C.ulong)(&jobid), @@ -121,7 +119,7 @@ func ReapiCliMatchAllocate( } -// ReapiCliUpdateAllocate updates the resource state with R. +// UpdateAllocate updates the resource state with R. // // \param ctx reapi_cli_ctx_t context object // \param jobid jobid of the uint64_t type. @@ -137,14 +135,11 @@ func ReapiCliMatchAllocate( // // const uint64_t jobid, const char *R, int64_t *at, // double *ov, const char **R_out); -func ReapiCliUpdateAllocate(ctx *ReapiCtx, jobid int, r string) (at int64, overhead float64, r_out string, err error) { +func (cli *ReapiClient) UpdateAllocate(jobid int, r string) (at int64, overhead float64, r_out string, err error) { var tmp_rout = C.CString("") - defer C.free(unsafe.Pointer(tmp_rout)) - var resource = C.CString(r) - defer C.free(unsafe.Pointer(resource)) - fluxerr := (int)(C.reapi_cli_update_allocate((*C.struct_reapi_cli_ctx)(ctx), + fluxerr := (int)(C.reapi_cli_update_allocate((*C.struct_reapi_cli_ctx)(cli.ctx), (C.ulong)(jobid), resource, (*C.long)(&at), @@ -157,7 +152,7 @@ func ReapiCliUpdateAllocate(ctx *ReapiCtx, jobid int, r string) (at int64, overh return at, overhead, r_out, err } -// ReapiCliCancel cancels the allocation or reservation corresponding to jobid. +// Cancel cancels the allocation or reservation corresponding to jobid. // // \param ctx reapi_cli_ctx_t context object // \param jobid jobid of the uint64_t type. @@ -167,14 +162,14 @@ func ReapiCliUpdateAllocate(ctx *ReapiCtx, jobid int, r string) (at int64, overh // int reapi_cli_cancel (reapi_cli_ctx_t *ctx, // // const uint64_t jobid, bool noent_ok); -func ReapiCliCancel(ctx *ReapiCtx, jobid int64, noent_ok bool) (err error) { - fluxerr := (int)(C.reapi_cli_cancel((*C.struct_reapi_cli_ctx)(ctx), +func (cli *ReapiClient) Cancel(jobid int64, noent_ok bool) (err error) { + fluxerr := (int)(C.reapi_cli_cancel((*C.struct_reapi_cli_ctx)(cli.ctx), (C.ulong)(jobid), (C.bool)(noent_ok))) return retvalToError(fluxerr, "issue resource api client cancel") } -// ReapiCliInfo gets the information on the allocation or reservation corresponding +// Info gets the information on the allocation or reservation corresponding // // to jobid. // \param ctx reapi_cli_ctx_t context object @@ -191,11 +186,10 @@ func ReapiCliCancel(ctx *ReapiCtx, jobid int64, noent_ok bool) (err error) { // int reapi_cli_info (reapi_cli_ctx_t *ctx, const uint64_t jobid, // // bool *reserved, int64_t *at, double *ov); -func ReapiCliInfo(ctx *ReapiCtx, jobid int64) (reserved bool, at int64, overhead float64, mode string, err error) { +func (cli *ReapiClient) Info(jobid int64) (reserved bool, at int64, overhead float64, mode string, err error) { var tmp_mode = C.CString("") - defer C.free(unsafe.Pointer(tmp_mode)) - fluxerr := (int)(C.reapi_cli_info((*C.struct_reapi_cli_ctx)(ctx), + fluxerr := (int)(C.reapi_cli_info((*C.struct_reapi_cli_ctx)(cli.ctx), (C.ulong)(jobid), (&tmp_mode), (*C.bool)(&reserved), @@ -206,7 +200,7 @@ func ReapiCliInfo(ctx *ReapiCtx, jobid int64) (reserved bool, at int64, overhead return reserved, at, overhead, C.GoString(tmp_mode), err } -// ReapiCliStat gets the performance information about the resource infrastructure. +// Stat gets the performance information about the resource infrastructure. // // \param ctx reapi_cli_ctx_t context object // \param V Number of resource vertices @@ -222,9 +216,9 @@ func ReapiCliInfo(ctx *ReapiCtx, jobid int64) (reserved bool, at int64, overhead // // int64_t *J, double *load, // double *min, double *max, double *avg); -func ReapiCliStat(ctx *ReapiCtx) (v int64, e int64, +func (cli *ReapiClient) Stat() (v int64, e int64, jobs int64, load float64, min float64, max float64, avg float64, err error) { - fluxerr := (int)(C.reapi_cli_stat((*C.struct_reapi_cli_ctx)(ctx), + fluxerr := (int)(C.reapi_cli_stat((*C.struct_reapi_cli_ctx)(cli.ctx), (*C.long)(&v), (*C.long)(&e), (*C.long)(&jobs), @@ -237,18 +231,18 @@ func ReapiCliStat(ctx *ReapiCtx) (v int64, e int64, return v, e, jobs, load, min, max, avg, err } -// ReapiCliGetErrMsg returns a string error message from the resource api -func ReapiCliGetErrMsg(ctx *ReapiCtx) string { - errmsg := C.reapi_cli_get_err_msg((*C.struct_reapi_cli_ctx)(ctx)) +// GetErrMsg returns a string error message from the resource api +func (cli *ReapiClient) GetErrMsg() string { + errmsg := C.reapi_cli_get_err_msg((*C.struct_reapi_cli_ctx)(cli.ctx)) return C.GoString(errmsg) } -// ReapiCliGetErrMsg clears error messages -func ReapiCliClearErrMsg(ctx *ReapiCtx) { - C.reapi_cli_clear_err_msg((*C.struct_reapi_cli_ctx)(ctx)) +// ClearErrMsg clears error messages +func (cli *ReapiClient) ClearErrMsg() { + C.reapi_cli_clear_err_msg((*C.struct_reapi_cli_ctx)(cli.ctx)) } -// ReapiCliSetHandleSet emulates setting the opaque handle to the reapi cli context. +// SetHandleSet emulates setting the opaque handle to the reapi cli context. // \param ctx reapi_cli_ctx_t context object // \param h Opaque handle. How it is used is an implementation // @@ -258,16 +252,16 @@ func ReapiCliClearErrMsg(ctx *ReapiCtx) { // \return 0 on success; -1 on error. // // int reapi_cli_set_handle (reapi_cli_ctx_t *ctx, void *handle); -func ReapiCliSetHandle(ctx *ReapiCtx) int { +func SetHandle() int { return -1 } -// ReapiCliGetHandle emulates setting the opaque handle to the reapi cli context. +// GetHandle emulates setting the opaque handle to the reapi cli context. // // \param ctx reapi_cli_ctx_t context object // \return handle // // void *reapi_cli_get_handle (reapi_cli_ctx_t *ctx); -func ReapiCliGetHandle(ctx *ReapiCtx) int { +func GetHandle() int { return -1 } diff --git a/resource/reapi/bindings/go/src/fluxcli/reapi_cli_test.go b/resource/reapi/bindings/go/src/fluxcli/reapi_cli_test.go index 4dc5d5d5f..7bf8fa3c6 100644 --- a/resource/reapi/bindings/go/src/fluxcli/reapi_cli_test.go +++ b/resource/reapi/bindings/go/src/fluxcli/reapi_cli_test.go @@ -13,8 +13,8 @@ package fluxcli import "testing" func TestFluxcli(t *testing.T) { - ctx := NewReapiCli() - if ctx == nil { + cli := NewReapiClient() + if !cli.HasContext() { t.Errorf("Context is null") } diff --git a/resource/reapi/bindings/go/src/fluxmodule/reapi_module.go b/resource/reapi/bindings/go/src/fluxmodule/reapi_module.go index b956d3d27..668ae2a4e 100644 --- a/resource/reapi/bindings/go/src/fluxmodule/reapi_module.go +++ b/resource/reapi/bindings/go/src/fluxmodule/reapi_module.go @@ -18,13 +18,22 @@ import ( ) type ( - ReapiCtx C.struct_reapi_module_ctx_t + ReapiCtx C.struct_reapi_module_ctx_t + ReapiModule struct { + ctx ReapiCtx + } ) // NewReapiModule creates a new resource API module // reapi_module_ctx_t *reapi_module_new (); -func NewReapiModule() *ReapiCtx { - return (*ReapiCtx)(C.reapi_module_new()) +func NewReapiModule() *ReapiModule { + ctx := (*ReapiCtx)(C.reapi_module_new()) + return &ReapiModule{ctx: ctx} +} + +// HasContext exposes the private ctx, telling the caller if it is set +func (m *ReapiModule) HasContext() bool { + return m.ctx != nil } // Given an integer return code, convert to go error @@ -36,30 +45,27 @@ func retvalToError(code int, message string) error { return fmt.Errorf(message+" %d", code) } -// ReapiModuleDestroy destroys the resource API context +// Destroy destroys the resource API context // void reapi_module_destroy (reapi_module_ctx_t *ctx); -func ReapiModuleDestroy(ctx *ReapiCtx) { - C.reapi_module_destroy((*C.struct_reapi_module_ctx)(ctx)) +func (m *ReapiModule) Destroy() { + C.reapi_module_destroy((*C.struct_reapi_module_ctx)(m.ctx)) } -// ReapiModuleMatchAllocate matches and allocates resources +// MatchAllocate matches and allocates resources // int reapi_module_match_allocate (reapi_module_ctx_t *ctx, bool orelse_reserve, // at: is the scheduled time "at" -func ReapiModuleMatchAllocate( - ctx *ReapiCtx, +func (m *ReapiModule) MatchAllocate( orelse_reserve bool, jobspec string, jobid int, ) (reserved bool, allocated string, at int64, overhead float64, err error) { // var atlong C.long = (C.long)(at) var r = C.CString("teststring") - defer C.free(unsafe.Pointer(r)) // Jobspec as a CString spec := C.CString(jobspec) - defer C.free(unsafe.Pointer(spec)) - fluxerr := (int)(C.reapi_module_match_allocate((*C.struct_reapi_module_ctx)(ctx), + fluxerr := (int)(C.reapi_module_match_allocate((*C.struct_reapi_module_ctx)(m.ctx), (C.bool)(orelse_reserve), spec, (C.ulong)(jobid), @@ -74,7 +80,7 @@ func ReapiModuleMatchAllocate( return reserved, allocated, at, overhead, err } -// ReapiModuleInfo gets the information on the allocation or reservation corresponding +// Info gets the information on the allocation or reservation corresponding // to jobid. // // \param ctx reapi_module_ctx_t context object @@ -91,8 +97,8 @@ func ReapiModuleMatchAllocate( // int reapi_module_info (reapi_module_ctx_t *ctx, const uint64_t jobid, // // bool *reserved, int64_t *at, double *ov); -func ReapiModuleInfo(ctx *ReapiCtx, jobid int64) (reserved bool, at int64, overhead float64, err error) { - fluxerr := (int)(C.reapi_module_info((*C.struct_reapi_module_ctx)(ctx), +func (m *ReapiModule) Info(ctx *ReapiCtx, jobid int64) (reserved bool, at int64, overhead float64, err error) { + fluxerr := (int)(C.reapi_module_info((*C.struct_reapi_module_ctx)(m.ctx), (C.ulong)(jobid), (*C.bool)(&reserved), (*C.long)(&at), @@ -102,7 +108,7 @@ func ReapiModuleInfo(ctx *ReapiCtx, jobid int64) (reserved bool, at int64, overh return reserved, at, overhead, err } -// ReapiModuleCancel cancels the allocation or reservation corresponding to jobid. +// Cancel cancels the allocation or reservation corresponding to jobid. // // \param ctx reapi_module_ctx_t context object // \param jobid jobid of the uint64_t type. @@ -112,9 +118,9 @@ func ReapiModuleInfo(ctx *ReapiCtx, jobid int64) (reserved bool, at int64, overh // int reapi_module_cancel (reapi_module_ctx_t *ctx, // // const uint64_t jobid, bool noent_ok);*/ -func ReapiModuleCancel(ctx *ReapiCtx, jobid int64, noent_ok bool) (err error) { - fluxerr := (int)(C.reapi_module_cancel((*C.struct_reapi_module_ctx)(ctx), +func (m *ReapiModule) Cancel(jobid int64, noent_ok bool) (err error) { + fluxerr := (int)(C.reapi_module_cancel((*C.struct_reapi_module_ctx)(m.ctx), (C.ulong)(jobid), (C.bool)(noent_ok))) return retvalToError(fluxerr, "issue with cancel") -} +} \ No newline at end of file diff --git a/resource/reapi/bindings/go/src/fluxmodule/reapi_module_test.go b/resource/reapi/bindings/go/src/fluxmodule/reapi_module_test.go index 65ddde534..7177d7fd1 100644 --- a/resource/reapi/bindings/go/src/fluxmodule/reapi_module_test.go +++ b/resource/reapi/bindings/go/src/fluxmodule/reapi_module_test.go @@ -12,17 +12,17 @@ package fluxmodule import ( "fmt" - "io/ioutil" + "os" "testing" ) func TestFluxmodule(t *testing.T) { - ctx := NewReapiModule() - jobspec, err := ioutil.ReadFile("/root/flux-sched/t/data/resource/jobspecs/basics/test001.yaml") - if ctx == nil { + mod := NewReapiModule() + jobspec, err := os.ReadFile("/root/flux-sched/t/data/resource/jobspecs/basics/test001.yaml") + if !mod.HasContext() { t.Errorf("Context is null") } - reserved, allocated, at, overhead, err1 := ReapiModuleMatchAllocate(ctx, false, string(jobspec), 4) + reserved, allocated, at, overhead, err1 := mod.MatchAllocate(false, string(jobspec), 4) fmt.Printf("%t, %s, %d, %f, %d, %s", reserved, allocated, at, overhead, err1, err) } diff --git a/resource/reapi/bindings/go/src/test/main.go b/resource/reapi/bindings/go/src/test/main.go index e5436a535..5c5706524 100644 --- a/resource/reapi/bindings/go/src/test/main.go +++ b/resource/reapi/bindings/go/src/test/main.go @@ -14,66 +14,66 @@ import ( "flag" "fluxcli" "fmt" - "io/ioutil" + "os" ) func main() { - ctx := fluxcli.NewReapiCli() jgfPtr := flag.String("jgf", "", "path to jgf") jobspecPtr := flag.String("jobspec", "", "path to jobspec") reserve := flag.Bool("reserve", false, "or else reserve?") flag.Parse() - jgf, err := ioutil.ReadFile(*jgfPtr) + jgf, err := os.ReadFile(*jgfPtr) if err != nil { fmt.Println("Error reading JGF file") return } - err = fluxcli.ReapiCliInit(ctx, string(jgf), "{}") + cli := fluxcli.NewReapiClient() + err = cli.InitContext(string(jgf), "{}") if err != nil { - fmt.Printf("Error init ReapiCli: %v\n", err) + fmt.Printf("Error initializing jobspec context for ReapiClient: %v\n", err) return } - fmt.Printf("Errors so far: %s\n", fluxcli.ReapiCliGetErrMsg(ctx)) + fmt.Printf("Errors so far: %s\n", cli.GetErrMsg()) - jobspec, err := ioutil.ReadFile(*jobspecPtr) + jobspec, err := os.ReadFile(*jobspecPtr) if err != nil { fmt.Printf("Error reading jobspec file: %v\n", err) return } fmt.Printf("Jobspec:\n %s\n", jobspec) - reserved, allocated, at, overhead, jobid, err := fluxcli.ReapiCliMatchAllocate(ctx, *reserve, string(jobspec)) + reserved, allocated, at, overhead, jobid, err := cli.MatchAllocate(*reserve, string(jobspec)) if err != nil { - fmt.Printf("Error in ReapiCliMatchAllocate: %v\n", err) + fmt.Printf("Error in ReapiClient MatchAllocate: %v\n", err) return } printOutput(reserved, allocated, at, jobid, err) - reserved, allocated, at, overhead, jobid, err = fluxcli.ReapiCliMatchAllocate(ctx, *reserve, string(jobspec)) - fmt.Println("Errors so far: \n", fluxcli.ReapiCliGetErrMsg(ctx)) + reserved, allocated, at, overhead, jobid, err = cli.MatchAllocate(*reserve, string(jobspec)) + fmt.Println("Errors so far: \n", cli.GetErrMsg()) if err != nil { - fmt.Printf("Error in ReapiCliMatchAllocate: %v\n", err) + fmt.Printf("Error in ReapiClient MatchAllocate: %v\n", err) return } printOutput(reserved, allocated, at, jobid, err) - err = fluxcli.ReapiCliCancel(ctx, 1, false) + err = cli.Cancel(1, false) if err != nil { - fmt.Printf("Error in ReapiCliCancel: %v\n", err) + fmt.Printf("Error in ReapiClient Cancel: %v\n", err) return } fmt.Printf("Cancel output: %v\n", err) - reserved, at, overhead, mode, err := fluxcli.ReapiCliInfo(ctx, 1) + reserved, at, overhead, mode, err := cli.Info(1) if err != nil { - fmt.Printf("Error in ReapiCliInfo: %v\n", err) + fmt.Printf("Error in ReapiClient Info: %v\n", err) return } fmt.Printf("Info output jobid 1: %t, %d, %f, %s, %v\n", reserved, at, overhead, mode, err) - reserved, at, overhead, mode, err = fluxcli.ReapiCliInfo(ctx, 2) + reserved, at, overhead, mode, err = cli.Info(2) if err != nil { - fmt.Println("Error in ReapiCliInfo: %v\n", err) + fmt.Println("Error in ReapiClient Info: %v\n", err) return } fmt.Printf("Info output jobid 2: %t, %d, %f, %v\n", reserved, at, overhead, err)