feat: controller-wide pipeline templates. Closes #305 #314

dseapy · 2022-11-04T12:44:38Z

Signed-off-by: David Seapy [email protected]

Closes #305.

Adds support for specifying controller-wide pipeline templates in the controller configmap.

Signed-off-by: David Seapy <[email protected]>

dseapy · 2022-11-04T12:58:42Z

pkg/apis/numaflow/v1alpha1/vertex_template.go

+
+type VertexTemplate struct {
+	// +optional
+	AbstractPodTemplate `json:",inline" protobuf:"bytes,1,opt,name=abstractPodTemplate"`


initially did inline to be consistent with daemon/job, but just looked back at issue and seems like maybe json:"podTemplate,omitempty" is desired to be consistent with containerTemplate/initContainerTemplate. let me know if i should make that change here & the docs.

podTemplate is better.

just to confirm, i should leave daemon and job unchanged (keep those inline)?

whynowy · 2022-11-04T18:39:14Z

pkg/reconciler/pipeline/controller.go

@@ -76,11 +77,16 @@ func (r *pipelineReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 	log := r.logger.With("namespace", pl.Namespace).With("pipeline", pl.Name)
 	plCopy := pl.DeepCopy()
 	ctx = logging.WithLogger(ctx, log)
+	if err := plCopy.Spec.Templates.UpdateWithDefaultsFrom(r.templates); err != nil {


nil check for plCopy.Spec.Templates

currently there is a check in the func & unit tests for it being nil, which is not typical but done to reduce code on the caller side.

if tpl == nil { *tpl = *defaultTemplate.DeepCopy() return nil }

should i move that bit out to the couple places the func is called?

Can also change func that takes 2 templates that doesn't modify either, but returns a separate one, and do nil checks on the caller side?

whynowy · 2022-11-04T18:41:00Z

pkg/apis/numaflow/v1alpha1/vertex_template.go

+
+type VertexTemplate struct {
+	// +optional
+	AbstractPodTemplate `json:",inline" protobuf:"bytes,1,opt,name=abstractPodTemplate"`


podTemplate is better.

whynowy · 2022-11-04T18:53:14Z

pkg/reconciler/pipeline/controller.go


 	result, reconcileErr := r.reconcile(ctx, plCopy)
 	if reconcileErr != nil {
 		log.Errorw("Reconcile error", zap.Error(reconcileErr))
 	}
+	plCopy.Spec.Templates = pl.Spec.Templates.DeepCopy()


Is it because plCopy.Spec.Templates gets changed in reconcile()? This would be confusing and tricky if that was the case. I would prefer nothing to get changed under .Spec.

templates do not get changed in reconcile(), but they do get changed from pl immediately after plCopy is created in Reconcile().

Mostly I was trying to find a good way to do the cluster-wide + pipeline-wide templates merge once & not need to pass down the merged templates through the func calls. However maybe each job, daemon, vertex component can handle their own merge to avoid changing it... will look at that.

docs/controller-configmap.md

docs/pipeline-customization.md

whynowy · 2022-11-04T19:02:09Z

docs/pipeline-customization.md

+
+## Vertex customization
+
+See `Component customization` described above.


Is there an place to mention that it can also be done in pipeline level?

I'm a bit confused, as this is for the pipeline level, do you mean modify this to mention it's for the pipeline-level? At the top of the page in the Pipeline Customization section I try to describe things can be done in the controller configmap and vertexes can be customized individually, but may not be doing a great job with the wording.

With this change, there are 3 places to customize the vertex template - configmap, pipeline spec.template.vertex, and pipeline.spec.vertices[*].xxx, I don't see the 2nd one is mentioned.

The "component customization" section in this file and the comment in the example attempt to describe that.

docs/pipeline-customization.md

whynowy · 2022-11-04T19:15:15Z

pkg/reconciler/pipeline_templates.go

+	dfv1 "github.com/numaproj/numaflow/pkg/apis/numaflow/v1alpha1"
+)
+
+func LoadPipelineTemplates() (*dfv1.Templates, error) {


Use the same way as LoadConfig to achieve hot reloading?

sure, i was struggling to get viper to unmarshal the yaml the way it does for config but I can still use viper to watch it but not unmarshal. or if we need viper to unmarshal, I can look at why viper was unmarshaling it to empty Templates.

i'm slightly concerned with thread-safety. does it make sense to add read/write locking?

Signed-off-by: David Seapy <[email protected]>

whynowy · 2022-11-06T17:02:11Z

I just thought a little bit more - is it too much for the templates customization (cluster level in the configmap, pipeline level and on the vertex)? Maybe pipeline level and vertex level support is good enough, and that makes the implementation simpler. WDYT? @vigith @dseapy

dseapy · 2022-11-07T09:48:15Z

I just thought a little bit more - is it too much for the templates customization (cluster level in the configmap, pipeline level and on the vertex)? Maybe pipeline level and vertex level support is good enough, and that makes the implementation simpler. WDYT? @vigith @dseapy

I'm happy to do whatever is desired, for my current use-case setting on controller and vertex is sufficient.

That said, I'm not sure how to leverage that for simpler code. At the moment, there is code for:

Loading from ConfigMap to a Templates struct
VertexTemplate struct and applying it to an AbstractVertex
Merging Templates struct with another one using StrategicMergePatch (cluster-wide merging with pipeline-wide)

For daemon & job we already need to do (1) and (3), and regardless of pipeline-level vertex customization support I think we need to do (2).

Maybe it's simpler because if changing approach to merging DaemonTemplate and JobTemplate individually (replacing UpdateWithDefaultsFrom which is merging the single Templates), then that would avoid needing to have a func for merging VertexTemplate?

whynowy · 2022-11-07T19:26:14Z

I just thought a little bit more - is it too much for the templates customization (cluster level in the configmap, pipeline level and on the vertex)? Maybe pipeline level and vertex level support is good enough, and that makes the implementation simpler. WDYT? @vigith @dseapy

I'm happy to do whatever is desired, for my current use-case setting on controller and vertex is sufficient.

That said, I'm not sure how to leverage that for simpler code. At the moment, there is code for:

Loading from ConfigMap to a Templates struct

VertexTemplate struct and applying it to an AbstractVertex

Merging Templates struct with another one using StrategicMergePatch (cluster-wide merging with pipeline-wide)

For daemon & job we already need to do (1) and (3), and regardless of pipeline-level vertex customization support I think we need to do (2).

Maybe it's simpler because if changing approach to merging DaemonTemplate and JobTemplate individually (replacing UpdateWithDefaultsFrom which is merging the single Templates), then that would avoid needing to have a func for merging VertexTemplate?

Ignore what I said, let's follow what we discussed before :), having 3 controls is valuable.

vigith · 2022-11-07T20:44:57Z

Ignore what I said, let's follow what we discussed before :), having 3 controls is valuable.

+1, we have some internal use cases where we will be sharing the controller and running multiple ML pipelines.

dseapy · 2022-11-10T11:31:53Z

closing for now, ref: #331 (comment)

tldr: as-written this doesn't resolve need for pipeline/vertex specific info (i.e. affinity/pdb label matching).

feat: controller-wide pipeline templates. Closes numaproj#305

219470a

Signed-off-by: David Seapy <[email protected]>

dseapy commented Nov 4, 2022

View reviewed changes

dseapy marked this pull request as ready for review November 4, 2022 12:59

dseapy requested review from whynowy and vigith as code owners November 4, 2022 12:59

whynowy reviewed Nov 4, 2022

View reviewed changes

doc updates, most comments not addressed yet

cbd450e

Signed-off-by: David Seapy <[email protected]>

dseapy mentioned this pull request Nov 10, 2022

Support controller-wide HA Daemon configuration #331

Open

dseapy closed this Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: controller-wide pipeline templates. Closes #305 #314

feat: controller-wide pipeline templates. Closes #305 #314

dseapy commented Nov 4, 2022

dseapy Nov 4, 2022

whynowy Nov 4, 2022

dseapy Nov 4, 2022

whynowy Nov 4, 2022

dseapy Nov 4, 2022

dseapy Nov 4, 2022

whynowy Nov 4, 2022

whynowy Nov 4, 2022

dseapy Nov 4, 2022

whynowy Nov 4, 2022

dseapy Nov 4, 2022

whynowy Nov 4, 2022

dseapy Nov 4, 2022

whynowy Nov 4, 2022

dseapy Nov 4, 2022

whynowy commented Nov 6, 2022

dseapy commented Nov 7, 2022 •

edited

Loading

whynowy commented Nov 7, 2022

vigith commented Nov 7, 2022

dseapy commented Nov 10, 2022


		## Vertex customization

		See `Component customization` described above.

feat: controller-wide pipeline templates. Closes #305 #314

feat: controller-wide pipeline templates. Closes #305 #314

Conversation

dseapy commented Nov 4, 2022

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whynowy commented Nov 6, 2022

dseapy commented Nov 7, 2022 • edited Loading

whynowy commented Nov 7, 2022

vigith commented Nov 7, 2022

dseapy commented Nov 10, 2022

dseapy commented Nov 7, 2022 •

edited

Loading