feat: Introduce new MultiContainerRun function to run pods with two containers #3095

hairyhum · 2024-09-06T21:18:19Z

Change Overview

In order to separate data dump generation by database tools and export by kando, a pod performing database dump can have two separate containers and set up the pipe over file instead of anonymous pipe.

The goal of this function is to enable database-specific blueprints to use official database images separately from kanister-tools image instead of building a custom image for each databse blueprint.

Pull request type

Please check the type of change your PR introduces:

Issues

fixes #issue-number

Test Plan

💪 Manual
⚡ Unit test
💚 E2E

hairyhum · 2024-09-06T21:19:21Z

There is an example in tests and docs.
Another example of blueprint:

apiVersion: cr.kanister.io/v1alpha1
kind: Blueprint
metadata:
  name: kube-task-parallel-bp
  namespace: kanister
actions:
  dump:
    phases:
    - func: MultiContainerRun
      name: dump
      args:
        namespace: kanister
        sharedVolumeMedium: Memory
        sharedVolumeSizeLimit: 1Gi
        sharedVolumeDir: "/tmp/"
        backgroundImage: ubuntu:latest
        backgroundCommand:
        - bash
        - -o
        - errexit
        - -o
        - pipefail
        - -c
        - |
          mkfifo /tmp/pipe-file
          for i in {1..10}
          do
            echo $i
            sleep 1
          done > /tmp/pipe-file
        outputImage: ghcr.io/kanisterio/kanister-tools:v9.99.9-dev
        outputCommand:
        - bash
        - -o
        - errexit
        - -o
        - pipefail
        - -c
        - |
          while [ ! -e /tmp/pipe-file  ]; do sleep 1; done
          kando output numbers "$(cat /tmp/pipe-file)"

docs/functions.rst

viveksinghggits · 2024-09-09T16:33:38Z

Hi @hairyhum ,
Are we doing this to solve any performance issue that were reported? Can you please share some background just for my understanding.

hairyhum · 2024-09-09T17:13:53Z

Hi @hairyhum , Are we doing this to solve any performance issue that were reported? Can you please share some background just for my understanding.

The main goal of this change is to separate image management for databse dump blueprints. Not the performance.

hairyhum · 2024-09-11T16:10:17Z

Renamed function into KubeTaskParallel and the arguments for containers to background and output containers.

e-sumin

Overall LGTM, but for me function name looks a bit misleading.
But, frankly, I can't propose better name. Maybe other reviewers will have an ideas ?

pkg/function/kube_task_parallel.go

e-sumin

LGTM

viveksinghggits

looks good to me overall. just left some nit comments/formatting suggestion.

viveksinghggits · 2024-09-25T10:08:42Z

docs/functions.rst

@@ -150,6 +150,82 @@ Example:
        - |
          echo "Example"

+KubeTaskParallel


I think maybe we should the name of the function in next community call? It's good right now, but by reading the name it looks like in this function we are doing things parallelly that kubetask does sequentially, which doesn't seem to be the case right?
I think it doesn't necessarily have to have KubeTask in its name right? Maybe MultiContainerUpload or something in that line?

I was trying to avoid the "purpose" style names like "dump", "upload" or "export". While this function is used for that purpose it's deliberately made generic.

ok, let's take opinions from other folks as well. @pavannd1 @PrasadG193 @mlavi .

Somehow KubeTaskParallel doesn't sound good to me. I agree that it would be better if we avoid KubeTask* prefix for the new function.

We can use MultiContainerPod or MultiContainerRun, but we already use KubeTaslk instead of Pod in the other function.

I like MultiContainerRun. Do we need to change the arguments name as well? that are currently named background and ouptut I think.

Naming is hard. Okay with MultiContainerRun or maybe MultiContainerJob

I don't have better names than background and output for containers.

I was thinking about the use of the named pipe - one of the containers pushes data while the other one receives it.
Here are some options I could think of:

input and output

producer and consumer

writer and reader

I was thinking about the use of the named pipe - one of the containers pushes data while the other one receives it.

That is not a part of the function itself, but a part of the command. The only practical difference in the implementation of those containers is that only one of them sends its ouput into the function output. Otherwise you can make the output container wirte data to a file and you can make the background container read from the file.

docs/functions.rst

viveksinghggits · 2024-09-25T10:17:43Z

docs/functions.rst

+   `backgroundImage`, Yes, `string`, image to be used in "background" container
+   `backgroundCommand`, Yes, `[]string`,  command list to execute in "background" container


should we call this container snapshot or backup container? or maybe data container? just a suggestion, if you think background is ok, I am good.

viveksinghggits · 2024-09-25T10:18:00Z

docs/functions.rst

+   `outputImage`, Yes, `string`, image to be used in "output" container
+   `outputCommand`, Yes, `[]string`,  command list to execute in "output" container


should this be called upload container?

Same note as a note before, trying to name things for what they do rather than what they can be used for.

releasenotes/notes/kube-task-parallel-function-d488516c0f3b22c6.yaml

hairyhum · 2024-10-02T20:37:25Z

