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.

Sign up for GitHub

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

Jump to bottom

Set App virtualization mode via PodConfig #1038

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Set App virtualization mode via PodConfig #1038

wants to merge 1 commit into from

Conversation

shjala
Copy link
Member

@shjala shjala commented Oct 24, 2024

This is needed, for example for loading FML based windows VMs.

@shjala shjala requested a review from uncleDecart as a code owner October 24, 2024 15:31
@uncleDecart
Copy link
Member

Do we need any changes on Adam side?

giggsoff
giggsoff reviewed Oct 24, 2024
View reviewed changes
pkg/openevec/config.go
@@ -210,6 +211,7 @@ type PodConfig struct {
OpenStackMetadata bool
DatastoreOverride string
ACLOnlyHost bool
VirtMode config.VmMode
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be great to add related virt-mode flag into podDeployCmd to support it in CLI.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure!

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, is the default value is correct? or should be something else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure, please check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I set the default according to what you provided.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like some tests failed with level=fatal msg="Unknown virt-mode: " (virtualization test). Can you please try to deploy something locally?

Copy link
Member Author

@shjala shjala Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

shoot, sorry, I'll fix it.

@shjala
Copy link
Member Author

shjala commented Oct 25, 2024 •
edited
Loading

Do we need any changes on Adam side?

I don't think so, the expect already sends this, but before this change it was the default virt mode. I have a Windows VM test that works with just these changes (I mean EVE gets the virt-mode, which in my case is FML).

@shjala shjala requested a review from giggsoff October 29, 2024 11:43
@uncleDecart uncleDecart mentioned this pull request Nov 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uncleDecart uncleDecart Awaiting requested review from uncleDecart uncleDecart is a code owner

@giggsoff giggsoff Awaiting requested review from giggsoff

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shjala @uncleDecart @giggsoff