Implement template rendering action #1364

lburgazzoli · 2024-11-11T11:42:51Z

Description

This PR introduces a new rendering engine that is based on the go text/template package.

The template is rendered to the actual objects taking a subset of the ReconciliationRequest struct: Instance and DSCI so as an example, the ServiceMeshMember template that is part of the model registry component would looks like:

apiVersion: maistra.io/v1
kind: ServiceMeshMember
metadata:
  name: default
  namespace: {{.Instance.Spec.RegistriesNamespace}}
spec:
  controlPlaneRef:
    namespace: {{ .DSCI.Spec.ServiceMesh.ControlPlane.Namespace }}
    name: {{ .DSCI.Spec.ServiceMesh.ControlPlane.Name }}

https://issues.redhat.com/browse/RHOAIENG-15534

How Has This Been Tested?

Enable the model registry component in DSC
Check for the presence of a ServiceMeshMember instance in the model registries namespace named default and that it contains the expected values
Check metrics related to the render action, it should include something like:

# HELP action_renderer_manifests_total Number of rendered resources
# TYPE action_renderer_manifests_total counter
action_renderer_manifests_total{controller="modelregistry",engine="kustomize"} 17
action_renderer_manifests_total{controller="modelregistry",engine="template"} 1

Screenshot or short clip

Merge criteria

You have read the contributors guide.
Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

codecov · 2024-11-11T12:53:17Z

Codecov Report

Attention: Patch coverage is 73.56322% with 23 lines in your changes missing coverage. Please review.

Please upload report for BASE (feature-operator-refactor@1209501). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...actions/render/template/action_render_templates.go	73.25%	17 Missing and 6 partials ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##             feature-operator-refactor    #1364   +/-   ##
============================================================
  Coverage                             ?   26.68%           
============================================================
  Files                                ?       55           
  Lines                                ?     4455           
  Branches                             ?        0           
============================================================
  Hits                                 ?     1189           
  Misses                               ?     3125           
  Partials                             ?      141

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

grdryn

/lgtm

pkg/controller/predicates/resources/resources.go

pkg/controller/actions/render/template/resources/smm.tmpl.yaml

pkg/controller/actions/render/template/action_render_templates.go

zdtsw · 2024-11-12T07:48:25Z

pkg/controller/actions/render/template/action_render_templates.go

+	var buffer bytes.Buffer
+
+	for i := range rr.Templates {
+		content, err := fs.ReadFile(rr.Templates[i].FS, rr.Templates[i].Path)


the pat only accept file name not the folder name, not for this PR, but we will need extend it a bit later if we have multiple templates

if you have multiple templates, the you can pass multiple paths, I didn't want to make it over complicated at this stage given we have only a few templates.

But yeah, it would probably be useful. I'll have a look for a follow up PR

pkg/controller/actions/render/template/action_render_templates.go

pkg/controller/reconciler/component_reconciler_support.go

pkg/controller/predicates/resources/resources.go

zdtsw

LGTM

lburgazzoli · 2024-11-12T11:22:06Z

/retest

lburgazzoli · 2024-11-12T12:36:10Z

pkg/controller/actions/render/template/resources/smm.tmpl.yaml

+  name: default
+  namespace: {{.DSCI.Spec.ApplicationsNamespace}}
+  annotations:
+    instance-name: {{.Component.Name}}


@zdtsw renamed Instance -> Component as it sounds better

lburgazzoli · 2024-11-12T12:36:16Z

pkg/controller/actions/render/template/resources/smm-data.tmpl.yaml

+  name: {{.SMM.Name}}
+  namespace: {{.DSCI.Spec.ApplicationsNamespace}}
+  annotations:
+    instance-name: {{.Component.Name}}


@zdtsw renamed Instance -> Component as it sounds better

lburgazzoli · 2024-11-12T14:07:54Z

/retest

lburgazzoli · 2024-11-12T15:57:46Z

/retest

grdryn

/lgtm

openshift-ci · 2024-11-12T23:05:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grdryn, zdtsw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [grdryn,zdtsw]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ykaliuta · 2025-01-20T15:01:45Z

pkg/controller/types/types.go

+	//
+	// and use the scheme as discriminator for the rendering engine
+	//
+	Templates []TemplateInfo


If I'm not mistaken, it looks like there is difference between Manifests and Templates. Manifests belong to ReconciliationRequest due to substitution via devFlags. But Templates are statically provided and the location can be configured on Action creation.

Not for every component, so as an example, for kserve, the inclusion of templates depends on some configuration options (serveless, servicemesh, etc)

Ok, I looked in more details and reconsidered.

Resources are like "context" for the pipeline, output for template or kustomize renderings and input for deploy.

With dynamic template data computation #1527 Templates serve the same for preparing configuration and input for template rendering action (so, putting them together as in comment does not sound needed).

Without that, like in existing proposal #1521, every config creation action can directly call template rendering to contribute to the Resources.

May be makes sense to put a comment before that fields that it's "Request processing Context". Well, again, if I'm not mistaken.

make sense, I'll add some more context

…flux/component-updates/odh-ml-pipelines-launcher-v2-17 chore(deps): update odh-ml-pipelines-launcher-v2-17 to 8afafa3

openshift-ci bot requested review from grdryn and ykaliuta November 11, 2024 11:43

lburgazzoli requested review from zdtsw and removed request for ykaliuta November 11, 2024 11:43

grdryn added the feature-operator-refactor label Nov 12, 2024

grdryn reviewed Nov 12, 2024

View reviewed changes

pkg/controller/predicates/resources/resources.go Outdated Show resolved Hide resolved

openshift-ci bot assigned grdryn Nov 12, 2024

openshift-ci bot added lgtm and removed lgtm labels Nov 12, 2024

lburgazzoli requested a review from grdryn November 12, 2024 06:51

zdtsw reviewed Nov 12, 2024

View reviewed changes

lburgazzoli requested a review from zdtsw November 12, 2024 09:13

zdtsw approved these changes Nov 12, 2024

View reviewed changes

openshift-ci bot assigned zdtsw Nov 12, 2024

openshift-ci bot added lgtm approved labels Nov 12, 2024

openshift-ci bot removed the lgtm label Nov 12, 2024

lburgazzoli commented Nov 12, 2024

View reviewed changes

zdtsw approved these changes Nov 12, 2024

View reviewed changes

openshift-ci bot added the lgtm label Nov 12, 2024

lburgazzoli added 4 commits November 12, 2024 17:02

Implement template rendering action

4f3639d

Update controllers to use template rendering action

46c3571

Fix findings

3a1a199

Fix findings (2)

771f563

openshift-ci bot removed the lgtm label Nov 12, 2024

lburgazzoli added 3 commits November 12, 2024 22:15

Rename Instance to Component for clarity

a7ca580

Fix findings (3)

1685f6a

Fix linter

45a5623

lburgazzoli force-pushed the RHOAIENG-15534-render-template branch from 4b28fa5 to 45a5623 Compare November 12, 2024 21:15

grdryn approved these changes Nov 12, 2024

View reviewed changes

openshift-ci bot added the lgtm label Nov 12, 2024

openshift-merge-bot bot merged commit acd89b8 into opendatahub-io:feature-operator-refactor Nov 12, 2024
8 checks passed

lburgazzoli deleted the RHOAIENG-15534-render-template branch November 13, 2024 06:44

This was referenced Dec 8, 2024

A reconciler should not require the DSCI to perform cleanup action to avoid deadlock #1428

Closed

Refactor DSC controller #1430

Closed

ykaliuta reviewed Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement template rendering action #1364

Implement template rendering action #1364

lburgazzoli commented Nov 11, 2024 •

edited

Loading

codecov bot commented Nov 11, 2024 •

edited

Loading

grdryn left a comment

zdtsw Nov 12, 2024

lburgazzoli Nov 12, 2024

zdtsw left a comment

lburgazzoli commented Nov 12, 2024

lburgazzoli Nov 12, 2024

lburgazzoli Nov 12, 2024

lburgazzoli commented Nov 12, 2024

lburgazzoli commented Nov 12, 2024

grdryn left a comment

openshift-ci bot commented Nov 12, 2024

ykaliuta Jan 20, 2025

lburgazzoli Jan 20, 2025

ykaliuta Jan 20, 2025

lburgazzoli Jan 20, 2025

Implement template rendering action #1364

Implement template rendering action #1364

Conversation

lburgazzoli commented Nov 11, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

codecov bot commented Nov 11, 2024 • edited Loading

Codecov Report

grdryn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdtsw left a comment

Choose a reason for hiding this comment

lburgazzoli commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lburgazzoli commented Nov 12, 2024

lburgazzoli commented Nov 12, 2024

grdryn left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 12, 2024

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

lburgazzoli commented Nov 11, 2024 •

edited

Loading

codecov bot commented Nov 11, 2024 •

edited

Loading