Renamed function to `MultiContainerRun'

…ntainers In order to separate data dump generation by database tools and export by `kando`, a pod performing database dump can have two separate containers and set up the pipe over file instead of anonymous pipe

Init container can be used to set up data in the shared volume.

hairyhum force-pushed the dump-function branch from 31be024 to 32d48b2 Compare September 6, 2024 21:33

hairyhum requested review from mlavi, pavannd1 and viveksinghggits September 6, 2024 21:34

hairyhum force-pushed the dump-function branch 2 times, most recently from 7067c1e to ed6143f Compare September 9, 2024 15:56

viveksinghggits reviewed Sep 9, 2024

View reviewed changes

docs/functions.rst Outdated Show resolved Hide resolved

hairyhum force-pushed the dump-function branch from ed6143f to 8314e4a Compare September 9, 2024 17:11

hairyhum changed the title ~~feat: Introduce new Dump function to run pods with two containers~~ feat: Introduce new KubeTaskParallel function to run pods with two containers Sep 11, 2024

hairyhum force-pushed the dump-function branch from 0a792a0 to 8dbc0e2 Compare September 11, 2024 16:09

hairyhum force-pushed the dump-function branch 2 times, most recently from 514f8b2 to fdc0a1b Compare September 11, 2024 16:11

e-sumin reviewed Sep 13, 2024

View reviewed changes

pkg/function/kube_task_parallel.go Outdated Show resolved Hide resolved

hairyhum force-pushed the dump-function branch from dda5d4d to f3b2e13 Compare September 17, 2024 21:18

hairyhum requested review from e-sumin and viveksinghggits September 18, 2024 19:58

e-sumin approved these changes Sep 25, 2024

View reviewed changes

viveksinghggits reviewed Sep 25, 2024

View reviewed changes

hairyhum force-pushed the dump-function branch from 6bcc499 to 19c31e8 Compare September 25, 2024 19:37

hairyhum requested a review from viveksinghggits September 26, 2024 19:22

hairyhum changed the title ~~feat: Introduce new KubeTaskParallel function to run pods with two containers~~ feat: Introduce new MultiContainerRun function to run pods with two containers Oct 2, 2024

hairyhum force-pushed the dump-function branch from a383df6 to bf7017d Compare October 3, 2024 15:04

hairyhum added 3 commits October 3, 2024 11:05

Code cleanup

14c4e1a

Added init container support.

64113fa

Init container can be used to set up data in the shared volume.

hairyhum added 4 commits October 3, 2024 11:05

Fix docs warnings

7fd8be4

Wording

40a73b8

Linter fixes

61d1267

Rename KubeTaskParallel to MultiContainerRun

5938e47

hairyhum force-pushed the dump-function branch from bf7017d to 5938e47 Compare October 3, 2024 15:05

hairyhum added the kueue label Oct 3, 2024

mergify bot merged commit 7723a4a into master Oct 3, 2024
18 checks passed

mergify bot deleted the dump-function branch October 3, 2024 19:14

github-actions bot mentioned this pull request Oct 18, 2024

pre-release: Update version to 0.112.0 #3188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Introduce new MultiContainerRun function to run pods with two containers #3095

feat: Introduce new MultiContainerRun function to run pods with two containers #3095

hairyhum commented Sep 6, 2024 •

edited

Loading

hairyhum commented Sep 6, 2024 •

edited

Loading

viveksinghggits commented Sep 9, 2024

hairyhum commented Sep 9, 2024

hairyhum commented Sep 11, 2024

e-sumin left a comment

e-sumin left a comment

viveksinghggits left a comment

viveksinghggits Sep 25, 2024 •

edited

Loading

hairyhum Sep 25, 2024

viveksinghggits Sep 26, 2024 •

edited

Loading

PrasadG193 Sep 27, 2024

hairyhum Sep 27, 2024

viveksinghggits Sep 30, 2024

pavannd1 Oct 1, 2024

hairyhum Oct 1, 2024

pavannd1 Oct 2, 2024 •

edited

Loading

hairyhum Oct 2, 2024

viveksinghggits Sep 25, 2024

viveksinghggits Sep 25, 2024

hairyhum Sep 25, 2024

hairyhum commented Oct 2, 2024

		`backgroundImage`, Yes, `string`, image to be used in "background" container
		`backgroundCommand`, Yes, `[]string`, command list to execute in "background" container

		`outputImage`, Yes, `string`, image to be used in "output" container
		`outputCommand`, Yes, `[]string`, command list to execute in "output" container

feat: Introduce new MultiContainerRun function to run pods with two containers #3095

feat: Introduce new MultiContainerRun function to run pods with two containers #3095

Conversation

hairyhum commented Sep 6, 2024 • edited Loading

Change Overview

Pull request type

Issues

Test Plan

hairyhum commented Sep 6, 2024 • edited Loading

viveksinghggits commented Sep 9, 2024

hairyhum commented Sep 9, 2024

hairyhum commented Sep 11, 2024

e-sumin left a comment

Choose a reason for hiding this comment

e-sumin left a comment

Choose a reason for hiding this comment

viveksinghggits left a comment

Choose a reason for hiding this comment

viveksinghggits Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viveksinghggits Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavannd1 Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hairyhum commented Oct 2, 2024

hairyhum commented Sep 6, 2024 •

edited

Loading

hairyhum commented Sep 6, 2024 •

edited

Loading

viveksinghggits Sep 25, 2024 •

edited

Loading

viveksinghggits Sep 26, 2024 •

edited

Loading

pavannd1 Oct 2, 2024 •

edited

Loading