[WIP] Add config for FML mode custom resolution #4264

shjala · 2024-09-17T10:11:24Z

Add a new config value app.fml.resolution to set custom resolution for FML apps. This is a string value in the format of "widthxheight" and must be one of the predefined values, it can be a global config set by zcli :

zcli edge-node update <name> --config="app.fml.resolution:1024x768"

or it can be set per-vm as part of the "cloud-config" top-level values:

#cloud-config

hostname: vm01
manage_etc_hosts: true
app.fml.resolution: 800x600

ethernets:
    eth0:
        addresses:
        - 192.168.1.3/24
        dhcp4: false
        gateway4: 192.168.1.1
        match:
            macaddress: DE:AD:BE:EF:06:97
        nameservers:
            addresses:
            - 1.1.1.1
            - 8.8.8.8
        set-name: eth0
version: 2

This needs to be reworked and go after we #4261.

Tests

I've successfully tested the followings :

Config is received if set either via cloud-config or global config ✅
app.fml.resolution does not make cloud-config invalid and it is not affecting the other configs in the cloud-config (tested with Ubuntu) ✅
If both global and cloud-config are set, cloud-config takes precedence ✅
If no cloud-config is set, the global config is used ✅

OhmSpectator · 2024-09-17T10:17:09Z

Great! Thank you! Later based on it we will choose the OVMF file...

milan-zededa · 2024-09-17T10:23:17Z

Would it make sense to have it configurable per app? Or is device-wide setting sufficient?
Also, don't forget to mention this in CONFIG-PROPERTIES.md

shjala · 2024-09-17T10:26:04Z

I will split the doc commit later.

shjala · 2024-09-17T10:27:40Z

Would it make sense to have it configurable per app? Or is device-wide setting sufficient? Also, don't forget to mention this in CONFIG-PROPERTIES.md

Sure it can be, but that is the ask, It is needed to be device-wide, so they can set it and then deploy windows based FML apps and it should come up with the enforced resolution.

milan-zededa · 2024-09-17T10:31:29Z

pkg/pillar/types/global.go

@@ -943,6 +955,15 @@ func validateSyslogKernelLevel(level string) error {
 	return nil
 }

+func validateCustomResolution(res string) error {
+	// it can be notset or one of the predefined resolutions
+	if res == FmlResolutionNotSet || res == FmlResolution1024x768 || res == FmlResolution1920x1080 {


What is limiting the set of available resolutions? Qemu or our code?

Probably we need to create custom OVMF variable (a binary that we can't generate on the fly) with predefined resolution and load it via qemu, so it needs to be one of the values we already created OVMF_vars for.

There is the -device VGA,xres=1280,yres=800 option too, but I'm not sure if that works, but if later works, then I will remove the limitation.

OhmSpectator · 2024-09-17T10:39:14Z

Would it make sense to have it configurable per app?

I'm just curious: do we support per-vm config props?

shjala · 2024-09-17T10:44:24Z

Would it make sense to have it configurable per app?

I'm just curious: do we support per-vm config props?

I think we do, like VmMode or EnableVnc.

milan-zededa · 2024-09-17T10:46:32Z

Would it make sense to have it configurable per app?

I'm just curious: do we support per-vm config props?

Not that I'm aware of.
I was just thinking if it would make sense to add resolution enum into VmConfig
It requires an EVE API enhancement, so not suitable for the current issue, but long-term it might make sense to create something more user-friendly.

OhmSpectator · 2024-09-17T11:10:43Z

I was just thinking if it would make sense to add resolution enum into VmConfig
It requires an EVE API enhancement, so not suitable for the current issue, but long-term it might make sense to create something more user-friendly.

Yeah, we have it in mind but do not go the "proper" way now, as we need to avoid API changes...

OhmSpectator · 2024-09-19T17:23:48Z

pkg/pillar/hypervisor/kvm.go

@@ -796,6 +807,33 @@ func (ctx KvmContext) CreateDomConfig(domainName string,
 		isVncShimVMEnabled(globalConfig, config)
 	tmplCtx.DomainConfig.DisplayName = domainName

+	// if we are not getting the resolution from the cloud-init, check the
+	// global config.
+	fmlResolutions := status.FmlCustomResolution


Actually, we need the resolution to be known earlier before calling CreateDomConfig. The resolution is used to define the OVMF file at the beginning of the Setup() function...

I know, this hasn't been updated with the changes of 9.4 backport.

OhmSpectator · 2024-09-20T06:52:22Z

I took the commits from this PR as part of #4261

shjala · 2024-09-20T07:34:47Z

I took the commits from this PR as part of #4261

this needs to be updated.

Add a new global config value app.fml.resolution to set custom resolution for FML apps. This is a string value in the format of "widthxheight". This value can be set device-wide as a global config value or set in a per-app/vm setting by defining it as top-level variable in the cloud-config. Signed-off-by: Shahriyar Jalayeri <[email protected]>

OhmSpectator · 2024-09-20T16:51:12Z

Closing the PR as we moved the commits into #4261

shjala requested review from OhmSpectator, rene, rouming and eriknordmark as code owners September 17, 2024 10:11

shjala changed the title ~~Add global config for FML mode custom resolution~~ [WIP] Add global config for FML mode custom resolution Sep 17, 2024

shjala marked this pull request as draft September 17, 2024 10:11

github-actions bot requested a review from rucoder September 17, 2024 10:11

shjala force-pushed the fml.cus.res branch from 502ec0a to 33bac62 Compare September 17, 2024 10:13

shjala force-pushed the fml.cus.res branch from 33bac62 to 6a083e4 Compare September 17, 2024 10:25

shjala force-pushed the fml.cus.res branch 2 times, most recently from 1baaf14 to 054a98e Compare September 17, 2024 10:30

milan-zededa reviewed Sep 17, 2024

View reviewed changes

shjala force-pushed the fml.cus.res branch from 054a98e to b093cac Compare September 17, 2024 11:07

github-actions bot requested review from jsfakian, milan-zededa and uncleDecart September 18, 2024 14:52

shjala force-pushed the fml.cus.res branch 2 times, most recently from 1f57b55 to 20b6443 Compare September 19, 2024 08:18

shjala changed the title ~~[WIP] Add global config for FML mode custom resolution~~ [WIP] Add config for FML mode custom resolution Sep 19, 2024

shjala mentioned this pull request Sep 19, 2024

[9.4-stable] Add Support for Persistent OVMF Settings in Pillar #4269

Closed

OhmSpectator reviewed Sep 19, 2024

View reviewed changes

shjala force-pushed the fml.cus.res branch from 9f05236 to 41effaa Compare September 20, 2024 07:42

github-actions bot requested a review from OhmSpectator September 20, 2024 07:43

shjala mentioned this pull request Sep 20, 2024

Add Support for Persistent OVMF Settings in Pillar #4261

Merged

5 tasks

shjala force-pushed the fml.cus.res branch from 41effaa to a3a98f7 Compare September 20, 2024 08:16

OhmSpectator closed this Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add config for FML mode custom resolution #4264

[WIP] Add config for FML mode custom resolution #4264

shjala commented Sep 17, 2024 •

edited

Loading

OhmSpectator commented Sep 17, 2024

milan-zededa commented Sep 17, 2024

shjala commented Sep 17, 2024

shjala commented Sep 17, 2024 •

edited

Loading

milan-zededa Sep 17, 2024

shjala Sep 17, 2024

OhmSpectator commented Sep 17, 2024

shjala commented Sep 17, 2024

milan-zededa commented Sep 17, 2024

OhmSpectator commented Sep 17, 2024

OhmSpectator Sep 19, 2024

shjala Sep 20, 2024

OhmSpectator commented Sep 20, 2024

shjala commented Sep 20, 2024 •

edited

Loading

OhmSpectator commented Sep 20, 2024

[WIP] Add config for FML mode custom resolution #4264

[WIP] Add config for FML mode custom resolution #4264

Conversation

shjala commented Sep 17, 2024 • edited Loading

Tests

OhmSpectator commented Sep 17, 2024

milan-zededa commented Sep 17, 2024

shjala commented Sep 17, 2024

shjala commented Sep 17, 2024 • edited Loading

milan-zededa Sep 17, 2024

Choose a reason for hiding this comment

shjala Sep 17, 2024

Choose a reason for hiding this comment

OhmSpectator commented Sep 17, 2024

shjala commented Sep 17, 2024

milan-zededa commented Sep 17, 2024

OhmSpectator commented Sep 17, 2024

OhmSpectator Sep 19, 2024

Choose a reason for hiding this comment

shjala Sep 20, 2024

Choose a reason for hiding this comment

OhmSpectator commented Sep 20, 2024

shjala commented Sep 20, 2024 • edited Loading

OhmSpectator commented Sep 20, 2024

shjala commented Sep 17, 2024 •

edited

Loading

shjala commented Sep 17, 2024 •

edited

Loading

shjala commented Sep 20, 2024 •

edited

Loading