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

Support jetson power fan configs #2379

Merged
merged 4 commits into from
Dec 10, 2024
Merged

Conversation

cywang117
Copy link
Contributor

@cywang117 cywang117 commented Oct 16, 2024

Support HOST_CONFIG_power_mode and HOST_CONFIG_fan_profile with new PowerFanConfig backend, initially for Jetpack 6 devices. The configs are received in target state and written to config.json, where os services will pick up the changes and apply the configs.

This PR combined with #2382 constitutes the complete power fan config feature.

Change-type: minor

@cywang117 cywang117 force-pushed the support-jetson-power-fan-configs branch 2 times, most recently from cfa4a14 to 2089ca4 Compare October 19, 2024 09:29
src/config/configJson.ts Fixed Show fixed Hide fixed
@cywang117 cywang117 force-pushed the support-jetson-power-fan-configs branch from 2089ca4 to 268b984 Compare October 22, 2024 20:12
cywang117 added a commit that referenced this pull request Nov 4, 2024
Add `os-power-mode.service` and `os-fan-profile.service` which report status
from applying power mode and fan profile configs as read from config.json.
The Supervisor sets these configs in config.json for these host services
to pick up and apply.

Relates-to: #2379
See: balena-io/open-balena-api#1792
See: balena-os/balena-jetson-orin#513
Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>
cywang117 added a commit that referenced this pull request Nov 6, 2024
Add `os-power-mode.service` and `os-fan-profile.service` which report status
from applying power mode and fan profile configs as read from config.json.
The Supervisor sets these configs in config.json for these host services
to pick up and apply.

Relates-to: #2379
See: balena-io/open-balena-api#1792
See: balena-os/balena-jetson-orin#513
Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>
cywang117 added a commit that referenced this pull request Nov 7, 2024
Add `os-power-mode.service` and `os-fan-profile.service` which report status
from applying power mode and fan profile configs as read from config.json.
The Supervisor sets these configs in config.json for these host services
to pick up and apply.

Relates-to: #2379
See: balena-io/open-balena-api#1792
See: balena-os/balena-jetson-orin#513
Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>
cywang117 added a commit that referenced this pull request Nov 8, 2024
Add `os-power-mode.service` and `os-fan-profile.service` which report status
from applying power mode and fan profile configs as read from config.json.
The Supervisor sets these configs in config.json for these host services
to pick up and apply.

Relates-to: #2379
See: balena-io/open-balena-api#1792
See: balena-os/balena-jetson-orin#513
Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>
cywang117 added a commit that referenced this pull request Nov 11, 2024
Add `os-power-mode.service`, `nvpmodel.service`, and `os-fan-profile.service`
which report status from applying power mode and fan profile configs as read
from config.json. The Supervisor sets these configs in config.json for these
host services to pick up and apply.

Relates-to: #2379
See: balena-io/open-balena-api#1792
See: balena-os/balena-jetson-orin#513
Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>
@cywang117 cywang117 force-pushed the support-jetson-power-fan-configs branch 3 times, most recently from ab0cf19 to decd94d Compare November 14, 2024 09:02
cywang117 added a commit that referenced this pull request Nov 18, 2024
Add `os-power-mode.service`, `nvpmodel.service`, and `os-fan-profile.service`
which report status from applying power mode and fan profile configs as read
from config.json. The Supervisor sets these configs in config.json for these
host services to pick up and apply.

Relates-to: #2379
See: balena-io/open-balena-api#1792
See: balena-os/balena-jetson-orin#513
Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>
cywang117 added a commit that referenced this pull request Nov 18, 2024
Add `os-power-mode.service`, `nvpmodel.service`, and `os-fan-profile.service`
which report status from applying power mode and fan profile configs as read
from config.json. The Supervisor sets these configs in config.json for these
host services to pick up and apply.

Also add host log streaming from `jetson-qspi-manager.service` as it
will very soon be needed for Jetson Orins.

