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

Update EdgeCloud_LcM.yaml #166

Merged
merged 4 commits into from
Feb 16, 2024
Merged

Update EdgeCloud_LcM.yaml #166

merged 4 commits into from
Feb 16, 2024

Conversation

gunjald
Copy link
Collaborator

@gunjald gunjald commented Jan 10, 2024

Removing HELM from virtType as Helm is packaging format for k8s applications

Removing HELM from virtType as Helm is packaging format for k8s applications
@@ -413,7 +413,6 @@ components:
enum:
- VM
- CONTAINER
- HELM
Copy link
Collaborator

Choose a reason for hiding this comment

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

Helm needs to remain as an option here. The intent is to specify the binary type such that the system knows how to deploy it. Perhaps "virtType" should be renamed to something more appropriate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or, alternatively we move this to a "imageType" field under "repository". Also in some cases they system may need to know if the VM image is of type QCOW2 or OVA/VMDK since underlying IaaS may not support certain file types, so the system may need to convert the image on the fly. So maybe the choices are really QCOW2/OVA/OVF/Container/Helm.

Copy link
Collaborator Author

@gunjald gunjald Jan 10, 2024

Choose a reason for hiding this comment

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

Helm needs to remain as an option here. The intent is to specify the binary type such that the system knows how to deploy it. Perhaps "virtType" should be renamed to something more appropriate.

I think if we keep the "HELM" as an option then it is good to change the virtType field to indicate the correct meaning. In that case VM or Container or Helm indicate what kind of images the application comprises of. For container type of Helm it is implicit that the image is pointing to a container while VM could be QCOW2 image. In that case an alternative for virtType could be "imageType". But an issue see with including Helm alongside VM or container is that these names could suggest using a single QCOW2 or a docker container while Helm chart can contain multiple containers and the resource requirements. In that case to make a common interpretation the VM type or container type in the enum should point to an associated specification that will carry the additional details associated to the VMs or Containers that could carry the information about storage, CPU, GPU etc needed for the execution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Totally in favor of keeping Helm and change virtType. Another option could be packageType which fits with all the options that Jon said - QCOW2/OVA/OVF/Container/Helm.
My opinion is purely biased by what wee see from ISV. In fact, we see more and more GA solutions packaged as Helm charts that deploy many containers.

The issue regarding Helm and resource doesn't really persist in my view. In fact, in k8s you can create a namespace with resource quotas and deploy the Helm chart in that namespace. So, also Helm charts can have the info "about storage, CPU, GPU etc needed for the execution".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

packageType sounds fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also suggest to replace VM with QCOW2 and OVA. While it is possible for the system to detect the image type from the file using "qemu-img info", I think it's better to enumerate the supported VM image types in the enum because:

  • to use qemu-img info to detect the file type, the system would need to download the file first, which may not be necessary if the user specifies the file type
  • qemu-img info just reports "raw" for anything it doesn't understand, including OVA because OVA is just a tar file.
  • having the supported file types in the spec makes it clear to both the user and the OP what VM image file types must be supported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would also suggest to replace VM with QCOW2 and OVA. While it is possible for the system to detect the image type from the file using "qemu-img info", I think it's better to enumerate the supported VM image types in the enum because:

  • to use qemu-img info to detect the file type, the system would need to download the file first, which may not be necessary if the user specifies the file type
  • qemu-img info just reports "raw" for anything it doesn't understand, including OVA because OVA is just a tar file.
  • having the supported file types in the spec makes it clear to both the user and the OP what VM image file types must be supported.

I think suggestion looks good. For now I would replace the VM with the 2 proposed options. In future we may slightly reorganize to introduce high-level categories to not mix multiple types of VM and container image formats.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I propose to merge the changes as there are no further comments.

As per review comments changing virtType to packageType and retaining the Helm as an option as it existed in original PR
As per the Jon comment replacing VM with QCOW2 and OVA as packaging types.
Copy link
Collaborator

@gainsley gainsley left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Collaborator

@sergiofranciscoortiz sergiofranciscoortiz left a comment

Choose a reason for hiding this comment

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

Resolved conflicts with main before approving changes already approved by others.

@sergiofranciscoortiz sergiofranciscoortiz merged commit 43ef829 into main Feb 16, 2024
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.

4 participants