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

feat: controller changes for Side Inputs support #866

Merged
merged 22 commits into from
Jul 18, 2023
Merged

Conversation

whynowy
Copy link
Member

@whynowy whynowy commented Jul 16, 2023

Be able to orchestrate a pipeline spec like following:

apiVersion: numaflow.numaproj.io/v1alpha1
kind: Pipeline
metadata:
  name: simple-pipeline
spec:
  sideInputs:
    - name: s1
      container:
        image: my-s1-image
      trigger:
        schedule: "@every 300s"
    - name: s2
      container:
        image: my-s2-image
      trigger:
        interval: "@every 300s"
  vertices:
    - name: in
      source:
        http: {}
    - name: my-udf
       sideInputs:
         - s1
         - s2
      udf:
        container:
          image: my-image
    - name: out
      sink:
        # A simple log printing sink
        log: {}
  edges:
    - from: in
      to: my-udf
    - from: my-udf
      to: out

For a pipeline like above, following new resources will be created:

  • Side Inputs data store in JetStream.
  • Two Side Input Manager Deployments for s1 and s2, each of them contains 1 init container to check if the store is ready, and 2 containers.
  • A new init container on the my-udf vertex pod, which is supposed to load the data from data store.
  • A sidecar container to watch the data store changes.

Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
.
Signed-off-by: Derek Wang <whynowy@gmail.com>
.
Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
.
Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
.
Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
@whynowy whynowy marked this pull request as ready for review July 17, 2023 06:42
@whynowy whynowy requested a review from vigith as a code owner July 17, 2023 06:42
@whynowy whynowy requested a review from a team July 17, 2023 06:42
@@ -46,4 +46,7 @@ func init() {
rootCmd.AddCommand(NewServerCommand())
rootCmd.AddCommand(NewServerInitCommand())
rootCmd.AddCommand(NewWebhookCommand())
rootCmd.AddCommand(NewSideInputsInitCommand())
rootCmd.AddCommand(NewSideInputManagerCommand())
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep either use SideInput or SideInputs, but not both. WDYT?

Signed-off-by: Derek Wang <whynowy@gmail.com>
}

func (p Pipeline) GetSideInputsStoreName() string {
return p.Name
Copy link
Member

Choose a reason for hiding this comment

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

Are we setting SideInputs store name the same as pipeline name? Maybe it worth documenting the rationale behind.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just a unique name in a namespace.

@@ -393,6 +402,81 @@ func (r *pipelineReconciler) findExistingVertices(ctx context.Context, pl *dfv1.
return result, nil
}

// Create or update Side Input Mapager deployments
func (r *pipelineReconciler) createOrUpdateSIMDeployments(ctx context.Context, pl *dfv1.Pipeline, isbSvcConfig dfv1.BufferServiceConfig) error {
Copy link
Member

Choose a reason for hiding this comment

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

Shall we unit test it?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
.
Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
@vigith
Copy link
Member

vigith commented Jul 17, 2023

  • Please make the mount point readonly for UDF
  • Prefix namespace name to KV bucket name

Signed-off-by: Derek Wang <whynowy@gmail.com>
x
Signed-off-by: Derek Wang <whynowy@gmail.com>
@whynowy
Copy link
Member Author

whynowy commented Jul 17, 2023

  • Please make the mount point readonly for UDF
  • Prefix namespace name to KV bucket name

done

@whynowy whynowy merged commit 6d14998 into numaproj:main Jul 18, 2023
15 checks passed
@whynowy whynowy deleted the sp branch July 18, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants