windows restart service #1681

zackattack01 · 2024-04-16T20:01:31Z

Here is a basic sequence diagram displaying the enable path for the windows watchdog service. The launcher_watchdog_enabled control flag will trigger the initial configuration and installation, and removal of the flag will trigger removal of the service.

sequenceDiagram
    participant LauncherKolideK2Svc
    Note right of LauncherKolideK2Svc: ./launcher.exe svc ...
    create participant WindowsServiceManager
    LauncherKolideK2Svc->>WindowsServiceManager: if launcher_watchdog_enabled
    create participant LauncherKolideWatchdogSvc
    WindowsServiceManager->>LauncherKolideWatchdogSvc: have we installed the watchdog?
    Note left of LauncherKolideWatchdogSvc: ./launcher.exe watchdog

    alt yes the service already exists
        LauncherKolideK2Svc->>LauncherKolideWatchdogSvc: Restart to ensure latest
    else no the service does not exist
        LauncherKolideK2Svc->>WindowsServiceManager: 1 - create, configure, etc
        LauncherKolideK2Svc->>LauncherKolideWatchdogSvc: 2 - Start
        activate LauncherKolideWatchdogSvc
    end

    loop every n minutes
        LauncherKolideWatchdogSvc->>WindowsServiceManager: Query LauncherKolideK2Svc status
        LauncherKolideWatchdogSvc->>LauncherKolideK2Svc: Start if Stopped
    end

The restart functionality is currently limited to detecting a stopped state, but the idea here is to lay out the foundation for more advanced healthchecking.
The watchdog service itself runs as a separate invocation of launcher, writing all logs to sqlite. The main invocation of launcher runs a watchdog controller, which responds to the launcher_watchdog_enabled flag, and publishes all sqlite logs to debug.json.
I am very open to any suggestions for better test coverage here. I had started to add more but the vast majority of the logic is dependent specifically on the windows service manager calls, and the amount of stubbing required significantly reduced the value. I suspect adding tests to CI test suite and being able to look across logs after the fact would be a much better approach, I'd like to explore that soon depending on where we land with this

cmd/launcher/watchdog/controller_windows.go

directionless

Initial comments on the windows stuff. Haven't dug into the log or sqlite stuff

directionless · 2024-06-06T01:54:59Z

cmd/launcher/watchdog/controller_windows.go

+ launcherWatchdogServiceName string = `LauncherKolideWatchdogSvc`
+ launcherServiceName string = `LauncherKolideK2Svc`


These are k2 specific names, as such, I would probably move this all into ee

directionless · 2024-06-06T01:57:22Z

cmd/launcher/watchdog/controller_windows.go

+ // do nothing if watchdog is not enabled
+ if !wc.knapsack.LauncherWatchdogEnabled() {
+ return
+ }


oo. This is interesting. What happens if there are pending logs, and then watchdog is disabled?

I was unsure about the right call here- i think if we're expecting that most windows devices will have this enabled someday then removing this enabled check is the right call. I had added because the case of stuck logs seemed extremely rare (and logs would still be recoverable) vs most devices having this disabled and checking logs every 5 minutes for no reason

Yeah, this is a fine choice. I guess if we get stale logs, hopefully we'll notice the timestamp

cmd/launcher/watchdog/controller_windows.go

directionless · 2024-06-06T03:10:38Z

cmd/launcher/watchdog/watchdog_service_windows.go

+}
+
+func runLauncherWatchdogService(ctx context.Context, w *winWatchdogSvc) error {
+ ticker := time.NewTicker(1 * time.Minute)


I wonder if this frequency will be okay. Or if there's going to be some memory leak. Well, it's behind a feature flag, so we'll get to find out!

directionless · 2024-06-06T03:11:12Z

cmd/launcher/watchdog/windows_watchdog.md

@@ -0,0 +1,30 @@
+### Watchdog Service


You could call this file README.md

Or move to docs/architecture?

directionless · 2024-06-06T03:15:49Z

cmd/launcher/watchdog/watchdog_service_windows.go

+ }
+}
+
+func (w *winWatchdogSvc) checkLauncherStatus(ctx context.Context) error {


Is there a way to skip this if we're in powersave mode?

I will add a link to the docs I'm working on at the end but the short answer is not easily- unless we are willing to:

add logic into here to subscribe to power events from this process as well (similar to current launcher functionality)

add logic to register directly for power events as a handler

I would love to be wrong here but there does not appear to be any reliable mechanism to gather the current state in real time (without significantly complicating the watchdog)

directionless · 2024-06-06T03:16:29Z

ee/agent/storage/sqlite/keyvalue_store_sqlite.go

@@ -19,20 +19,23 @@ import (
 _ "modernc.org/sqlite"
 )

-type storeName int
+type StoreName int


This needs to be exported?

Exported for use in the pkg/log/sqlitelogger package, looks like

Hrm. Feels a little weird. I wonder if we should be create stores and passing them around. But okay...

directionless · 2024-06-06T03:18:17Z

ee/agent/types/logstore.go

+// TimestampedIteratorDeleterAppenderCloser is an interface to support the storage and retrieval of
+// sets of timestamped values. This can be used where a strict key/value interface may not suffice,
+// e.g. for writing logs or historical records to sqlite
+type TimestampedIteratorDeleterAppenderCloser interface {


PR feedback: less noisy log levels, comment updates and style fixes Co-authored-by: seph <[email protected]>

RebeccaMahany · 2024-06-06T13:36:02Z

cmd/launcher/watchdog/controller_windows.go

+ if err := wc.logPublisher.ForEach(func(rowid, timestamp int64, v []byte) error {
+ logRecord := make(map[string]any)
+ if err := json.Unmarshal(v, &logRecord); err != nil {
+ wc.slogger.Log(ctx, slog.LevelError, "failed to unmarshal sqlite log", "log", string(v))


Can we include the err in this log also?

RebeccaMahany · 2024-06-06T13:37:45Z

cmd/launcher/watchdog/controller_windows.go

+ logRecord := make(map[string]any)
+ if err := json.Unmarshal(v, &logRecord); err != nil {
+ wc.slogger.Log(ctx, slog.LevelError, "failed to unmarshal sqlite log", "log", string(v))
+ logsToDelete = append(logsToDelete, rowid)


It looks like we can move this line to the top of the ForEach function, since there's no circumstance where we won't want to delete this log -- instead of having the append here and on line 113?

yes thank you! will do

RebeccaMahany · 2024-06-06T13:51:34Z

cmd/launcher/watchdog/controller_windows.go

+ return
+ }
+
+ if err.Error() == serviceDoesNotExistError {


I agree w/ an if err != nil + a switch or if/elseif inside to check the error type

RebeccaMahany · 2024-06-06T13:55:17Z

cmd/launcher/watchdog/controller_windows.go

+ wc.slogger.Log(ctx, slog.LevelError,
+ "installing launcher watchdog, unable to collect current executable path",
+ "err", err,
+ )


Do we need this log since we're returning an error and the calling function logs it?

nope I can remove, good call thank you!

RebeccaMahany · 2024-06-06T13:57:05Z

cmd/launcher/watchdog/controller_windows.go

+ },
+ }
+
+ if err = restartService.SetRecoveryActions(recoveryActions, 10800); err != nil {


I think this would be more readable as a const with a comment --

const serviceResetPeriod = 10800 // 3 hours in seconds

because this looks like a fun, unimportant thing to quibble over, I would go with

const serviceResetPeriodSeconds = 3 * 60 * 60 // 3 hours in seconds

RebeccaMahany · 2024-06-06T14:11:45Z

cmd/launcher/watchdog/windows_watchdog.md

@@ -0,0 +1,30 @@
+### Watchdog Service


Or move to docs/architecture?

RebeccaMahany · 2024-06-06T14:12:43Z

ee/agent/storage/sqlite/keyvalue_store_sqlite.go

@@ -19,20 +19,23 @@ import (
 _ "modernc.org/sqlite"
 )

-type storeName int
+type StoreName int


Exported for use in the pkg/log/sqlitelogger package, looks like

RebeccaMahany · 2024-06-06T14:15:04Z

ee/agent/storage/sqlite/logstore_sqlite.go

+ "strings"
+)
+
+func (s *sqliteStore) getColumns() *sqliteColumns {


This function feels like maybe it belongs in keyvalue_store_sqlite.go instead, since it applies to both tables? What do you think?

yeah that probably makes more sense, I was torn about adding a new shared file but think we can probably wait on that. i'll move there for now!

ee/agent/storage/sqlite/logstore_sqlite.go

RebeccaMahany · 2024-06-06T14:19:06Z

ee/agent/storage/sqlite/logstore_sqlite.go

+ colInfo := s.getColumns()
+ if s == nil || s.conn == nil || colInfo == nil {
+ return errors.New("store is nil")
+ }


I think you also want a restriction on calling this for a store that isn't the logstore, since scanning data into var timestamp int64 on line 91 would fail with a different table schema?

oh yeah that's a good call! I can probably expand on getColumns to either gate usage here or make it work for either, i'll take a look

…ovements, add checks for non-LogStore table methods

Co-authored-by: Rebecca Mahany-Horton <[email protected]>

directionless · 2024-06-07T01:46:17Z

side note -- do not merge until 1.7 is stable

directionless · 2024-06-07T01:49:51Z

ee/agent/storage/sqlite/keyvalue_store_sqlite.go

@@ -19,20 +19,23 @@ import (
 _ "modernc.org/sqlite"
 )

-type storeName int
+type StoreName int


Hrm. Feels a little weird. I wonder if we should be create stores and passing them around. But okay...

directionless · 2024-06-07T01:54:03Z

pkg/log/sqlitelogger/sqlite_logger.go

+ }
+)
+
+func NewSqliteLogWriter(ctx context.Context, rootDirectory string, tableName agentsqlite.StoreName) (*SqliteLogWriter, error) {


This signature feels a little off to me. I wonder if it would be cleaner if it took types.LogStore directly.

directionless · 2024-06-07T02:01:08Z

pkg/log/sqlitelogger/sqlite_logger.go

@@ -0,0 +1,52 @@
+package sqlitelogger


I don't think there's anything here related to sqlite or logging? It's implementing an io.WriteCloser. Why not in ee/agent/storage/sqlite/logstore_sqlite.go?

(I mean, it's also pretty cool, but I'm not sure why it's split out)

directionless · 2024-06-07T02:03:56Z

ee/agent/storage/sqlite/keyvalue_store_sqlite.go

 switch s {
 case StartupSettingsStore:
 return "startup_settings"
+ case WatchdogLogStore:
+ return "watchdog_logs"


If we want to push other logs here (say early startup logs) how would we do it? Would we make a new store? Accept it as slightly misnamed?

directionless · 2024-06-07T02:06:06Z

ee/agent/storage/sqlite/logstore_sqlite.go

+ }
+
+ insertSql := fmt.Sprintf(
+ `INSERT INTO %s (%s, %s) VALUES (?, ?)`,


For timestamps, should one of this be a number, not a string? Might not matter, since sqlite everything is a string...

zackattack01 force-pushed the zack/windows_watchdog branch from a38204e to b84ffd4 Compare April 30, 2024 20:43

zackattack01 force-pushed the zack/windows_watchdog branch 2 times, most recently from 1fca6bc to e530c23 Compare May 17, 2024 19:29

zackattack01 force-pushed the zack/windows_watchdog branch from 467defd to 5a6e8c1 Compare May 29, 2024 13:14

zackattack01 added 22 commits June 4, 2024 17:29

add basic working restart service installation and subcommand

fa333e4

wire in opts from launcher proper and localLogger, more clean up

815fc27

gofmt and remove unused account const

48bb3ac

stash changes for rebase with exit wrapper updates

25da24a

fix up restart service to use new os.Exit wrapper

7ebc39d

add sqlite log publication

dde4068

update restart service to use new gowrapper

d4ed14e

clean up and close log publisher on interrupt

2934743

fix DeleteRows when called without rowids

11488ab

add in enable_launcher_watchdog flag and update types+knapsack

f26b969

remove watchdog when disabled, add launcher watchdog to options

4b087f6

unstage tests, stash working version post refactor

f43569f

rework logstore interfaces, break off from kevalue, bugfixes

2620b0c

re-add staged fix for non-windows compilation

3a61659

test updates

833388c

bring back test updates

eb69c4a

gofmt

34ecc12

remove temporary option manipulation from local testing

9afe225

remove flag from startup settings, reset tests

9da22a5

unstage options for PR breakout

031a4fd

add some documentation, small optimizations

03d7f17

test updates

faefa20

zackattack01 force-pushed the zack/windows_watchdog branch from c086936 to faefa20 Compare June 4, 2024 21:31

zackattack01 added 2 commits June 5, 2024 09:54

add explicit install and removal logs, update docs

f3bbbea

remove duplicate install log

4c5c02e

zackattack01 marked this pull request as ready for review June 5, 2024 14:20

James-Pickett reviewed Jun 5, 2024

View reviewed changes

cmd/launcher/watchdog/controller_windows.go Outdated Show resolved Hide resolved

cmd/launcher/watchdog/controller_windows.go Outdated Show resolved Hide resolved

cmd/launcher/watchdog/controller_windows.go Outdated Show resolved Hide resolved

zackattack01 added 2 commits June 5, 2024 12:30

PR feedback: early returns for failed installation paths, better logging

9342e16

PR feedback: use backoff.Waitfor for service restart logic

3cbaed0

directionless reviewed Jun 6, 2024

View reviewed changes

zackattack01 and others added 2 commits June 6, 2024 09:37

Apply suggestions from code review

16f1aa9

PR feedback: less noisy log levels, comment updates and style fixes Co-authored-by: seph <[email protected]>

PR feedback: move watchdog package to ee, rename docs to README.md

e876dc8

RebeccaMahany reviewed Jun 6, 2024

View reviewed changes

zackattack01 and others added 2 commits June 6, 2024 13:10

PR feedback: const for serviceResetPeriod, logging and code flow impr…

df285d2

…ovements, add checks for non-LogStore table methods

Update ee/agent/storage/sqlite/logstore_sqlite.go

4c7b268

Co-authored-by: Rebecca Mahany-Horton <[email protected]>

directionless reviewed Jun 7, 2024

View reviewed changes

		launcherWatchdogServiceName string = `LauncherKolideWatchdogSvc`
		launcherServiceName string = `LauncherKolideK2Svc`

windows restart service #1681

Are you sure you want to change the base?

windows restart service #1681

Conversation

zackattack01 commented Apr 16, 2024 • edited

directionless left a 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

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

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

directionless commented Jun 7, 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

zackattack01 commented Apr 16, 2024 •

edited