Relates-to: #2379
See: balena-io/open-balena-api#1792
See: balena-os/balena-jetson-orin#513
Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>
@cywang117 cywang117 force-pushed the support-jetson-power-fan-configs branch 2 times, most recently from 587483b to e699a2f Compare November 21, 2024 01:45
@cywang117 cywang117 requested a review from pipex November 21, 2024 01:46
@cywang117 cywang117 marked this pull request as ready for review November 21, 2024 01:46
@flowzone-app flowzone-app bot enabled auto-merge November 21, 2024 02:18
cywang117 added a commit that referenced this pull request Nov 21, 2024
Add `os-power-mode.service`, `nvpmodel.service`, and `os-fan-profile.service`
which report status from applying power mode and fan profile configs as read
from config.json. The Supervisor sets these configs in config.json for these
host services to pick up and apply.

Also add host log streaming from `jetson-qspi-manager.service` as it
will very soon be needed for Jetson Orins.

Relates-to: #2379
See: balena-io/open-balena-api#1792
See: balena-os/balena-jetson-orin#513
Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>
Comment on lines +162 to +157
public async isRebootRequired(opts: ConfigOptions): Promise<boolean> {
const supportedOpts = _.pickBy(
_.mapKeys(opts, (_value, key) => PowerFanConfig.stripPrefix(key)),
(_value, key) => this.isSupportedConfig(key),
);
const current = await this.getBootConfig();
// A reboot is only required if the power mode is changing
return current.power_mode !== supportedOpts.power_mode;
}
Copy link
Contributor

@pipex pipex Nov 26, 2024

Choose a reason for hiding this comment

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

Why was it decided to bother with this? Before, all host config changes would require a reboot. Even if it is not necessary for the fan profile, it doesn't seem worth it the extra complexity just to avoid a reboot in this particular case.

Copy link
Contributor Author

@cywang117 cywang117 Dec 2, 2024

Choose a reason for hiding this comment

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

Rebooting when only changing the fan control runtime config is bad UX for the user as it causes unnecessary disruption to devices as well as storage wear. A host config being tied to something that requires a reboot isn't a very strong tie in my opinion; there certainly exists the concept of a runtime OS configuration even if we don't support it in the Supervisor yet. Some examples of this include swappiness and other kernel runtime parameters via sysctl, network configuration such as iptables, and hardware-specific runtime configs: motor speed, sensor sensitivity, etc..

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it cause storage wear? How often do we expect users to be changing the fan control speed?

I agree that there are use cases for changing configurations without a reboot.

I don't think the config variable interface is great for that. Config variables require an API call and a target state pull before the changes are applied, is not an interface made for runtime requirements.

If there is need for such an interface then we should work on that rather than trying to fit the config var interface for a specific use case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I think this feature has exposed some limits of the way we handle configs on top of what we were aware of before.

src/config/configJson.ts Outdated Show resolved Hide resolved
!ConfigTxt.forbiddenConfigKeys.includes(configName) &&
// power_mode and fan_profile are managed by the power-fan backend, so
// need to be excluded here as the config var name prefix is the same.
!isPowerFanSupportedConfig(configName)
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks the independence between backends, why use the same prefix HOST_CONFIG for these new configurations rather than use something different like HOST_OS which is the way we would distinguish configurations before?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add it to forbiddenConfigKeys instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about HOST_OS, the only occurrence of that I can see is the HOST_OS_VERSION env var. I'll move to adding to forbiddenConfigKeys instead.

Copy link
Contributor Author

@cywang117 cywang117 Dec 2, 2024

Choose a reason for hiding this comment

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

With latest iteration 52f3258, forbiddenConfigKeys is now separated into forbidden and unsupported keys:

// These keys are not config.txt keys and are managed by the power-fan backend.
private static UNSUPPORTED_KEYS = ['power_mode', 'fan_profile'];
// These keys are config.txt keys, but are not mutable by the Supervisor.
private static FORBIDDEN_KEYS = [
	'disable_commandline_tags',
	'cmdline',
	'kernel',
	'kernel_address',
	'kernel_old',
	'ramfsfile',
	'ramfsaddr',
	'initramfs',
	'device_tree_address',
	'init_emmc_clock',
	'avoid_safe_mode',
].concat(ConfigTxt.UNSUPPORTED_KEYS);
...
public isSupportedConfig(configName: string): boolean {
	return !ConfigTxt.FORBIDDEN_KEYS.includes(configName);
}
public isBootConfigVar(envVar: string): boolean {
	return (
		envVar.startsWith(ConfigTxt.PREFIX) &&
		!ConfigTxt.UNSUPPORTED_KEYS.includes(ConfigTxt.stripPrefix(envVar))
	);
}

The concept of unsupported keys is used by isBootConfigVar to filter out non-config.txt keys, but keep in forbidden config.txt keys such as initiramfs. When isBootConfigVar is called by envToBootConfig in bootConfigChangeRequired, this will allow bootConfigChangeRequired to provide the proper feedback to the user that they're attempting to set a forbidden (but not unsupported) key.

This still isn't ideal IMO, I think isSupportedConfig/isBootConfigVar are a bit redundant in terms of functionality. The config module could be simplified in this regard, but I would save this for another PR.

Copy link
Contributor

@pipex pipex left a comment

Choose a reason for hiding this comment

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

Seems generally OK, just some questions that relate to the discussion on the project

src/config/backends/power-fan.ts Outdated Show resolved Hide resolved
src/config/backends/power-fan.ts Outdated Show resolved Hide resolved
src/config/configJson.ts Show resolved Hide resolved
cywang117 added a commit that referenced this pull request Nov 29, 2024
Add `os-power-mode.service`, `nvpmodel.service`, and `os-fan-profile.service`
which report status from applying power mode and fan profile configs as read
from config.json. The Supervisor sets these configs in config.json for these
host services to pick up and apply.

Also add host log streaming from `jetson-qspi-manager.service` as it
will very soon be needed for Jetson Orins.

Relates-to: #2379
See: balena-io/open-balena-api#1792
See: balena-os/balena-jetson-orin#513
Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>
@cywang117 cywang117 force-pushed the support-jetson-power-fan-configs branch from e699a2f to 26b3adc Compare December 2, 2024 20:36
@cywang117 cywang117 requested a review from pipex December 2, 2024 20:49
cywang117 added a commit that referenced this pull request Dec 3, 2024
Add `os-power-mode.service`, `nvpmodel.service`, and `os-fan-profile.service`
which report status from applying power mode and fan profile configs as read
from config.json. The Supervisor sets these configs in config.json for these
host services to pick up and apply.

Also add host log streaming from `jetson-qspi-manager.service` as it
will very soon be needed for Jetson Orins.

Relates-to: #2379
See: balena-io/open-balena-api#1792
See: balena-os/balena-jetson-orin#513
Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>
cywang117 added a commit that referenced this pull request Dec 6, 2024
Add `os-power-mode.service`, `nvpmodel.service`, and `os-fan-profile.service`
which report status from applying power mode and fan profile configs as read
from config.json. The Supervisor sets these configs in config.json for these
host services to pick up and apply.

Also add host log streaming from `jetson-qspi-manager.service` as it
will very soon be needed for Jetson Orins.

Relates-to: #2379
See: balena-io/open-balena-api#1792
See: balena-os/balena-jetson-orin#513
Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>
Also deprecate path-getting method, and remove OS version check.
The OS version itself is not used in ConfigJsonConfigBackend, so
it seems the OS version check is to confirm the existence of config.json
during class init, because OS version is a field that's always there
in a valid config.json.

Signed-off-by: Christina Ying Wang <christina@balena.io>
Signed-off-by: Christina Ying Wang <christina@balena.io>
This config backend uses ConfigJsonConfigBackend to update
os.power and os.fan subfields under the "os" key, in order
to set power and fan configs. The expected format for os.power
and os.fan settings is:
```
{
  os: {
    power: {
      mode: string
    },
    fan: {
      profile: string
    }
  }
}
```

There may be other keys in os which are not managed by the Supervisor,
so PowerFanConfig backend doesn't read or write to them. Extra keys in os.power
and os.fan are ignored when getting boot config and removed when setting
boot config.

After this backend writes to config.json, host services os-power-mode
and os-fan-profile pick up the changes, on reboot in the former's case
and at runtime in the latter's case. The changes are applied by the host
services, which the Supervisor does not manage aside from streaming
their service logs to the dashboard.

Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>
Signed-off-by: Christina Ying Wang <christina@balena.io>
@cywang117 cywang117 force-pushed the support-jetson-power-fan-configs branch from 26b3adc to 2f2b2e1 Compare December 10, 2024 02:44
@flowzone-app flowzone-app bot merged commit e085013 into master Dec 10, 2024
61 checks passed
@flowzone-app flowzone-app bot deleted the support-jetson-power-fan-configs branch December 10, 2024 23:27
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.

2 participants