mirror of
https://github.com/fleetdm/fleet
synced 2026-05-24 09:28:54 +00:00
5e7f5a7584
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #44609 # Details This PR optimizes the historical data collection system in two ways: 1. Adds an additional index on the `host_scd_data` table allowing more efficient lookups of rows by their `valid_to`, to optimize both closing out open rows and deleting old rows 2. Implements batching in the job that deletes old rows, so that it no longer blocks writes if the collection job happens to happen at the same time as the cleanup job # Checklist for submitter If some of the following don't apply, delete the relevant line. - [ ] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. n/a, unreleased - [X] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [ ] Timeouts are implemented and retries are limited to avoid infinite loops ## Testing - [ ] Added/updated automated tests - [X] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [X] QA'd all new/changed functionality manually SQL explains -- before: ``` +----+-------------+---------------+------------+------+---------------+------+---------+------+--------+----------+-------------+ | id | select_type | table | partitions | type | possible_keys | key | key_len | ref | rows | filtered | Extra | +----+-------------+---------------+------------+------+---------------+------+---------+------+--------+----------+-------------+ | 1 | DELETE | host_scd_data | NULL | ALL | NULL | NULL | NULL | NULL | 144320 | 100.00 | Using where | +----+-------------+---------------+------------+------+---------------+------+---------+------+--------+----------+-------------+ +----+-------------+---------------+------------+-------+--------------------------------------+--------------------+---------+-------------+------+----------+-------------+ | id | select_type | table | partitions | type | possible_keys | key | key_len | ref | rows | filtered | Extra | +----+-------------+---------------+------------+-------+--------------------------------------+--------------------+---------+-------------+------+----------+-------------+ | 1 | UPDATE | host_scd_data | NULL | range | uniq_entity_bucket,idx_dataset_range | uniq_entity_bucket | 604 | const,const | 3030 | 100.00 | Using where | +----+-------------+---------------+------------+-------+--------------------------------------+--------------------+---------+-------------+------+----------+-------------+ ``` Using a test set of data (~144k "open" rows), UPDATES happened at 9 ops per second. after: ``` +----+-------------+---------------+------------+-------+----------------------+----------------------+---------+-------+-------+----------+-------------+ | id | select_type | table | partitions | type | possible_keys | key | key_len | ref | rows | filtered | Extra | +----+-------------+---------------+------------+-------+----------------------+----------------------+---------+-------+-------+----------+-------------+ | 1 | DELETE | host_scd_data | NULL | range | idx_valid_to_dataset | idx_valid_to_dataset | 5 | const | 55749 | 100.00 | Using where | +----+-------------+---------------+------------+-------+----------------------+----------------------+---------+-------+-------+----------+-------------+ +----+-------------+---------------+------------+-------+-----------------------------------------------------------+----------------------+---------+-------------------+------+----------+------------------------------+ | id | select_type | table | partitions | type | possible_keys | key | key_len | ref | rows | filtered | Extra | +----+-------------+---------------+------------+-------+-----------------------------------------------------------+----------------------+---------+-------------------+------+----------+------------------------------+ | 1 | UPDATE | host_scd_data | NULL | range | uniq_entity_bucket,idx_dataset_range,idx_valid_to_dataset | idx_valid_to_dataset | 609 | const,const,const | 4 | 100.00 | Using where; Using temporary | +----+-------------+---------------+------------+-------+-----------------------------------------------------------+----------------------+---------+-------------------+------+----------+------------------------------+ ``` Using the same test set of data, UPDATES happened at 4,910 ops per second. For unreleased bug fixes in a release candidate, one of: - [X] Confirmed that the fix is not expected to adversely impact load test results this should significantly improve results! - [ ] Alerted the release DRI if additional load testing is needed ## Database migrations - [X] Checked schema for all modified table for columns that will auto-update timestamps during migration. - [ ] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. - [ ] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Cleanup now runs in controlled, ordered batches, removing only closed/historical records while respecting cancellation; error reporting for cleanup was strengthened. * Added a new composite index on historical data to improve cleanup and query performance. * **Tests** * Added tests and test helpers validating batched cleanup behavior, preservation of open records, multi-batch operation, and cancellation handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
115 lines
2.9 KiB
Go
115 lines
2.9 KiB
Go
package chart_test
|
|
|
|
import (
|
|
"regexp"
|
|
"slices"
|
|
"testing"
|
|
|
|
"github.com/fleetdm/fleet/v4/server/archtest"
|
|
)
|
|
|
|
const m = archtest.ModuleName
|
|
|
|
var (
|
|
fleetDeps = regexp.MustCompile(`^github\.com/fleetdm/`)
|
|
|
|
// Common allowed dependencies across chart packages.
|
|
chartPkgs = []string{
|
|
m + "/server/chart",
|
|
m + "/server/chart/api",
|
|
m + "/server/chart/api/http",
|
|
m + "/server/chart/internal/types",
|
|
}
|
|
|
|
platformPkgs = []string{
|
|
m + "/server/platform/...",
|
|
m + "/server/contexts/...",
|
|
m + "/pkg/fleethttp",
|
|
m + "/pkg/str",
|
|
}
|
|
)
|
|
|
|
// TestChartPackageDependencies runs architecture tests for all chart packages.
|
|
// Each package has specific rules about what dependencies are allowed.
|
|
func TestChartPackageDependencies(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
cases := []struct {
|
|
name string
|
|
pkg string
|
|
shouldNotDepend []string // defaults to m + "/..." if empty
|
|
ignoreDeps []string
|
|
}{
|
|
{
|
|
// Root package only depends on api (for dataset implementations).
|
|
name: "root package only depends on api",
|
|
pkg: m + "/server/chart",
|
|
ignoreDeps: []string{m + "/server/chart/api"},
|
|
},
|
|
{
|
|
name: "api package has no Fleet dependencies",
|
|
pkg: m + "/server/chart/api",
|
|
},
|
|
{
|
|
name: "api/http only depends on api",
|
|
pkg: m + "/server/chart/api/http",
|
|
ignoreDeps: []string{m + "/server/chart/api"},
|
|
},
|
|
{
|
|
name: "internal/types only depends on api",
|
|
pkg: m + "/server/chart/internal/types",
|
|
ignoreDeps: []string{m + "/server/chart/api"},
|
|
},
|
|
{
|
|
name: "internal/mysql depends on chart, types, and platform",
|
|
pkg: m + "/server/chart/internal/mysql",
|
|
ignoreDeps: slices.Concat(chartPkgs, platformPkgs, []string{
|
|
m + "/server/chart/internal/testutils",
|
|
}),
|
|
},
|
|
{
|
|
name: "internal/service depends on chart and platform packages",
|
|
pkg: m + "/server/chart/internal/service",
|
|
ignoreDeps: slices.Concat(chartPkgs, platformPkgs),
|
|
},
|
|
{
|
|
name: "bootstrap depends on chart and platform packages",
|
|
pkg: m + "/server/chart/bootstrap",
|
|
ignoreDeps: slices.Concat([]string{
|
|
m + "/server/chart/internal/mysql",
|
|
m + "/server/chart/internal/service",
|
|
}, chartPkgs, platformPkgs),
|
|
},
|
|
{
|
|
name: "all packages only depend on chart and platform",
|
|
pkg: m + "/server/chart/...",
|
|
ignoreDeps: slices.Concat([]string{
|
|
m + "/server/chart/internal/mysql",
|
|
m + "/server/chart/internal/service",
|
|
m + "/server/chart/internal/testutils",
|
|
}, chartPkgs, platformPkgs),
|
|
},
|
|
}
|
|
|
|
for _, tc := range cases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
shouldNotDepend := tc.shouldNotDepend
|
|
if len(shouldNotDepend) == 0 {
|
|
shouldNotDepend = []string{m + "/..."}
|
|
}
|
|
|
|
test := archtest.NewPackageTest(t, tc.pkg).
|
|
OnlyInclude(fleetDeps).
|
|
ShouldNotDependOn(shouldNotDepend...).
|
|
WithTests()
|
|
|
|
if len(tc.ignoreDeps) > 0 {
|
|
test.IgnoreDeps(tc.ignoreDeps...)
|
|
}
|
|
|
|
test.Check()
|
|
})
|
|
}
|
|
}
|