feat: read architecture specific common templates file #1156

nestoracunablanco · 2024-12-03T11:59:21Z

This change prepares for architecture-specific delivery. The logic will first attempt to locate the architecture-specific common templates bundle. If it is not found, it will then fall back to the generic bundle.

What this PR does / why we need it: s390x enablement

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Release note:

NONE

akrejcir · 2024-12-03T12:16:19Z

internal/template-bundle/bundle.go

+	archDependentFileName := filepath.Join(templateBundleDir, commonTemplatesFilePrefix+runtime.GOARCH+"-"+common_templates.Version+commonTemplatesFileSuffix)
+	archIndependentFileName := filepath.Join(templateBundleDir, commonTemplatesFilePrefix+common_templates.Version+commonTemplatesFileSuffix)


The string concatenation is a little confusing here. Can you use fmt.Sprintf()? The format string could be:

common-templates-%s-%s.yaml`

akrejcir · 2024-12-03T12:20:28Z

internal/template-bundle/bundle.go

+func RetrieveCommonTemplatesBundleFile(templateBundleDir string) (string, error) {
+	archDependentFileName := filepath.Join(templateBundleDir, commonTemplatesFilePrefix+runtime.GOARCH+"-"+common_templates.Version+commonTemplatesFileSuffix)
+	archIndependentFileName := filepath.Join(templateBundleDir, commonTemplatesFilePrefix+common_templates.Version+commonTemplatesFileSuffix)
+	commonTemplatesExistingFiles := filterExistingTemplateFiles([]string{archDependentFileName, archIndependentFileName})


Can you simplify this by not using a filter function? IMHO directly checking if the 2 files exists is more readable.

if _, err := os.Stat(archDependentFileName); err == nil { return archDependentFileName, nil } if _, err := os.Stat(archIndependentFileName); err == nil { return archIndependentFileName, nil }

akrejcir · 2024-12-03T12:22:50Z

internal/template-bundle/bundle_test.go

+		_, filename, _, ok := runtime.Caller(0)
+		Expect(ok).To(BeTrue())
+		fileAbsolutePath, err := filepath.Abs(filename)
+		Expect(err).NotTo(HaveOccurred())
+		testDir = filepath.Dir(fileAbsolutePath)


Instead of reading files from the repo, can you create them in a temp directory when the test starts?
Ginkgo can create the directory for you:

tmpDir := GinkgoT().TempDir()

Nice idea, thanks for pointing that out

akrejcir · 2024-12-03T12:23:16Z

internal/template-bundle/test-files/all-supported-archs/common-templates-amd64-v0.31.0.yaml

Please don't commit these empty files into the repo.

nestoracunablanco · 2024-12-04T08:47:18Z

/retest

internal/template-bundle/bundle_test.go

akrejcir · 2024-12-04T13:36:32Z

/approve

kubevirt-bot · 2024-12-04T13:36:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akrejcir

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~~ [akrejcir]

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

dominikholler · 2024-12-05T08:20:12Z

@nestoracunablanco
Is the assumption that the node, which runs the ssp-operator, has the same architecture as the nodes running the VMs?
Are you aware that this change might limit the usage of templates in clusters with worker nodes of multiple architectures?
Does this mean you prefer to go the volume (instancetypes/preferences) approach for clusters using worker nodes of mixed architectures?

nestoracunablanco · 2024-12-05T10:02:26Z

Hello @dominikholler,

Thank you for reaching out. Regarding your questions:

Are you aware that this change might limit the usage of templates in clusters with worker nodes of multiple architectures?

The assumption is not that the node running the same architecture as the SSP operator will have the same architecture as the nodes running the VMs. However, it is true that multi-architecture support is currently not available and would require significant design changes. With the existing design (which works well for amd64), the SSP operator attempts to import the data volumes even if they are not enabled for the host architecture (for example, Windows or OpenSUSE on s390x). To enhance the user experience for s390x, these options are not presented.
For amd64, this change is completely transparent. In the case of s390x, end users can clone a template and modify it according to their needs (in case they want to run amd64 workloads).

Does this mean you prefer to adopt the volume (instance types/preferences) approach for clusters using worker nodes of mixed architectures?

I do not have a personal opinion on this matter. My primary focus is to enable the existing features for the s390x architecture.

Looking ahead, here are some ideas to address the lack of multi-architecture worker node support (though they may not be fully feasible):

Load the templates based on the available architectures across all nodes.
Import the data volumes selectively.

In my opinion, enabling multi-architecture worker nodes would require a design change that necessitates consensus.

dominikholler · 2024-12-05T10:30:10Z

On Thu, 05 Dec 2024 02:02:48 -0800 Nestor Acuna Blanco ***@***.***> wrote: Hello @dominikholler, Thank you for reaching out. Regarding your questions: 1. _Are you aware that this change might limit the usage of templates in clusters with worker nodes of multiple architectures?_ - The assumption is not that the node running the same architecture as the SSP operator will have the same architecture as the nodes running the VMs. However, it is true that multi-architecture support is currently not available and would require significant design changes. With the existing design (which works well for amd64), the SSP operator attempts to import the data volumes even if they are not enabled for the host architecture (for example, Windows or OpenSUSE on s390x).

As mentioned more detailed below, the import of the DataImportCrons respects the architecture if possible.

To enhance the user experience for s390x, these options are not presented.

I do not understand. If the containerdisk imported by a DataImportCron contains s390x, and the containerdisk is referring to the s390x preference (kubevirt/common-instancetypes#290), wouldn't the OpenShift UI create a working VM from the related golden base image? Are you looking for a way to prevent importing DataImportCrons which does not support the architecture of the node?

For amd64, this change is completely transparent. In the case of s390x, end users can clone a template and modify it according to their needs (in case they want to run amd64 workloads). 2. _Does this mean you prefer to adopt the volume (instance types/preferences) approach for clusters using worker nodes of mixed architectures?_ - I do not have a personal opinion on this matter. My primary focus is to enable the existing features for the s390x architecture.

I see.

Looking ahead, here are some ideas to address the lack of multi-architecture worker node support (though they may not be fully feasible): - Load the templates based on the available architectures across all nodes.

Sounds interesting. What do you mean by "load" here, maybe showing them in the UI?

- Import the data volumes selectively.

If all worker nodes have the same architecture, this should be already done. You are welcome to chime in on https://issues.redhat.com/browse/CNV-52175?focusedId=26209275&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-26209275

…

In my opinion, enabling multi-architecture worker nodes would require a design change that necessitates consensus.

nestoracunablanco · 2024-12-05T12:22:19Z

I do not understand. If the containerdisk imported by a DataImportCron
contains s390x, and the containerdisk is referring to the s390x
preference
(kubevirt/common-instancetypes#290), wouldn't the OpenShift UI create a working VM from the related golden base image?

Are you looking for a way to prevent importing DataImportCrons which
does not support the architecture of the node?

Yes, if the container disk includes s390x, that is not an issue. However, some Linux distributions do not support s390x, and Windows does not either. In my opinion, it could be inconvenient (or even a bug) for users if we present features they cannot utilize, such as starting Windows or openSUSE VMs that are incompatible with s390x.
Another aspect to consider is when a user attempts to run a VM based on a template, such as selecting "Fedora." If this does not work out of the box because the default architecture does not match that of the worker nodes, it does not reflect an ideal user experience.
I am open to achieving these goals through various means, as long as it enhances the user experience and does not disrupt existing functionality. To facilitate this discussion, I suggest we hold a meeting with all involved parties, as lengthy messages in a pull request can be quite time-consuming.

Sounds interesting. What do you mean by "load" here, maybe showing
them in the UI?

For example, as long as the user is not presented with any "misleading" indicators, this can be addressed at the UI level, be filtered by the SSP operator, etc. etc. etc.

akrejcir · 2024-12-05T15:49:35Z

@0xFelix , can you please take a look?

0xFelix · 2024-12-06T08:09:42Z

internal/template-bundle/bundle_test.go

+		archIndependentFile = tmpDir + fmt.Sprintf("/common-templates-%s.yaml", common_templates.Version)
+		archDependentFile = tmpDir + fmt.Sprintf("/common-templates-%s-%s.yaml", runtime.GOARCH, common_templates.Version)


Use filepath.Join

0xFelix · 2024-12-06T08:10:02Z

internal/template-bundle/bundle_test.go

+
+	It("should throw an error retrieving the bundle file", func() {
+		_, err := RetrieveCommonTemplatesBundleFile(tmpDir)
+		Expect(err.Error()).To(ContainSubstring("failed to find common-templates bundles, none of the files were found"))


Suggested change

Expect(err.Error()).To(ContainSubstring("failed to find common-templates bundles, none of the files were found"))

Expect(err).To(MatchError(ContainSubstring("failed to find common-templates bundles, none of the files were found")))

This change prepares for architecture-specific delivery. The logic will first attempt to locate the architecture-specific common templates bundle. If it is not found, it will then fall back to the generic bundle. Signed-off-by: Nestor Acuna Blanco <nestor.acuna@ibm.com>

sonarqubecloud · 2024-12-06T08:38:21Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

0xFelix

/lgtm

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. size/M labels Dec 3, 2024

nestoracunablanco force-pushed the feat/selectArchFileIfExists branch from 9b4849c to 50f7390 Compare December 3, 2024 11:59

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Dec 3, 2024

kubevirt-bot requested review from akrejcir and codingben December 3, 2024 11:59

akrejcir reviewed Dec 3, 2024

View reviewed changes

nestoracunablanco force-pushed the feat/selectArchFileIfExists branch 2 times, most recently from 2cc78b0 to 0532aea Compare December 3, 2024 13:25

akrejcir reviewed Dec 4, 2024

View reviewed changes

internal/template-bundle/bundle_test.go Outdated Show resolved Hide resolved

nestoracunablanco force-pushed the feat/selectArchFileIfExists branch from 0532aea to 10ab46a Compare December 4, 2024 12:39

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2024

0xFelix reviewed Dec 6, 2024

View reviewed changes

nestoracunablanco force-pushed the feat/selectArchFileIfExists branch from 10ab46a to ca7e46f Compare December 6, 2024 08:37

0xFelix approved these changes Dec 9, 2024

View reviewed changes

kubevirt-bot assigned 0xFelix Dec 9, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2024

kubevirt-bot merged commit 5b3a952 into kubevirt:main Dec 9, 2024
11 of 12 checks passed

nestoracunablanco deleted the feat/selectArchFileIfExists branch December 9, 2024 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: read architecture specific common templates file #1156

feat: read architecture specific common templates file #1156

nestoracunablanco commented Dec 3, 2024

akrejcir Dec 3, 2024

nestoracunablanco Dec 3, 2024

akrejcir Dec 3, 2024

nestoracunablanco Dec 3, 2024

akrejcir Dec 3, 2024

nestoracunablanco Dec 3, 2024

akrejcir Dec 3, 2024

nestoracunablanco Dec 3, 2024

nestoracunablanco commented Dec 4, 2024

akrejcir commented Dec 4, 2024

kubevirt-bot commented Dec 4, 2024

dominikholler commented Dec 5, 2024 •

edited

Loading

nestoracunablanco commented Dec 5, 2024

dominikholler commented Dec 5, 2024 via email

nestoracunablanco commented Dec 5, 2024 •

edited

Loading

akrejcir commented Dec 5, 2024

0xFelix Dec 6, 2024

nestoracunablanco Dec 6, 2024

0xFelix Dec 6, 2024

nestoracunablanco Dec 6, 2024

sonarqubecloud bot commented Dec 6, 2024

0xFelix left a comment

		archDependentFileName := filepath.Join(templateBundleDir, commonTemplatesFilePrefix+runtime.GOARCH+"-"+common_templates.Version+commonTemplatesFileSuffix)
		archIndependentFileName := filepath.Join(templateBundleDir, commonTemplatesFilePrefix+common_templates.Version+commonTemplatesFileSuffix)

		archIndependentFile = tmpDir + fmt.Sprintf("/common-templates-%s.yaml", common_templates.Version)
		archDependentFile = tmpDir + fmt.Sprintf("/common-templates-%s-%s.yaml", runtime.GOARCH, common_templates.Version)

	Expect(err.Error()).To(ContainSubstring("failed to find common-templates bundles, none of the files were found"))
	Expect(err).To(MatchError(ContainSubstring("failed to find common-templates bundles, none of the files were found")))

feat: read architecture specific common templates file #1156

feat: read architecture specific common templates file #1156

Conversation

nestoracunablanco commented Dec 3, 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

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

nestoracunablanco commented Dec 4, 2024

akrejcir commented Dec 4, 2024

kubevirt-bot commented Dec 4, 2024

dominikholler commented Dec 5, 2024 • edited Loading

nestoracunablanco commented Dec 5, 2024

dominikholler commented Dec 5, 2024 via email

nestoracunablanco commented Dec 5, 2024 • edited Loading

akrejcir commented Dec 5, 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

sonarqubecloud bot commented Dec 6, 2024

Quality Gate passed

0xFelix left a comment

Choose a reason for hiding this comment

dominikholler commented Dec 5, 2024 •

edited

Loading

nestoracunablanco commented Dec 5, 2024 •

edited

Loading