From 46ade66c0fff6f7b663fcfc987f8a53280e47cd8 Mon Sep 17 00:00:00 2001 From: Tim Lee Date: Wed, 2 Oct 2024 15:43:19 -0600 Subject: [PATCH] Align battery health reporting (#22569) --- changes/19619-align-battery-health | 1 + .../Contributing/Understanding-host-vitals.md | 13 +- server/service/hosts.go | 12 -- server/service/hosts_test.go | 6 +- server/service/integration_core_test.go | 8 +- server/service/integration_desktop_test.go | 2 +- server/service/osquery_test.go | 2 +- server/service/osquery_utils/queries.go | 96 ++++++------- server/service/osquery_utils/queries_test.go | 126 +++++++++++------- 9 files changed, 138 insertions(+), 128 deletions(-) create mode 100644 changes/19619-align-battery-health diff --git a/changes/19619-align-battery-health b/changes/19619-align-battery-health new file mode 100644 index 000000000000..6bbe536bcf60 --- /dev/null +++ b/changes/19619-align-battery-health @@ -0,0 +1 @@ +- battery health definitions now defined as cycle counts greater than 1000 or max capacity falling under 80% of designed capacity for macOS and Windows \ No newline at end of file diff --git a/docs/Contributing/Understanding-host-vitals.md b/docs/Contributing/Understanding-host-vitals.md index 3aa2078574f1..c1c6d2b0c9d1 100644 --- a/docs/Contributing/Understanding-host-vitals.md +++ b/docs/Contributing/Understanding-host-vitals.md @@ -3,18 +3,9 @@ Following is a summary of the detail queries hardcoded in Fleet used to populate the device details: -## battery_macos +## battery -- Platforms: darwin - -- Query: -```sql -SELECT serial_number, cycle_count, health FROM battery; -``` - -## battery_windows - -- Platforms: windows +- Platforms: windows, darwin - Discovery query: ```sql diff --git a/server/service/hosts.go b/server/service/hosts.go index a4db5b73dc0a..eef4cb65f236 100644 --- a/server/service/hosts.go +++ b/server/service/hosts.go @@ -1152,18 +1152,6 @@ func (svc *Service) getHostDetails(ctx context.Context, host *fleet.Host, opts f nextMw.StartsAt = nextMw.StartsAt.In(gCalLoc) } - // Due to a known osquery issue with M1 Macs, we are ignoring the stored value in the db - // and replacing it at the service layer with custom values determined by the cycle count. - // See https://github.com/fleetdm/fleet/issues/6763. - // TODO: Update once the underlying osquery issue has been resolved. - for _, b := range bats { - if b.CycleCount < 1000 { - b.Health = "Normal" - } else { - b.Health = "Replacement recommended" - } - } - var policies *[]*fleet.HostPolicy if opts.IncludePolicies { hp, err := svc.ds.ListPoliciesForHost(ctx, host) diff --git a/server/service/hosts_test.go b/server/service/hosts_test.go index 26611cb8299c..59e1cc9413d7 100644 --- a/server/service/hosts_test.go +++ b/server/service/hosts_test.go @@ -65,7 +65,7 @@ func TestHostDetails(t *testing.T) { ds.ListPoliciesForHostFunc = func(ctx context.Context, host *fleet.Host) ([]*fleet.HostPolicy, error) { return nil, nil } - dsBats := []*fleet.HostBattery{{HostID: host.ID, SerialNumber: "a", CycleCount: 999, Health: "Check Battery"}, {HostID: host.ID, SerialNumber: "b", CycleCount: 1001, Health: "Good"}} + dsBats := []*fleet.HostBattery{{HostID: host.ID, SerialNumber: "a", CycleCount: 999, Health: "Normal"}, {HostID: host.ID, SerialNumber: "b", CycleCount: 1001, Health: "Service recommended"}} ds.ListHostBatteriesFunc = func(ctx context.Context, hostID uint) ([]*fleet.HostBattery, error) { return dsBats, nil } @@ -75,8 +75,6 @@ func TestHostDetails(t *testing.T) { ds.GetHostLockWipeStatusFunc = func(ctx context.Context, host *fleet.Host) (*fleet.HostLockWipeStatus, error) { return &fleet.HostLockWipeStatus{}, nil } - // Health should be replaced at the service layer with custom values determined by the cycle count. See https://github.com/fleetdm/fleet/issues/6763. - expectedBats := []*fleet.HostBattery{{HostID: host.ID, SerialNumber: "a", CycleCount: 999, Health: "Normal"}, {HostID: host.ID, SerialNumber: "b", CycleCount: 1001, Health: "Replacement recommended"}} opts := fleet.HostDetailOptions{ IncludeCVEScores: false, @@ -87,7 +85,7 @@ func TestHostDetails(t *testing.T) { assert.Equal(t, expectedLabels, hostDetail.Labels) assert.Equal(t, expectedPacks, hostDetail.Packs) require.NotNil(t, hostDetail.Batteries) - assert.Equal(t, expectedBats, *hostDetail.Batteries) + assert.Equal(t, dsBats, *hostDetail.Batteries) require.Nil(t, hostDetail.MDM.MacOSSettings) } diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index 8332e65eaf94..a1367e85c622 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -8327,8 +8327,8 @@ func (s *integrationTestSuite) TestGetHostBatteries() { require.NoError(t, err) bats := []*fleet.HostBattery{ - {HostID: host.ID, SerialNumber: "a", CycleCount: 1, Health: "Good"}, - {HostID: host.ID, SerialNumber: "b", CycleCount: 1002, Health: "Poor"}, + {HostID: host.ID, SerialNumber: "a", CycleCount: 1, Health: "Normal"}, + {HostID: host.ID, SerialNumber: "b", CycleCount: 1002, Health: "Service recommended"}, } require.NoError(t, s.ds.ReplaceHostBatteries(context.Background(), host.ID, bats)) @@ -8338,7 +8338,7 @@ func (s *integrationTestSuite) TestGetHostBatteries() { // only cycle count and health are returned require.ElementsMatch(t, []*fleet.HostBattery{ {CycleCount: 1, Health: "Normal"}, - {CycleCount: 1002, Health: "Replacement recommended"}, + {CycleCount: 1002, Health: "Service recommended"}, }, *getHostResp.Host.Batteries) // same for get host by identifier @@ -8347,7 +8347,7 @@ func (s *integrationTestSuite) TestGetHostBatteries() { // only cycle count and health are returned require.ElementsMatch(t, []*fleet.HostBattery{ {CycleCount: 1, Health: "Normal"}, - {CycleCount: 1002, Health: "Replacement recommended"}, + {CycleCount: 1002, Health: "Service recommended"}, }, *getHostResp.Host.Batteries) } diff --git a/server/service/integration_desktop_test.go b/server/service/integration_desktop_test.go index cd82faa191f7..ca3056e3a890 100644 --- a/server/service/integration_desktop_test.go +++ b/server/service/integration_desktop_test.go @@ -38,7 +38,7 @@ func (s *integrationTestSuite) TestDeviceAuthenticatedEndpoints() { require.NoError(t, s.ds.SetOrUpdateMunkiInfo(context.Background(), hosts[0].ID, "1.3.0", nil, nil)) // create a battery for hosts[0] require.NoError(t, s.ds.ReplaceHostBatteries(context.Background(), hosts[0].ID, []*fleet.HostBattery{ - {HostID: hosts[0].ID, SerialNumber: "a", CycleCount: 1, Health: "Good"}, + {HostID: hosts[0].ID, SerialNumber: "a", CycleCount: 1, Health: "Normal"}, })) // create an auth token for hosts[0] diff --git a/server/service/osquery_test.go b/server/service/osquery_test.go index ff913ac46f30..d5fc4a219a3d 100644 --- a/server/service/osquery_test.go +++ b/server/service/osquery_test.go @@ -1062,7 +1062,7 @@ func verifyDiscovery(t *testing.T, queries, discovery map[string]string) { hostDetailQueryPrefix + "orbit_info": {}, hostDetailQueryPrefix + "software_vscode_extensions": {}, hostDetailQueryPrefix + "software_macos_firefox": {}, - hostDetailQueryPrefix + "battery_windows": {}, + hostDetailQueryPrefix + "battery": {}, } for name := range queries { require.NotEmpty(t, discovery[name]) diff --git a/server/service/osquery_utils/queries.go b/server/service/osquery_utils/queries.go index 5c93b5d2d680..3858915f9ee5 100644 --- a/server/service/osquery_utils/queries.go +++ b/server/service/osquery_utils/queries.go @@ -555,17 +555,13 @@ var extraDetailQueries = map[string]DetailQuery{ DirectIngestFunc: directIngestChromeProfiles, Discovery: discoveryTable("google_chrome_profiles"), }, - "battery_macos": { - Query: `SELECT serial_number, cycle_count, health FROM battery;`, - Platforms: []string{"darwin"}, - DirectIngestFunc: directIngestBattery, - // the "battery" table doesn't need a Discovery query as it is an official - // osquery table on darwin (https://osquery.io/schema/5.3.0#battery), it is - // always present. - }, - "battery_windows": { + "battery": { + // This query is used to determine battery health of macOS and Windows hosts + // based on the cycle count, designed capacity, and max capacity of the battery. + // The `health` column is ommitted due to a known osquery issue with M1 Macs + // (https://github.com/fleetdm/fleet/issues/6763) and its absence on Windows. Query: `SELECT serial_number, cycle_count, designed_capacity, max_capacity FROM battery`, - Platforms: []string{"windows"}, + Platforms: []string{"windows", "darwin"}, DirectIngestFunc: directIngestBattery, Discovery: discoveryTable("battery"), // added to Windows in v5.12.1 (https://github.com/osquery/osquery/releases/tag/5.12.1) }, @@ -1300,71 +1296,79 @@ func directIngestChromeProfiles(ctx context.Context, logger log.Logger, host *fl return ds.ReplaceHostDeviceMapping(ctx, host.ID, mapping, fleet.DeviceMappingGoogleChromeProfiles) } +// directIngestBattery ingests battery data from a host on a Windows or macOS platform +// and calculates the battery health based on cycle count and capacity. +// Due to a known osquery issue with M1 Macs (https://github.com/fleetdm/fleet/issues/6763) +// and the ommission of the `health` column on Windows, we are not leveraging the `health` +// column in the query and instead aligning the definition of battery health between +// macOS and Windows. func directIngestBattery(ctx context.Context, logger log.Logger, host *fleet.Host, ds fleet.Datastore, rows []map[string]string) error { mapping := make([]*fleet.HostBattery, 0, len(rows)) + for _, row := range rows { - cycleCount, err := strconv.Atoi(EmptyToZero(row["cycle_count"])) + health, cycleCount, err := generateBatteryHealth(row, logger) if err != nil { - return err + level.Error(logger).Log("op", "directIngestBattery", "hostID", host.ID, "err", err) } - switch host.Platform { - case "darwin": - mapping = append(mapping, &fleet.HostBattery{ - HostID: host.ID, - SerialNumber: row["serial_number"], - CycleCount: cycleCount, - // database type is VARCHAR(40) and since there isn't a - // canonical list of strings we can get for health, we - // truncate the value just in case. - Health: fmt.Sprintf("%.40s", row["health"]), - }) - case "windows": - health, err := generateWindowsBatteryHealth(row["designed_capacity"], row["max_capacity"]) - if err != nil { - level.Error(logger).Log("op", "directIngestBattery", "hostID", host.ID, "err", err) - } - - mapping = append(mapping, &fleet.HostBattery{ - HostID: host.ID, - SerialNumber: row["serial_number"], - CycleCount: cycleCount, - Health: health, - }) - } + mapping = append(mapping, &fleet.HostBattery{ + HostID: host.ID, + SerialNumber: row["serial_number"], + CycleCount: cycleCount, + Health: health, + }) } + return ds.ReplaceHostBatteries(ctx, host.ID, mapping) } const ( - batteryStatusUnknown = "Unknown" - batteryStatusDegraded = "Check Battery" - batteryStatusGood = "Good" - batteryDegradedThreshold = 80 + batteryStatusUnknown = "Unknown" + batteryStatusDegraded = "Service recommended" + batteryStatusGood = "Normal" + batteryDegradedThreshold = 80 + batteryDegradedCycleCount = 1000 ) -func generateWindowsBatteryHealth(designedCapacity, maxCapacity string) (string, error) { +// generateBatteryHealth calculates the battery health based on the cycle count and capacity. +func generateBatteryHealth(row map[string]string, logger log.Logger) (string, int, error) { + designedCapacity := row["designed_capacity"] + maxCapacity := row["max_capacity"] + cycleCount := row["cycle_count"] + + count, err := strconv.Atoi(EmptyToZero(cycleCount)) + if err != nil { + level.Error(logger).Log("op", "generateBatteryHealth", "err", err) + // If we can't parse the cycle count, we'll assume it's 0 + // and continue with the rest of the battery health check. + count = 0 + } + + if count >= batteryDegradedCycleCount { + return batteryStatusDegraded, count, nil + } + if designedCapacity == "" || maxCapacity == "" { - return batteryStatusUnknown, fmt.Errorf("missing battery capacity values, designed: %s, max: %s", designedCapacity, maxCapacity) + return batteryStatusUnknown, count, fmt.Errorf("missing battery capacity values, designed: %s, max: %s", designedCapacity, maxCapacity) } designed, err := strconv.ParseInt(designedCapacity, 10, 64) if err != nil { - return batteryStatusUnknown, err + return batteryStatusUnknown, count, fmt.Errorf("failed to parse designed capacity: %s", designedCapacity) } max, err := strconv.ParseInt(maxCapacity, 10, 64) if err != nil { - return batteryStatusUnknown, err + return batteryStatusUnknown, count, fmt.Errorf("failed to parse max capacity: %s", maxCapacity) } health := float64(max) / float64(designed) * 100 if health < batteryDegradedThreshold { - return batteryStatusDegraded, nil + return batteryStatusDegraded, count, nil } - return batteryStatusGood, nil + return batteryStatusGood, count, nil } func directIngestWindowsUpdateHistory( diff --git a/server/service/osquery_utils/queries_test.go b/server/service/osquery_utils/queries_test.go index 3593ff20f1a5..512a30208e2d 100644 --- a/server/service/osquery_utils/queries_test.go +++ b/server/service/osquery_utils/queries_test.go @@ -279,8 +279,7 @@ func TestGetDetailQueries(t *testing.T) { "mdm_windows", "munki_info", "google_chrome_profiles", - "battery_macos", - "battery_windows", + "battery", "os_windows", "os_unix_like", "os_chrome", @@ -297,7 +296,7 @@ func TestGetDetailQueries(t *testing.T) { sortedKeysCompare(t, queriesNoConfig, baseQueries) queriesWithoutWinOSVuln := GetDetailQueries(context.Background(), config.FleetConfig{Vulnerabilities: config.VulnerabilitiesConfig{DisableWinOSVulnerabilities: true}}, nil, nil) - require.Len(t, queriesWithoutWinOSVuln, 26) + require.Len(t, queriesWithoutWinOSVuln, 25) queriesWithUsers := GetDetailQueries(context.Background(), config.FleetConfig{App: config.AppConfig{EnableScheduledQueryStats: true}}, nil, &fleet.Features{EnableHostUsers: true}) qs := append(baseQueries, "users", "users_chrome", "scheduled_query_stats") @@ -975,58 +974,87 @@ func TestDirectIngestChromeProfiles(t *testing.T) { } func TestDirectIngestBattery(t *testing.T) { - ds := new(mock.Store) - ds.ReplaceHostBatteriesFunc = func(ctx context.Context, id uint, mappings []*fleet.HostBattery) error { - require.Equal(t, mappings, []*fleet.HostBattery{ - {HostID: uint(1), SerialNumber: "a", CycleCount: 2, Health: "Good"}, - {HostID: uint(1), SerialNumber: "c", CycleCount: 3, Health: strings.Repeat("z", 40)}, - }) - return nil - } - - host := fleet.Host{ - ID: 1, - Platform: "darwin", + tests := []struct { + name string + input map[string]string + expectedBattery *fleet.HostBattery + }{ + { + name: "max_capacity >= 80%, cycleCount < 1000", + input: map[string]string{"serial_number": "a", "cycle_count": "2", "designed_capacity": "3000", "max_capacity": "2400"}, + expectedBattery: &fleet.HostBattery{HostID: uint(1), SerialNumber: "a", CycleCount: 2, Health: batteryStatusGood}, + }, + { + name: "max_capacity < 50%", + input: map[string]string{"serial_number": "b", "cycle_count": "3", "designed_capacity": "3000", "max_capacity": "2399"}, + expectedBattery: &fleet.HostBattery{HostID: uint(1), SerialNumber: "b", CycleCount: 3, Health: batteryStatusDegraded}, + }, + { + name: "missing max_capacity", + input: map[string]string{"serial_number": "c", "cycle_count": "4", "designed_capacity": "3000", "max_capacity": ""}, + expectedBattery: &fleet.HostBattery{HostID: uint(1), SerialNumber: "c", CycleCount: 4, Health: batteryStatusUnknown}, + }, + { + name: "missing designed_capacity and max_capacity", + input: map[string]string{"serial_number": "d", "cycle_count": "5", "designed_capacity": "", "max_capacity": ""}, + expectedBattery: &fleet.HostBattery{HostID: uint(1), SerialNumber: "d", CycleCount: 5, Health: batteryStatusUnknown}, + }, + { + name: "missing designed_capacity", + input: map[string]string{"serial_number": "e", "cycle_count": "6", "designed_capacity": "", "max_capacity": "2000"}, + expectedBattery: &fleet.HostBattery{HostID: uint(1), SerialNumber: "e", CycleCount: 6, Health: batteryStatusUnknown}, + }, + { + name: "invalid designed_capacity and max_capacity", + input: map[string]string{"serial_number": "f", "cycle_count": "7", "designed_capacity": "foo", "max_capacity": "bar"}, + expectedBattery: &fleet.HostBattery{HostID: uint(1), SerialNumber: "f", CycleCount: 7, Health: batteryStatusUnknown}, + }, + { + name: "cycleCount >= 1000", + input: map[string]string{"serial_number": "g", "cycle_count": "1000", "designed_capacity": "3000", "max_capacity": "2400"}, + expectedBattery: &fleet.HostBattery{HostID: uint(1), SerialNumber: "g", CycleCount: 1000, Health: batteryStatusDegraded}, + }, + { + name: "cycleCount >= 1000 with degraded health", + input: map[string]string{"serial_number": "h", "cycle_count": "1001", "designed_capacity": "3000", "max_capacity": "2399"}, + expectedBattery: &fleet.HostBattery{HostID: uint(1), SerialNumber: "h", CycleCount: 1001, Health: batteryStatusDegraded}, + }, + { + name: "missing cycle_count", + input: map[string]string{"serial_number": "i", "cycle_count": "", "designed_capacity": "3000", "max_capacity": "2400"}, + expectedBattery: &fleet.HostBattery{HostID: uint(1), SerialNumber: "i", CycleCount: 0, Health: batteryStatusGood}, + }, + { + name: "missing cycle_count with degraded health", + input: map[string]string{"serial_number": "j", "cycle_count": "", "designed_capacity": "3000", "max_capacity": "2399"}, + expectedBattery: &fleet.HostBattery{HostID: uint(1), SerialNumber: "j", CycleCount: 0, Health: batteryStatusDegraded}, + }, + { + name: "invalid cycle_count", + input: map[string]string{"serial_number": "k", "cycle_count": "foo", "designed_capacity": "3000", "max_capacity": "2400"}, + expectedBattery: &fleet.HostBattery{HostID: uint(1), SerialNumber: "k", CycleCount: 0, Health: batteryStatusGood}, + }, } - err := directIngestBattery(context.Background(), log.NewNopLogger(), &host, ds, []map[string]string{ - {"serial_number": "a", "cycle_count": "2", "health": "Good"}, - {"serial_number": "c", "cycle_count": "3", "health": strings.Repeat("z", 100)}, - }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ds := new(mock.Store) - require.NoError(t, err) - require.True(t, ds.ReplaceHostBatteriesFuncInvoked) - - ds.ReplaceHostBatteriesFunc = func(ctx context.Context, id uint, mappings []*fleet.HostBattery) error { - require.Equal(t, mappings, []*fleet.HostBattery{ - {HostID: uint(2), SerialNumber: "a", CycleCount: 2, Health: batteryStatusGood}, - {HostID: uint(2), SerialNumber: "b", CycleCount: 3, Health: batteryStatusDegraded}, - {HostID: uint(2), SerialNumber: "c", CycleCount: 4, Health: batteryStatusUnknown}, - {HostID: uint(2), SerialNumber: "d", CycleCount: 5, Health: batteryStatusUnknown}, - {HostID: uint(2), SerialNumber: "e", CycleCount: 6, Health: batteryStatusUnknown}, - {HostID: uint(2), SerialNumber: "f", CycleCount: 7, Health: batteryStatusUnknown}, - }) - return nil - } + ds.ReplaceHostBatteriesFunc = func(ctx context.Context, id uint, mappings []*fleet.HostBattery) error { + require.Len(t, mappings, 1) + require.Equal(t, tt.expectedBattery, mappings[0]) + return nil + } - // reset the ds flag - ds.ReplaceHostBatteriesFuncInvoked = false + host := fleet.Host{ + ID: 1, + } - host = fleet.Host{ - ID: 2, - Platform: "windows", + err := directIngestBattery(context.Background(), log.NewNopLogger(), &host, ds, []map[string]string{tt.input}) + require.NoError(t, err) + require.True(t, ds.ReplaceHostBatteriesFuncInvoked) + }) } - - err = directIngestBattery(context.Background(), log.NewNopLogger(), &host, ds, []map[string]string{ - {"serial_number": "a", "cycle_count": "2", "designed_capacity": "3000", "max_capacity": "2400"}, // max_capacity >= 80% - {"serial_number": "b", "cycle_count": "3", "designed_capacity": "3000", "max_capacity": "2399"}, // max_capacity < 50% - {"serial_number": "c", "cycle_count": "4", "designed_capacity": "3000", "max_capacity": ""}, // missing max_capacity - {"serial_number": "d", "cycle_count": "5", "designed_capacity": "", "max_capacity": ""}, // missing designed_capacity and max_capacity - {"serial_number": "e", "cycle_count": "6", "designed_capacity": "", "max_capacity": "2000"}, // missing designed_capacity - {"serial_number": "f", "cycle_count": "7", "designed_capacity": "foo", "max_capacity": "bar"}, // invalid designed_capacity and max_capacity - }) - require.NoError(t, err) - require.True(t, ds.ReplaceHostBatteriesFuncInvoked) } func TestDirectIngestOSWindows(t *testing.T) {