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

feature: make watchdog configuration more flexible #272

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

lexxxel
Copy link
Collaborator

@lexxxel lexxxel commented Sep 18, 2024

resolves: #271

Copy link
Contributor

✨ Amplify has finished checking this pull request

👍 Everything looks good! No issues detected in 📄 2 files and ❇️ 17 lines of code.

Security Pipeline

Tool Configured Result
Semgrep

Last updated by commit 206af71 at 2024-09-18 23:42:04 UTC.

Note

To ignore a finding, append @amplify-ignore in a comment to the end of the vulnerable line like // @amplify-ignore or # @amplify-ignore. For more details, visit Amplify Security.

@lexxxel lexxxel requested a review from lae September 19, 2024 07:34
tasks/kernel_module_cleanup.yml Show resolved Hide resolved
modprobe:
name: softdog
name: "{{ pve_watchdog }}"
Copy link
Owner

Choose a reason for hiding this comment

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

I'm kind of brooding over whether or not "just fallback to the value of pve_watchdog" is appropriate or not. The intel watchdog module is itco_wdt, which is a bit inconsistent with how pve_watchdog: ipmi loads ipmi_watchdog (i.e. not ipmi).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that it's part of the kernel_module_cleanup.yml is kind of dirty. It would look better to have separate tasks and import them depending on the pve_watchdog value.

Copy link
Owner

@lae lae left a comment

Choose a reason for hiding this comment

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

After thinking about it, I might (emphasis on might) eventually just replace/override the pve_watchdog role variable with a pve_watchdog_module role variable so that it's clearer that it's the module name and use that as the basis for making logical decisions about what configuration to create or clean up. Cause otherwise I'd've probably just wanted to have pve_watchdog: intel indicate configuring itco_wdt and whatever else.

Kernel module cleanup also does need to happen at a particular stage separate from configuration (iirc after/before kernel installation/upgrade) so it's a tricky line to tread. But we can work this out another day.

@lae lae merged commit 459001c into develop Sep 21, 2024
2 checks passed
@lae lae deleted the feature/extend_watchdog branch September 21, 2024 01:44
@lexxxel
Copy link
Collaborator Author

lexxxel commented Sep 21, 2024

Sounds good. Thank you for merging

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.

pve_watchdog is not flexible enough
2 participants