Process SM related templates in memory #15

cam-garrison · 2023-12-11T19:54:30Z

This PR changes our approach with templates to now process/store the templates in memory, rather than copying .tmpl files to /tmp/.... and then creating the .yaml files there as well.

This means that when creating a feature.Manifests(path), you no longer will include the "rootdir" or similar variable in the path. See PR changes for examples of this, since you point directly to the path in /templates.

Used image quay.io/cgarriso/opendatahub-operator:template-in-memory for testing, created DSCI with SM enabled as well as DSC with dashboard to ensure changes with Manifest(path) there still functioned as expected.

bartoszmajsak

Thanks for this improvement! I left a few suggestions about naming and small change on the way how we can structure builder so we allow other filesystems to be used easily.

When reviewing the code I found a bug in the original implementation too :)

There's also dead code I cannot comment directly on, which we could remove https://github.com/maistra/opendatahub-operator/pull/15/files#diff-897801bd6ac96d166eb19cd078cd2b46eb522fce98d8e3573f08473ca2661e70R119-R121

bartoszmajsak · 2023-12-12T10:56:03Z

pkg/feature/manifest.go

 }

-func loadManifestsFrom(path string) ([]manifest, error) {
+func loadManifestsFrom(embeddedFS embed.FS, rootPath string) ([]manifest, error) {


I think we could align it with walk fs.WalkDir expects. We do not need anything specific from embeddedFS to make this whole thing work, and this is a limitation we don't need.

Suggested change

func loadManifestsFrom(embeddedFS embed.FS, rootPath string) ([]manifest, error) {

func loadManifestsFrom(fsys fs.FS, rootPath string) ([]manifest, error) {

note: I can't apply suggestion on the whole func so you will need to make changes in the editor, not here.

bartoszmajsak · 2023-12-12T10:56:20Z

pkg/feature/manifest.go

@@ -62,28 +75,30 @@ func (m *manifest) targetPath() string {
 	return fmt.Sprintf("%s%s", m.path[:len(m.path)-len(filepath.Ext(m.path))], ".yaml")
 }

-func (m *manifest) processTemplate(data interface{}) error {
+func (m *manifest) processTemplate(fs embed.FS, data interface{}) error {


Same reasons as above

Suggested change

func (m *manifest) processTemplate(fs embed.FS, data interface{}) error {

func (m *manifest) processTemplate(fsys fs.FS, data interface{}) error {

bartoszmajsak · 2023-12-12T10:59:28Z

pkg/feature/manifest.go

+	m.processedContent = buffer.String()
+	m.processed = true


Perhaps we don't need m.processed anymore. Can't we assume that if m.processedContent == "" that means it was not a template?

8a2c375 agreed

pkg/feature/raw_resources.go

bartoszmajsak · 2023-12-12T11:09:27Z

pkg/feature/raw_resources.go

@@ -106,7 +97,7 @@ func (f *Feature) patchResourceFromFile(filename string) error {
 		}

 		// Convert the patch from YAML to JSON
-		patchAsJSON, err := yaml.YAMLToJSON(data)
+		patchAsJSON, err := yaml.YAMLToJSON([]byte(data))


I think there's a bug in here. We split the whole thing into chunks based on --- separate, but instead of patching resources one by one we apply the entire data (so all the resources all at once). This means that currently we only have single-resource files in use, that's why this bug never showed up.

We should either give up on the assumption there can be more than one resource per file OR fix it to apply one by one. I would vouch for fixing it. Can you take care of it in this PR?

bartoszmajsak · 2023-12-12T11:19:38Z

pkg/feature/builder.go

@@ -79,7 +79,7 @@ func (fb *featureBuilder) Manifests(paths ...string) *featureBuilder {
 		var manifests []manifest

 		for _, path := range paths {
-			manifests, err = loadManifestsFrom(path)
+			manifests, err = loadManifestsFrom(embeddedFiles, path)


I am not quite sure about this one from the pure API perspective. By hardcoding it to the global variable (//go:embed) we are locking it for any other sources like we made possible before.

What I would think about instead is to make fsys fs.FS a field on the featureBuilder struct, add extra method such as func (fb *featureBuilder) ManifestSource(fsys fs.FS) *featureBuilder which will allow us to set it (with godoc mentioning it's a root fs from which manifest paths are loaded). By default, in the Load() func, where we instantiate a Feature, we would set it to embeddedFiles and if ManifestSource is not called in the builder chain that would stay like that. Not a big change, but gives us flexibility. WDYT?

e072121 done here

bartoszmajsak · 2023-12-12T11:27:22Z

pkg/feature/feature.go

 			return errors.WithStack(err)
 		}

-		log.Info("converted template to manifest", "feature", f.Name, "path", m.targetPath())


I think as a follow-up PR should change log levels from info to debug in most places. It's too much meaningless output in the logs now just to see that stuff is happening

pkg/feature/raw_resources.go

bartoszmajsak

See my other comments, but I think I wasn't exactly clear on the suggestions. Here's what I had in mind:

func (fb *featureBuilder) Load() (*Feature, error) {
	feature := &Feature{
		Name:    fb.name,
		Enabled: true,
        fsys: embeddedFiles,
	}

+ suggestions below.

bartoszmajsak · 2023-12-14T09:01:14Z

pkg/feature/builder.go

+	fsys := fb.fsys
+	if fsys == nil {
+		fsys = embeddedFiles
+	}
+


I didn't mean it like this, sorry for misleading you with my original comment. I thought about using featureBuilder to populate this field in Feature instance eventually... let's blame it on the fact that I'm sick now.

Have a look at the follow-up suggestions.

Suggested change

fsys := fb.fsys

if fsys == nil {

fsys = embeddedFiles

}

pkg/feature/builder.go

pkg/feature/feature.go

bartoszmajsak · 2023-12-14T10:16:25Z

pkg/feature/feature.go

@@ -159,34 +159,34 @@ func (f *Feature) addCleanup(cleanupFuncs ...Action) {

 func (f *Feature) ApplyManifest(filename string) error {
 	m := loadManifestFrom(filename)
-	if err := m.processTemplate(f.Spec); err != nil {
+	if err := m.processTemplate(embeddedFiles, f.Spec); err != nil {


Suggested change

if err := m.processTemplate(embeddedFiles, f.Spec); err != nil {

if err := m.processTemplate(f.fsys, f.Spec); err != nil {

pkg/feature/builder.go

pkg/feature/feature.go

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

bartoszmajsak

Few minor suggestions + bunch of commits I couldn't make as suggestions, but I hope you agree with them :)

Great job!

tests/integration/servicemesh/test-templates/authorino/mesh-authz-ext-provider.patch.tmpl

tests/integration/servicemesh/service_mesh_setup_features_int_test.go

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

cam-garrison added 6 commits December 11, 2023 11:46

process in memory w timing

4f942c6

fix templates path for dash and kserve paths

cb3e484

remove timing logs

2b65946

remove unnec copy function

13d3675

remove unused path

426bf77

make lint

dfd58f0

cam-garrison requested a review from bartoszmajsak December 11, 2023 19:54

bartoszmajsak requested changes Dec 12, 2023

View reviewed changes

cam-garrison added 5 commits December 12, 2023 10:15

remove dead code, rename input var

b4dc2e3

rename func, use more generic fs

0807413

make fsys part of featbuilder struct

e072121

fix bug ignoring multiple resources in 1 file

26c0da8

remove m.processed bool

8a2c375

cam-garrison requested a review from bartoszmajsak December 13, 2023 21:47

bartoszmajsak requested changes Dec 14, 2023

View reviewed changes

cam-garrison and others added 8 commits December 14, 2023 11:25

add fsys to feature struct

a6e938b

batch commits, add f.fsys

aa0d9d1

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

set f.fsys in load()

27c54f9

add testing for non-default ManifestSource

c48fcfb

add copying file test

a9f7b65

fix: reworks test for DirFS

ab6d573

unfocus added tests

5ac9395

make consts, satisfy linter

0f0d110

cam-garrison requested a review from bartoszmajsak December 18, 2023 21:04

bartoszmajsak added 2 commits December 19, 2023 12:00

chore: minor code improvements

0f1b79a

chore(test): allows to copy multiplepaths

00c597d

bartoszmajsak approved these changes Dec 19, 2023

View reviewed changes

batch improvements to tests

e3b12a8

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

cam-garrison merged commit edf0ebc into maistra:service-mesh-integration Dec 19, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process SM related templates in memory #15

Process SM related templates in memory #15

cam-garrison commented Dec 11, 2023

bartoszmajsak left a comment

bartoszmajsak Dec 12, 2023 •

edited

Loading

bartoszmajsak Dec 12, 2023

bartoszmajsak Dec 12, 2023

cam-garrison Dec 13, 2023

bartoszmajsak Dec 12, 2023

cam-garrison Dec 13, 2023

bartoszmajsak Dec 12, 2023

cam-garrison Dec 13, 2023

bartoszmajsak Dec 12, 2023

bartoszmajsak left a comment •

edited

Loading

bartoszmajsak Dec 14, 2023 •

edited

Loading

bartoszmajsak Dec 14, 2023

bartoszmajsak left a comment

	func loadManifestsFrom(embeddedFS embed.FS, rootPath string) ([]manifest, error) {
	func loadManifestsFrom(fsys fs.FS, rootPath string) ([]manifest, error) {

	func (m *manifest) processTemplate(fs embed.FS, data interface{}) error {
	func (m *manifest) processTemplate(fsys fs.FS, data interface{}) error {

	if err := m.processTemplate(embeddedFiles, f.Spec); err != nil {
	if err := m.processTemplate(f.fsys, f.Spec); err != nil {

Process SM related templates in memory #15

Process SM related templates in memory #15

Conversation

cam-garrison commented Dec 11, 2023

bartoszmajsak left a comment

Choose a reason for hiding this comment

bartoszmajsak Dec 12, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoszmajsak left a comment • edited Loading

Choose a reason for hiding this comment

bartoszmajsak Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoszmajsak left a comment

Choose a reason for hiding this comment

bartoszmajsak Dec 12, 2023 •

edited

Loading

bartoszmajsak left a comment •

edited

Loading

bartoszmajsak Dec 14, 2023 •

edited

Loading