Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PUT /v1/transform/{name}/meta #18236

Merged
merged 12 commits into from
May 15, 2024

Conversation

oleiman
Copy link
Member

@oleiman oleiman commented May 3, 2024

Introduce an Admin API endpoint for patching transform metadata

  • update environment vars with PUT semantics
  • pause/resume transform

Also adds rpk experience for pause/resume (no env manipulation for now)

TODO:

  • Probably split admin endpoint into two
    • Went with a patch-like endpoint instead
  • rpk for pause/start
  • rpk for env mutation (PUNT)

Docs issue: https://github.com/redpanda-data/documentation-private/issues/2437

Closes CORE-1653

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

Improvements

  • Adds the ability to pause a deployed data transform w/o removing it from the system

@oleiman oleiman self-assigned this May 3, 2024
@github-actions github-actions bot added area/redpanda area/wasm WASM Data Transforms labels May 3, 2024
@oleiman
Copy link
Member Author

oleiman commented May 3, 2024

/dt

@rockwotj
Copy link
Contributor

rockwotj commented May 3, 2024

Probably split admin endpoint into two

An alternative is to have an endpoint like: PATCH /transform/{name} where you can patch the transform object. Parameters would include environment variables, is paused and maybe some others? @mo-redpanda this patch endpoint could support updating environment variables only for console based transforms.

src/v/model/transform.h Outdated Show resolved Hide resolved
src/v/redpanda/admin/api-doc/transform.json Outdated Show resolved Hide resolved
src/v/transform/api.cc Show resolved Hide resolved
src/v/transform/api.cc Outdated Show resolved Hide resolved
src/v/transform/api.cc Outdated Show resolved Hide resolved
src/v/transform/tests/transform_manager_test.cc Outdated Show resolved Hide resolved
src/v/transform/transform_manager.cc Show resolved Hide resolved
src/v/redpanda/admin/api-doc/transform.json Outdated Show resolved Hide resolved
@oleiman
Copy link
Member Author

oleiman commented May 3, 2024

Probably split admin endpoint into two

An alternative is to have an endpoint like: PATCH /transform/{name} where you can patch the transform object. Parameters would include environment variables, is paused and maybe some others? @mo-redpanda this patch endpoint could support updating environment variables only for console based transforms.

I like that idea. Unfortunately our seastar http mess doesn't support PATCH, so maybe PUT /transform/{name}/meta or something like that?

@oleiman
Copy link
Member Author

oleiman commented May 3, 2024

(or finally go in and see how much work it would be to add PATCH support!)

@rockwotj
Copy link
Contributor

rockwotj commented May 3, 2024

I am not a REST purest so fine with PUT

@oleiman oleiman force-pushed the xform/core-1653/pause-transform branch from 2d26643 to 5d87f40 Compare May 5, 2024 01:14
@oleiman oleiman changed the title Pause a transform PUT /v1/transform/{name}/meta May 5, 2024
@oleiman
Copy link
Member Author

oleiman commented May 5, 2024

force push contents:

  • patch metadata instead of pause outright (allows update of transform env)
  • comments, minor CR changes

@oleiman oleiman force-pushed the xform/core-1653/pause-transform branch from 5d87f40 to f5d4848 Compare May 5, 2024 03:40
@oleiman
Copy link
Member Author

oleiman commented May 5, 2024

force push fix swagger

@oleiman oleiman force-pushed the xform/core-1653/pause-transform branch from f5d4848 to 9072e0c Compare May 6, 2024 00:24
@oleiman
Copy link
Member Author

oleiman commented May 6, 2024

force push contents:

  • rpk transform pause
  • rpk transform start

Copy link
Contributor

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌶️


