feat: support hardware watchdog timers #8313

dsseng · 2024-02-13T18:10:55Z

Pull Request

What? (description)

Add support for Linux hardware watchdog API and corresponding configuration values. Watchdog timer is armed and fed by a controller (is it reliable to be sure a ticker will tick fine as long as no hang occurs?).

Watchdog timer can help automatically restoring a bare metal/supported VM node after a hang caused by a software or hardware error.

Why? (reasoning)

Fixes #8284, approved by developers in the comments

Acceptance

Please use the following checklist:

you linked an issue (if applicable)
you included tests (not applicable due to small footprint and mainly being ioctl-centered)
you ran conformance (make conformance - not done due to certainly being unrelated)
you formatted your code (make fmt)
you linted your code (make lint)
you generated documentation (make docs)
you ran unit-tests (make unit-tests)

See make help for a description of the available targets.

dsseng · 2024-02-13T18:15:01Z

internal/app/machined/pkg/controllers/runtime/watchdog_timer.go

+				}
+
+				logger.Info("Opened hardware watchdog", zap.String("filename", dev))
+				// TODO: support reboot/shutdown watchdogs


We currently disable watchdog when quitting. systemd however allows for shutdown, reboot and kexec watchdogs to be set up to ensure power operations don't hang the system in inaccessible state. I could also do so if we clarify requirements for this and how are controllers being shut down during a restart or poweroff to be sure what timeout would be sane

Controllers are terminated right before the reboot/shutdown (from the controller point of view, context will be cancelled).

Okay, then I might add a prop for that after the first review. However shutdown/reboot watchdog won't be turned on by default as I'm not sure what are the safe values and they depend on target config.

dsseng · 2024-02-21T15:50:47Z

Why are the CI checks failing? Does conform/commit/gpg-identity require me to add a GPG key and re-sign commits using it?

frezbo · 2024-02-21T16:09:46Z

Why are the CI checks failing? Does conform/commit/gpg-identity require me to add a GPG key and re-sign commits using it?

You can ignore the gpg check, it needs to be signed by one of our devs

dsseng · 2024-02-21T16:27:23Z

Ok, would be happy to hear and address reviews then

smira · 2024-02-21T18:03:11Z

@dsseng your PR looks good to me, I would like to refactor some machine configuration stuff and probably see if we can do an integration test. I will find some time to look into that, but it might not be quick.

dsseng · 2024-02-22T06:07:00Z

No problem, I'll do needed improvements as soon as you request something. Thanks for reviewing!

dsseng · 2024-02-23T11:03:49Z

Note: kernel changes are required to load watchdog drivers, I haven't committed those yet

See siderolabs/talos#8313 Signed-off-by: Dmitry Sharshakov <dmitry.sharshakov@siderolabs.com> Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>

smira · 2024-03-19T17:28:47Z

internal/app/machined/pkg/controllers/runtime/watchdog_timer.go

+			}
+
+			// 3 pings per timeout should suffice in any case
+			ticker = time.NewTicker(time.Duration(timeout/3) * time.Second)


we don't stop old ticker here... do we even need a ticker?

we could do something like https://github.com/cosi-project/runtime/blob/main/pkg/controller/runtime/internal/qruntime/internal/timer/resettable.go, but for Tickers

Doesn't a ticker stop when GC'ed? Actually we can make use of resettable timer and reset it at each trigger, or develop an in-tree wrapper (or one contributed to the Cosi runtime) for ticker yes.

Added ticker.Stop

I meant to have a resettable Ticker here copying the approach from the above (to some extent).

The reason is that we don't want to have a ticker if there's no watchdog.

Or it could be implemented easier (there are plenty of examples in the codebase) by using a channel variable and setting it to nil when ticket is inactive.

internal/app/machined/pkg/controllers/runtime/watchdog_timer.go

dsseng · 2024-03-19T19:32:57Z

Will update the kernel once more and rebase/squash stuff

internal/app/machined/pkg/controllers/runtime/watchdog_timer.go

smira · 2024-03-21T19:11:13Z

@dsseng refactored controller a bit, other changes are config-related:

moved configuration to a separate machine config document (we're slowly moving towards multi-doc)
validation for the config, use proper time.Duration type
introduced intermediate resource/controller to simplify the Watchdog controller

I will ask you to do integration test as a separate PR.

smira · 2024-03-21T19:11:36Z

/ok-to-test

smira · 2024-03-21T19:40:04Z

@dsseng added WatchdogTimerStatus, we can even try to read this:

All watchdog drivers are required return more information about the system,
some do temperature, fan and power level monitoring, some can tell you
the reason for the last reboot of the system.  The GETSUPPORT ioctl is
available to ask what the device can do:

	struct watchdog_info ident;
	ioctl(fd, WDIOC_GETSUPPORT, &ident);

the fields returned in the ident struct are:

        identity		a string identifying the watchdog driver
	firmware_version	the firmware version of the card if available
	options			a flags describing what the device supports

frezbo · 2024-03-22T04:36:38Z

hack/release.toml

+"""
+
+    [notes.watchdog]
+        title = "Hardware Watchdog Timers"


i guess we also mention this is only available for amd64?

It is available on aarch64. We actually had all drivers built-in previously, so I didn't have to modify aarch64 config except for sysfs

oh, were they built in? I guess we need to keep them also as modules if not, just to be in parity with amd64 🤔 . I'm not so sure, @smira wdyt?

https://github.com/siderolabs/pkgs/blob/6364d991db023e5f614cbbc450b4afd471e16203/kernel/build/config-arm64#L4441

Maybe some of those mustn't be modules due to the way they need to be probed, so that should be checked

it supports PCI id database https://cateee.net/lkddb/web-lkddb/WATCHDOG.html, but we might hit other issue like we try to read before udev picks them up, I was more talking about these:

❯ grep IPMI_WA kernel/build/config-arm64 3586:# CONFIG_IPMI_WATCHDOG is not set kernel/build/config-amd64 3157:CONFIG_IPMI_WATCHDOG=m ❯ grep MFD_CORE kernel/build/config-arm64 4514:CONFIG_MFD_CORE=y kernel/build/config-amd64 3630:CONFIG_MFD_CORE=m ❯ grep SP5100 kernel/build/config-amd64 3581:CONFIG_SP5100_TCO=m

which are not consistent across amd64 and arm64

we can fix it up, but it shouldn't be related to to this PR specifically.

I think arm64 in general needs a module cleanup the way I did for amd64 - try to make everything which can be a module to be a module.

arm64 kernel is not compressed, so moving to initramfs.xz is a good thing imho

yeh, arm64 needs some cleanup

SP5100 is AMD WDT, MFD is for Intel one, but IPMI might work for ARM. Regarding PCI ID DB: aarch64 WDTs are typically part of the SoC

frezbo · 2024-03-22T04:42:07Z

pkg/machinery/config/types/runtime/event_sink.go

@@ -93,6 +93,11 @@ func (s *EventSinkV1Alpha1) KmsgLogURLs() []*url.URL {
 	return nil
 }

+// WatchdogTimer implements config.RuntimeConfig interface.
+func (s *EventSinkV1Alpha1) WatchdogTimer() config.WatchdogTimerConfig {


hmm why does eventsink need watchdogtimer method implemented?

that's the way I implemented Runtime() config interface - all documents implement all methods, but return nil if it's not their thing. it might be good or bad idea, idk, but it is this way right now ;)

dsseng · 2024-03-22T05:50:45Z

@dsseng added WatchdogTimerStatus, we can even try to read this:

All watchdog drivers are required return more information about the system,
some do temperature, fan and power level monitoring, some can tell you
the reason for the last reboot of the system.  The GETSUPPORT ioctl is
available to ask what the device can do:

	struct watchdog_info ident;
	ioctl(fd, WDIOC_GETSUPPORT, &ident);

the fields returned in the ident struct are:

        identity		a string identifying the watchdog driver
	firmware_version	the firmware version of the card if available
	options			a flags describing what the device supports

We should probably report that using sysfs to also know timeout and timeleft

smira · 2024-03-22T08:14:50Z

We should probably report that using sysfs to also know timeout and timeleft

Timeout we already report (as we set it), while timeleft makes sense only in the moment, and the controller wakes up on feedInterval, so basically from controller point of view it's always timeleft == timeout, as we ping the watchdog.

Only enabled when activated by config, disabled on shutdown/reboot Fixes siderolabs#8284 Signed-off-by: Dmitry Sharshakov <dmitry.sharshakov@siderolabs.com> Signed-off-by: Dmitry Sharshakov <d3dx12.xx@gmail.com> Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>

dsseng · 2024-03-25T15:22:55Z

/m

dsseng force-pushed the wdt branch from b9f9735 to 75a0807 Compare February 13, 2024 18:12

dsseng commented Feb 13, 2024

View reviewed changes

dsseng force-pushed the wdt branch 2 times, most recently from d135c0e to 88ca123 Compare March 7, 2024 19:27

dsseng mentioned this pull request Mar 7, 2024

kernel: enable most common amd64 watchdog drivers siderolabs/pkgs#911

Merged

dsseng force-pushed the wdt branch from 88ca123 to 04abd51 Compare March 8, 2024 08:03

smira reviewed Mar 19, 2024

View reviewed changes

internal/app/machined/pkg/controllers/runtime/watchdog_timer.go Outdated Show resolved Hide resolved

smira reviewed Mar 19, 2024

View reviewed changes

internal/app/machined/pkg/controllers/runtime/watchdog_timer.go Outdated Show resolved Hide resolved

dsseng force-pushed the wdt branch from 5b3a2b7 to 2b011bc Compare March 19, 2024 19:45

smira reviewed Mar 19, 2024

View reviewed changes

internal/app/machined/pkg/controllers/runtime/watchdog_timer.go Outdated Show resolved Hide resolved

smira mentioned this pull request Mar 21, 2024

📦 Talos v1.7.0-beta.0 #8481

Closed

smira force-pushed the wdt branch 2 times, most recently from fb19f4a to 60f47ab Compare March 21, 2024 19:08

smira added this to the v1.7 milestone Mar 21, 2024

talos-bot added the status/ok-to-test label Mar 21, 2024

smira force-pushed the wdt branch from 60f47ab to 352b871 Compare March 21, 2024 19:39

smira force-pushed the wdt branch from 352b871 to deb622a Compare March 21, 2024 19:47

frezbo reviewed Mar 22, 2024

View reviewed changes

frezbo approved these changes Mar 22, 2024

View reviewed changes

smira approved these changes Mar 22, 2024

View reviewed changes

dsseng force-pushed the wdt branch from deb622a to 9456489 Compare March 25, 2024 15:19

talos-bot added the status/ok-to-merge label Mar 25, 2024

talos-bot merged commit 9456489 into siderolabs:main Mar 25, 2024
18 checks passed

talos-bot removed status/ok-to-test status/ok-to-merge labels Mar 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support hardware watchdog timers #8313

feat: support hardware watchdog timers #8313

dsseng commented Feb 13, 2024

dsseng Feb 13, 2024

smira Feb 14, 2024

dsseng Feb 14, 2024

dsseng commented Feb 21, 2024

frezbo commented Feb 21, 2024

dsseng commented Feb 21, 2024

smira commented Feb 21, 2024

dsseng commented Feb 22, 2024

dsseng commented Feb 23, 2024

smira Mar 19, 2024

dsseng Mar 19, 2024

dsseng Mar 19, 2024

smira Mar 19, 2024

dsseng commented Mar 19, 2024

smira commented Mar 21, 2024

smira commented Mar 21, 2024

smira commented Mar 21, 2024

frezbo Mar 22, 2024

dsseng Mar 22, 2024

frezbo Mar 22, 2024

dsseng Mar 22, 2024

frezbo Mar 22, 2024

smira Mar 22, 2024

frezbo Mar 22, 2024

dsseng Mar 22, 2024

frezbo Mar 22, 2024

smira Mar 22, 2024

dsseng commented Mar 22, 2024

smira commented Mar 22, 2024

dsseng commented Mar 25, 2024

feat: support hardware watchdog timers #8313

feat: support hardware watchdog timers #8313

Conversation

dsseng commented Feb 13, 2024

Pull Request

What? (description)

Why? (reasoning)

Acceptance

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsseng commented Feb 21, 2024

frezbo commented Feb 21, 2024

dsseng commented Feb 21, 2024

smira commented Feb 21, 2024

dsseng commented Feb 22, 2024

dsseng commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsseng commented Mar 19, 2024

smira commented Mar 21, 2024

smira commented Mar 21, 2024

smira commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsseng commented Mar 22, 2024

smira commented Mar 22, 2024

dsseng commented Mar 25, 2024