fix: delete rule in planned maintenance #9245
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 13884cc in 6 minutes and 9 seconds. Click for details.
- Reviewed
48lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. pkg/query-service/rules/manager.go:485
- Draft comment:
Use a consistent UUID-to-string conversion method. Here you use id.StringValue() in some places and id.String() in others. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. pkg/ruler/rulestore/sqlrulestore/maintenance.go:227
- Draft comment:
The new DeletePlannedMaintenanceRulesByRuleID function is implemented correctly. Consider adding logging of the deletion outcome for easier debugging. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
3. pkg/types/ruletypes/maintenance.go:340
- Draft comment:
Document the purpose and usage of the new DeletePlannedMaintenanceRulesByRuleID method in the MaintenanceStore interface for clarity. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
Workflow ID: wflow_IPjpLiozR1FX5DYX
You can customize 
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 0634a88 in 1 minute and 36 seconds. Click for details.
- Reviewed
196lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. pkg/query-service/rules/manager_test.go:816
- Draft comment:
TestDeleteRule: Good table‐driven deletion test covering planned maintenance cleanup. Consider asserting that no orphaned planned maintenance records remain (if possible) to strengthen the verification. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment suggests a potentially valid test improvement, but there are several issues: 1) It's speculative - we don't see evidence that orphaned maintenance records are actually a concern 2) It's not clearly actionable without more context about the maintenance system 3) The comment uses "if possible" which makes it uncertain 4) The test is already verifying the core deletion contract through mocks. The suggestion could catch real bugs if orphaned maintenance records are a known issue in the system. I may be dismissing a valid testing concern. While orphaned records could be an issue, the comment is too speculative without evidence. A good comment would need to demonstrate that this is a real problem and provide clear guidance on how to test for it. The comment should be deleted as it makes a speculative suggestion without clear evidence of the problem or actionable guidance on implementing the test.
Workflow ID: wflow_CwRcqZPFZwI88hls
You can customize
|
|
||
| // ExpectDeleteRule sets up SQL expectations for DeleteRule operation | ||
| func (m *MockSQLRuleStore) ExpectDeleteRule(ruleID valuer.UUID) { | ||
| plannedMaintenancePattern := `DELETE FROM "planned_maintenance_rule".+WHERE \(rule_id = '` + ruleID.StringValue() + `'\)` |
There was a problem hiding this comment.
ExpectDeleteRule now expects deletion from the 'planned_maintenance_rule' table. Please add an inline comment explaining that this is done to work around the lack of cascade delete for foreign key constraints.
| plannedMaintenancePattern := `DELETE FROM "planned_maintenance_rule".+WHERE \(rule_id = '` + ruleID.StringValue() + `'\)` | |
| plannedMaintenancePattern := `DELETE FROM "planned_maintenance_rule".+WHERE (rule_id = '` + ruleID.StringValue() + `')` // Workaround: no cascade delete for FK constraints, so must delete explicitly |
|
|
||
| func (r *rule) DeleteRule(ctx context.Context, id valuer.UUID, cb func(context.Context) error) error { | ||
| if err := r.sqlstore.RunInTxCtx(ctx, nil, func(ctx context.Context) error { | ||
| _, err := r.sqlstore. |
There was a problem hiding this comment.
In DeleteRule, add a comment explaining that the deletion of planned maintenance records is performed first to avoid foreign key constraint errors due to lack of cascade delete.
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes a foreign key constraint issue when deleting rules that are part of planned maintenance. The fix removes the rule from the planned_maintenance_rule table before deleting the rule itself.
- Adds deletion of planned maintenance rule associations before rule deletion
- Updates test expectations to include the new planned maintenance deletion step
- Adds comprehensive test coverage for rule deletion with planned maintenance
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/ruler/rulestore/sqlrulestore/rule.go | Adds deletion from planned_maintenance_rule table before deleting the rule |
| pkg/ruler/rulestore/rulestoretest/rule.go | Updates mock expectations to include planned maintenance rule deletion |
| pkg/query-service/rules/manager_test.go | Adds comprehensive test cases for rule deletion scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| BunDBCtx(ctx). | ||
| NewDelete(). | ||
| Model(new(ruletypes.StorablePlannedMaintenanceRule)). | ||
| Where("rule_id = ?", id.StringValue()). | ||
| Exec(ctx) |
There was a problem hiding this comment.
The deletion from planned_maintenance_rule table should handle the case where no rows are affected, as this is expected when a rule has no planned maintenance associations. Consider storing the result and checking if it's acceptable to have 0 affected rows.
Copilot uses AI. Check for mistakes.
|
I created two issues #9270 and #9272. The planned downtime will be part of alertmanager work and shares the same spirit that of the route policy i.e downtime based on the set of labels and ruleId as a label to suppress a specific. Eventually, we will get rid of the table with the flattened rules mapping and instead communicate with AM do schedule downtimes directly. |
|
2 participants
📄 Summary
Currently Rule which our part of planned maintenance give error on delete due to foreign key constraint on planned maintenance table, so we added functionality to remove it from the table first then on deleting the rule.
The problem arises because there is no delete cascade on rule foreign constraint in planned maintenance table, and it would require migration, with sqlite we need to delete and create the table again.
Important
Fixes rule deletion with planned maintenance by removing from
planned_maintenance_ruletable first.planned_maintenance_ruletable inDeleteRule()insqlrulestore/rule.go.TestDeleteRuleinmanager_test.goto verify rule deletion with planned maintenance.ExpectDeleteRuleinrulestoretest/rule.goto expect deletion fromplanned_maintenance_ruletable.This description was created by
for 0634a88. You can customize this summary. It will automatically update as commits are pushed.