func newStartCommand(fs afero.Fs, p *config.Params) *cobra.Command {
cmd := &cobra.Command{
Use: "start [NAME]",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend creating an RFC and/or bringing this naming up with Mo + Rogger. Start makes me think I need to start transforms after deploying. Alternative name suggestion: resume (gcloud uses this terminology). Others to consider: unpause

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, waffled on this a bit and went with what's in the ticket as a placeholder...I'm partial to suspend/resume in this context, but RFC seems prudent.


func newPauseCommand(fs afero.Fs, p *config.Params) *cobra.Command {
cmd := &cobra.Command{
Use: "pause [NAME]",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively consider the naming gcloud uses: suspend. Not sure if that is more clear or not to see what's happening under the hood.

},
"transform_metadata_patch": {
"id": "transform_metadata_patch",
"description": "A partial update to transform metadata",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to document the semantics? I feel like patch requests are sometimes hard to understand. For example would be good to say things like:

  • environment map is overridden (so no deep patches)
  • If not present then it is unchanged

src/v/redpanda/admin/api-doc/transform.json Outdated Show resolved Hide resolved
src/v/model/transform.h Show resolved Hide resolved
@oleiman oleiman force-pushed the xform/core-1653/pause-transform branch from 9072e0c to 0572830 Compare May 6, 2024 22:55
@oleiman
Copy link
Member Author

oleiman commented May 6, 2024

force push contents:

  • various comments/route descriptions
  • pause/start -> suspend/resume (tentative, pending input from devex & product)

src/v/redpanda/admin/transform.cc Outdated Show resolved Hide resolved
tests/rptest/tests/data_transforms_test.py Outdated Show resolved Hide resolved
@oleiman oleiman force-pushed the xform/core-1653/pause-transform branch from 0572830 to eb29a27 Compare May 7, 2024 21:06
@oleiman
Copy link
Member Author

oleiman commented May 7, 2024

force push - rebase dev

@oleiman oleiman force-pushed the xform/core-1653/pause-transform branch from eb29a27 to 2f90747 Compare May 7, 2024 21:45
@oleiman
Copy link
Member Author

oleiman commented May 7, 2024

force push contents:

  • address CR comments (use new seastar path param function, add a patch env test case)
  • switch over from suspend/resume to pause/resume per discussion on slack and RFC

@oleiman oleiman marked this pull request as ready for review May 7, 2024 21:48
Also adds a couple of model types:

is_transform_paused (bool)

transform_metadata_patch
  - map<str, str> - Overwrite transform environment map (PUT semantics)
  - paused - Pause or unpause the transform

Also adds ::paused to transform_metadata stream operator

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
@oleiman oleiman force-pushed the xform/core-1653/pause-transform branch from fd1ff3e to 0074461 Compare May 10, 2024 21:01
@oleiman oleiman requested review from r-vasquez and pgellert May 10, 2024 21:02
r-vasquez
r-vasquez previously approved these changes May 10, 2024
Copy link
Contributor

@r-vasquez r-vasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rpk changes LGTM, thanks!

src/v/transform/api.cc Show resolved Hide resolved
src/v/redpanda/admin/api-doc/transform.json Outdated Show resolved Hide resolved
tests/rptest/clients/rpk.py Outdated Show resolved Hide resolved
oleiman added 4 commits May 13, 2024 11:36
- Paused transforms show `inactive` partitions in report
- Metadata updates with is_paused::yes prevents new processor creation

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Proxy for existing plugin_table method.

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Looks up a transform by name, modifies metadata accordingly with the
environment map and paused state in the request, and upserts.

includes a unit test for the pause update specifically, since it
affects processor state.

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
patch_transform_metadata

From request body reads:
  - A paused state (optional)
  - A list of environment_variables (optional)
    - If present, overwrites the environment map in current metadata

If neither field is present in the request body, return success but don't
bother propagating it to the cluster.

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
@oleiman oleiman force-pushed the xform/core-1653/pause-transform branch from 0074461 to b6f724d Compare May 13, 2024 20:20
@oleiman oleiman requested a review from andijcr May 13, 2024 20:21
@oleiman
Copy link
Member Author

oleiman commented May 13, 2024

force push contents:

  • fix typo in rpk.py and harden DataTransformsTest.test_patch
  • fix swagger formatting, commit history

andijcr
andijcr previously approved these changes May 13, 2024
@oleiman
Copy link
Member Author

oleiman commented May 14, 2024

CI Failure is test_raft_rpunit

oleiman added 7 commits May 14, 2024 21:23
Includes some code to parse a JSON request body into a transform_metadata_patch

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
PUT transform/{name}/meta

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Admin API returns environment variables as a list of objects,
but rpk is clever enough to parse those out into a map before
dumping json.

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
- calls /v1/transform/{name}/meta
- patchMetadataRequest body:
  - IsPaused *bool
  - Environment *[]EnvironmentVariable

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Pause and resume a transform via Admin API

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Includes both rpk and admin API usage.

NOTE: env patches are not yet supported by `rpk transform`

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
@oleiman oleiman force-pushed the xform/core-1653/pause-transform branch from b6f724d to e9fd450 Compare May 15, 2024 04:29
@oleiman oleiman requested a review from a team as a code owner May 15, 2024 04:29
@oleiman
Copy link
Member Author

oleiman commented May 15, 2024

force push adds json::validator for patch request

@oleiman oleiman merged commit d0eedf7 into redpanda-data:dev May 15, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants