search

flux-framework / flux-core

core services for the Flux resource management framework
GNU Lesser General Public License v3.0
159 stars 49 forks source link

add output buffering to sdexec #6023

Closed garlick closed 4 weeks ago

garlick commented 1 month ago

This adds output buffering to sdexec.

I'll mark WIP temporarily while I ponder more tests.

chu11 commented 1 month ago

after initial skim, took awhile to grok the buffer + offset + used variables. Am trying to think of a way to make it grok a little easier. Dunno if a "class" would help? Like a get_data() function can return data and length, and internally it'll update all things it needs to update and hide that from channel.c?

garlick commented 1 month ago

That's a pretty good idea. I'll give that a whirl.

garlick commented 4 weeks ago

@chu11 - per your suggestion, I added an "outbuf" class to libsdexec. It's a tiny thing and purpose-built for use in the channel module so it really didn't feel right to flesh this out as a general purpose container. Let me know if it helps make the code more readable.

garlick commented 4 weeks ago

I also switched over flux-exec(1) to LOCAL_UNBUF, which should cause any line buffering issues introduced by this PR to cause stdout tests to fail in t2409-sdexec.t.

Dropping the WIP since I think this has pretty good test coverage now.

garlick commented 4 weeks ago

Thanks! I'll set MWP.

codecov[bot] commented 4 weeks ago

Codecov Report

Attention: Patch coverage is 90.08264% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 83.31%. Comparing base (8c0e8c0) to head (3438b64).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #6023 +/- ## =========================================== + Coverage 54.37% 83.31% +28.94% =========================================== Files 469 519 +50 Lines 75701 83617 +7916 =========================================== + Hits 41160 69666 +28506 + Misses 34541 13951 -20590 ``` | [Files](https://app.codecov.io/gh/flux-framework/flux-core/pull/6023?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flux-framework) | Coverage Δ | | |---|---|---| | [src/common/libsdexec/outbuf.c](https://app.codecov.io/gh/flux-framework/flux-core/pull/6023?src=pr&el=tree&filepath=src%2Fcommon%2Flibsdexec%2Foutbuf.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flux-framework#diff-c3JjL2NvbW1vbi9saWJzZGV4ZWMvb3V0YnVmLmM=) | `100.00% <100.00%> (ø)` | | | [src/cmd/flux-exec.c](https://app.codecov.io/gh/flux-framework/flux-core/pull/6023?src=pr&el=tree&filepath=src%2Fcmd%2Fflux-exec.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flux-framework#diff-c3JjL2NtZC9mbHV4LWV4ZWMuYw==) | `76.39% <50.00%> (+17.50%)` | :arrow_up: | | [src/common/libsdexec/channel.c](https://app.codecov.io/gh/flux-framework/flux-core/pull/6023?src=pr&el=tree&filepath=src%2Fcommon%2Flibsdexec%2Fchannel.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flux-framework#diff-c3JjL2NvbW1vbi9saWJzZGV4ZWMvY2hhbm5lbC5j) | `81.93% <96.42%> (+3.63%)` | :arrow_up: | | [src/modules/sdexec/sdexec.c](https://app.codecov.io/gh/flux-framework/flux-core/pull/6023?src=pr&el=tree&filepath=src%2Fmodules%2Fsdexec%2Fsdexec.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flux-framework#diff-c3JjL21vZHVsZXMvc2RleGVjL3NkZXhlYy5j) | `70.58% <74.19%> (+0.11%)` | :arrow_up: | ... and [437 files with indirect coverage changes](https://app.codecov.io/gh/flux-framework/flux-core/pull/6023/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flux-framework)