DAOS-18304 ddb: Introduce DdbAPI interface and fix db_path propagation #18124
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,46 +37,67 @@ func freeString(s *C.char) { | |
| C.free(unsafe.Pointer(s)) | ||
| } | ||
|
|
||
| type DdbAPI interface { | ||
|
Contributor
There was a problem hiding this comment. Hmm. I understand the intent behind this interface, but giant predefined "god" interfaces is a well-known anti-pattern in Go. Per the community review guidelines, interfaces should be defined at the consumer site. This guidance is codified in the "Accept interfaces, return structs" proverb. While superficially the predefined interface may seem like the easier or more convenient path, it's a trap. It means that every implementation has to define all of those methods, leading to maintenance headaches and friction that results in good tests not being written. At a minimum, just rearchitecting the tests to define interfaces for the subset of For an alternative approach to doing Go/cgo testing, take a look at src/control/lib/daos/api. It's not perfect, but I think it achieves the goal of exposing as much of the Go surface for unit testing as possible. It also demonstrates a better design (IMO) than a single API object thing that has (at current count) 28 methods. I suggest a smaller, focused fix for the db_path problem and a follow up effort to rework this package along similar lines to the libdaos Go API. It's more work in the short term, but will yield long-term benefits.
Contributor
There was a problem hiding this comment. Agreed. I think the argument for going this PR's route is entirely as an intermediate step to increase testability in the short term. It is indeed technical debt that will need a thoughtful redesign in the future either way.
Contributor
Author
There was a problem hiding this comment. @mjmac thank you for pointing this out with a concrete alternative to follow. Following @mjmac's suggestion, I preferred fixing it now rather than carrying it as technical debt, |
||
| Init(log *logging.LeveledLogger) (func(), error) | ||
| PoolIsOpen() bool | ||
| Ls(path string, recursive bool, details bool) error | ||
| Open(path string, dbPath string, writeMode bool) error | ||
| Version() error | ||
| Close() error | ||
| SuperblockDump() error | ||
| ValueDump(path string, dst string) error | ||
| Rm(path string) error | ||
| ValueLoad(src string, dst string) error | ||
| IlogDump(path string) error | ||
| IlogCommit(path string) error | ||
| IlogClear(path string) error | ||
| DtxDump(path string, active bool, committed bool) error | ||
| DtxCmtClear(path string) error | ||
| SmdSync(nvmeConf string, dbPath string) error | ||
| VeaDump() error | ||
| VeaUpdate(offset string, blkCnt string) error | ||
| DtxActCommit(path string, dtxID string) error | ||
| DtxActAbort(path string, dtxID string) error | ||
| Feature(path, dbPath, enable, disable string, show bool) error | ||
| RmPool(path string, dbPath string) error | ||
| DtxActDiscardInvalid(path string, dtxID string) error | ||
| DevList(dbPath string) error | ||
| DevReplace(dbPath string, oldDevid string, newDevid string) error | ||
| DtxStat(path string, details bool) error | ||
| ProvMem(dbPath string, tmpfsMount string, tmpfsMountSize uint) error | ||
| DtxAggr(path string, cmtTime uint64, cmtDate string) error | ||
| } | ||
|
|
||
| // DdbContext structure for wrapping the C code context structure | ||
| type DdbContext struct { | ||
| ctx C.struct_ddb_ctx | ||
| log *logging.LeveledLogger | ||
| } | ||
|
|
||
| // Init initializes the ddb context and returns a closure to finalize it. | ||
| func (ctx *DdbContext) Init(log *logging.LeveledLogger) (func(), error) { | ||
| // Must lock to OS thread because vos init/fini uses ABT init and finalize which must be called on the same thread | ||
| runtime.LockOSThread() | ||
|
|
||
| if err := daosError(C.ddb_init()); err != nil { | ||
| runtime.UnlockOSThread() | ||
| return nil, err | ||
| } | ||
|
|
||
| C.ddb_ctx_init(&ctx.ctx) // Initialize with ctx default values | ||
| ctx.log = log | ||
|
|
||
| return func() { | ||
| C.ddb_fini() | ||
| runtime.UnlockOSThread() | ||
| }, nil | ||
| } | ||
|
|
||
| func (ctx *DdbContext) PoolIsOpen() bool { | ||
|
kjacque marked this conversation as resolved.
|
||
| return bool(C.ddb_pool_is_open(&ctx.ctx)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) Ls(path string, recursive bool, details bool) error { | ||
| /* Set up the options */ | ||
| options := C.struct_ls_options{} | ||
| options.path = C.CString(path) | ||
|
|
@@ -87,33 +108,34 @@ func ddbLs(ctx *DdbContext, path string, recursive bool, details bool) error { | |
| return daosError(C.ddb_run_ls(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) Open(path string, dbPath string, writeMode bool) error { | ||
| /* Set up the options */ | ||
| options := C.struct_open_options{} | ||
| options.path = C.CString(path) | ||
| defer freeString(options.path) | ||
| options.db_path = C.CString(dbPath) | ||
| defer freeString(options.db_path) | ||
| options.write_mode = C.bool(writeMode) | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_open(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) Version() error { | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_version(&ctx.ctx)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) Close() error { | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_close(&ctx.ctx)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) SuperblockDump() error { | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_superblock_dump(&ctx.ctx)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) ValueDump(path string, dst string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_value_dump_options{} | ||
| options.path = C.CString(path) | ||
|
|
@@ -124,7 +146,7 @@ func ddbValueDump(ctx *DdbContext, path string, dst string) error { | |
| return daosError(C.ddb_run_value_dump(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) Rm(path string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_rm_options{} | ||
| options.path = C.CString(path) | ||
|
|
@@ -133,7 +155,7 @@ func ddbRm(ctx *DdbContext, path string) error { | |
| return daosError(C.ddb_run_rm(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) ValueLoad(src string, dst string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_value_load_options{} | ||
| options.src = C.CString(src) | ||
|
|
@@ -144,7 +166,7 @@ func ddbValueLoad(ctx *DdbContext, src string, dst string) error { | |
| return daosError(C.ddb_run_value_load(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) IlogDump(path string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_ilog_dump_options{} | ||
| options.path = C.CString(path) | ||
|
|
@@ -153,7 +175,7 @@ func ddbIlogDump(ctx *DdbContext, path string) error { | |
| return daosError(C.ddb_run_ilog_dump(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) IlogCommit(path string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_ilog_commit_options{} | ||
| options.path = C.CString(path) | ||
|
|
@@ -162,7 +184,7 @@ func ddbIlogCommit(ctx *DdbContext, path string) error { | |
| return daosError(C.ddb_run_ilog_commit(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) IlogClear(path string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_ilog_clear_options{} | ||
| options.path = C.CString(path) | ||
|
|
@@ -171,7 +193,7 @@ func ddbIlogClear(ctx *DdbContext, path string) error { | |
| return daosError(C.ddb_run_ilog_clear(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) DtxDump(path string, active bool, committed bool) error { | ||
| /* Set up the options */ | ||
| options := C.struct_dtx_dump_options{} | ||
| options.path = C.CString(path) | ||
|
|
@@ -182,7 +204,7 @@ func ddbDtxDump(ctx *DdbContext, path string, active bool, committed bool) error | |
| return daosError(C.ddb_run_dtx_dump(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) DtxCmtClear(path string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_dtx_cmt_clear_options{} | ||
| options.path = C.CString(path) | ||
|
|
@@ -191,61 +213,62 @@ func ddbDtxCmtClear(ctx *DdbContext, path string) error { | |
| return daosError(C.ddb_run_dtx_cmt_clear(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) SmdSync(nvmeConf string, dbPath string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_smd_sync_options{} | ||
| options.nvme_conf = C.CString(nvmeConf) | ||
| defer freeString(options.nvme_conf) | ||
| options.db_path = C.CString(dbPath) | ||
| defer freeString(options.db_path) | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_smd_sync(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) VeaDump() error { | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_vea_dump(&ctx.ctx)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) VeaUpdate(offset string, blkCnt string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_vea_update_options{} | ||
| options.offset = C.CString(offset) | ||
| defer freeString(options.offset) | ||
| options.blk_cnt = C.CString(blkCnt) | ||
| defer freeString(options.blk_cnt) | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_vea_update(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) DtxActCommit(path string, dtxID string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_dtx_act_options{} | ||
| options.path = C.CString(path) | ||
| defer freeString(options.path) | ||
| options.dtx_id = C.CString(dtxID) | ||
| defer freeString(options.dtx_id) | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_dtx_act_commit(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) DtxActAbort(path string, dtxID string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_dtx_act_options{} | ||
| options.path = C.CString(path) | ||
| defer freeString(options.path) | ||
| options.dtx_id = C.CString(dtxID) | ||
| defer freeString(options.dtx_id) | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_dtx_act_abort(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) Feature(path, dbPath, enable, disable string, show bool) error { | ||
| /* Set up the options */ | ||
| options := C.struct_feature_options{} | ||
| options.path = C.CString(path) | ||
| defer freeString(options.path) | ||
| options.db_path = C.CString(dbPath) | ||
| defer freeString(options.db_path) | ||
| if enable != "" { | ||
| err := daosError(C.ddb_feature_string2flags(&ctx.ctx, C.CString(enable), | ||
| &options.set_compat_flags, &options.set_incompat_flags)) | ||
|
|
@@ -265,50 +288,51 @@ func ddbFeature(ctx *DdbContext, path, enable, disable string, show bool) error | |
| return daosError(C.ddb_run_feature(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) RmPool(path string, dbPath string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_rm_pool_options{} | ||
| options.path = C.CString(path) | ||
| defer freeString(options.path) | ||
| options.db_path = C.CString(dbPath) | ||
| defer freeString(options.db_path) | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_rm_pool(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) DtxActDiscardInvalid(path string, dtxID string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_dtx_act_options{} | ||
| options.path = C.CString(path) | ||
| defer freeString(options.path) | ||
| options.dtx_id = C.CString(dtxID) | ||
| defer freeString(options.dtx_id) | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_dtx_act_discard_invalid(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) DevList(dbPath string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_dev_list_options{} | ||
| options.db_path = C.CString(dbPath) | ||
| defer freeString(options.db_path) | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_dev_list(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) DevReplace(dbPath string, oldDevID string, newDevID string) error { | ||
| /* Set up the options */ | ||
| options := C.struct_dev_replace_options{} | ||
| options.db_path = C.CString(dbPath) | ||
| defer freeString(options.db_path) | ||
| options.old_devid = C.CString(oldDevID) | ||
| defer freeString(options.old_devid) | ||
| options.new_devid = C.CString(newDevID) | ||
| defer freeString(options.new_devid) | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_dev_replace(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) DtxStat(path string, details bool) error { | ||
| /* Set up the options */ | ||
| options := C.struct_dtx_stat_options{} | ||
| options.path = C.CString(path) | ||
|
|
@@ -318,25 +342,25 @@ func ddbDtxStat(ctx *DdbContext, path string, details bool) error { | |
| return daosError(C.ddb_run_dtx_stat(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) ProvMem(dbPath string, tmpfsMount string, tmpfsMountSize uint) error { | ||
| /* Set up the options */ | ||
| options := C.struct_prov_mem_options{} | ||
| options.db_path = C.CString(dbPath) | ||
| defer freeString(options.db_path) | ||
| options.tmpfs_mount = C.CString(tmpfsMount) | ||
| defer freeString(options.tmpfs_mount) | ||
|
|
||
| options.tmpfs_mount_size = C.uint(tmpfsMountSize) | ||
| /* Run the c code command */ | ||
| return daosError(C.ddb_run_prov_mem(&ctx.ctx, &options)) | ||
| } | ||
|
|
||
| func (ctx *DdbContext) DtxAggr(path string, cmtTime uint64, cmtDate string) error { | ||
| if cmtTime != math.MaxUint64 && cmtDate != "" { | ||
| ctx.log.Error("'--cmt_time' and '--cmt_date' options are mutually exclusive") | ||
| return daosError(-C.DER_INVAL) | ||
| } | ||
| if cmtTime == math.MaxUint64 && cmtDate == "" { | ||
| ctx.log.Error("'--cmt_time' or '--cmt_date' option has to be defined") | ||
| return daosError(-C.DER_INVAL) | ||
| } | ||
|
|
@@ -345,13 +369,13 @@ func ddbDtxAggr(ctx *DdbContext, path string, cmt_time uint64, cmt_date string) | |
| options := C.struct_dtx_aggr_options{} | ||
| options.path = C.CString(path) | ||
| defer freeString(options.path) | ||
| if cmtTime != math.MaxUint64 { | ||
| options.format = C.DDB_DTX_AGGR_CMT_TIME | ||
| options.cmt_time = C.uint64_t(cmtTime) | ||
| } | ||
| if cmtDate != "" { | ||
| options.format = C.DDB_DTX_AGGR_CMT_DATE | ||
| options.cmt_date = C.CString(cmtDate) | ||
| defer freeString(options.cmt_date) | ||
| } | ||
| /* Run the c code command */ | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.