OSAC-3848: fix started.v1/resumed.v1 mapping on first activation - #227
Conversation
…tivation Transition tables mapped every non-empty previous state crossing into billable to resumed.v1, including a resource's genuine first activation. Resolve started vs resumed from a persisted EverBillable flag instead. Jira: OSAC-3848 Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
|
@omer-vishlitzky: This pull request references OSAC-3848 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
WalkthroughThe metering service now persists cumulative billability in resource state. Event mapping uses that history to emit ChangesCumulative billability tracking
Database API cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WatchConsumer
participant ProjectionPostgres
participant MapWatchEvent
participant EventStream
WatchConsumer->>ProjectionPostgres: Read ResourceState.EverBillable
ProjectionPostgres-->>WatchConsumer: Return cumulative billability
WatchConsumer->>ProjectionPostgres: Upsert updated EverBillable
WatchConsumer->>MapWatchEvent: Pass StateContext.EverBillable
MapWatchEvent->>EventStream: Emit started.v1 or resumed.v1
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
osac-metering/metering-service/internal/projection/postgres.go (1)
100-100: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider enforcing monotonicity of
ever_billablein SQL.
ever_billable = EXCLUDED.ever_billableallows a caller that computesfalseto erase a storedtrue. TodaybuildProjectionStateORs with the existing value, so the invariant holds. That invariant lives only in Go, and a single caller that builds aResourceStatewithout reading the current row will silently downgrade billing history. A downgrade later re-emitsstarted.v1for a resource that already started.Making the column monotonic in the statement removes the risk for every caller.
♻️ Proposed hardening
- ever_billable = EXCLUDED.ever_billable, + ever_billable = metering_resource_state.ever_billable OR EXCLUDED.ever_billable,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@osac-metering/metering-service/internal/projection/postgres.go` at line 100, Update the UPSERT assignment for ever_billable to preserve the stored true value, combining the existing database value with EXCLUDED.ever_billable rather than replacing it. Keep the column monotonic so once it is true, later writes cannot downgrade it to false.osac-metering/metering-service/internal/watch/consumer_test.go (1)
988-1033: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider extending this test to cover the full start-suspend-resume cycle.
This test proves
started.v1on first boot. The test at line 653 provesresumed.v1from a pre-seededEverBillable: true. Neither test proves the seam between them: that the consumer writesEverBillable = trueitself atbuildProjectionState, that the value survives in the store, and that a later reactivation then reads it back asresumed.v1.A regression in the accumulator at
consumer.goline 424 — for example dropping the|| existing.EverBillableterm — passes both current tests. Two more events on this stream close the gap and exercise the real store path.💚 Suggested extension
runningEvent := &privatev1.Event{ Id: "evt-running", Type: privatev1.EventType_EVENT_TYPE_OBJECT_UPDATED, Payload: &privatev1.Event_ComputeInstance{ComputeInstance: runningCI}, } + stoppedCI := makeComputeInstance("vm-fresh", "tenant-1") + stoppedCI.Status.State = privatev1.ComputeInstanceState_COMPUTE_INSTANCE_STATE_STOPPED + stoppedCI.Metadata.Version = 3 + stoppedEvent := &privatev1.Event{ + Id: "evt-stopped", + Type: privatev1.EventType_EVENT_TYPE_OBJECT_UPDATED, + Payload: &privatev1.Event_ComputeInstance{ComputeInstance: stoppedCI}, + } + + restartedCI := makeComputeInstance("vm-fresh", "tenant-1") + restartedCI.Status.State = privatev1.ComputeInstanceState_COMPUTE_INSTANCE_STATE_RUNNING + restartedCI.Metadata.Version = 4 + restartedEvent := &privatev1.Event{ + Id: "evt-restarted", + Type: privatev1.EventType_EVENT_TYPE_OBJECT_UPDATED, + Payload: &privatev1.Event_ComputeInstance{ComputeInstance: restartedCI}, + } + stream := &mockWatchStream{ responses: []*privatev1.EventsWatchResponse{ makeResponse(createEvent), makeResponse(runningEvent), + makeResponse(stoppedEvent), + makeResponse(restartedEvent), }, } client.results = []mockStreamResult{{stream: stream}} - pub := &mockPublisher{published: make([]cloudevents.Event, 0, 2), cancelFunc: cancel} + pub := &mockPublisher{published: make([]cloudevents.Event, 0, 4), cancelFunc: cancel} consumer := newConsumerWithStore(pub, store) err := consumer.Run(ctx) Expect(err).ToNot(HaveOccurred()) pub.mu.Lock() defer pub.mu.Unlock() - Expect(pub.published).To(HaveLen(2)) + Expect(pub.published).To(HaveLen(4)) Expect(pub.published[0].Type()).To(Equal(events.EventCreated)) Expect(pub.published[1].Type()).To(Equal(events.EventStarted), "first-ever activation of a brand-new resource must be started.v1, not resumed.v1") + Expect(pub.published[2].Type()).To(Equal(events.EventSuspended)) + Expect(pub.published[3].Type()).To(Equal(events.EventResumed), + "reactivation must be resumed.v1 once the consumer has recorded EverBillable") })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@osac-metering/metering-service/internal/watch/consumer_test.go` around lines 988 - 1033, Extend the test around newConsumerWithStore and its mock stream to cover the complete lifecycle: after the initial STARTING→RUNNING events, add suspension and subsequent reactivation events, then assert the first activation is started.v1 and the later activation is resumed.v1. Use the same store and consumer throughout so buildProjectionState persists EverBillable=true and the later event reads that value back through the real store path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@osac-metering/metering-service/internal/database/migrations/0_create_metering_resource_state.up.sql`:
- Line 16: Confirm migration 0 has not been applied to existing databases;
regardless, add a new forward migration for metering_resource_state that adds
the non-null ever_billable column with a false default and backfills it to true
where is_billable is true. Keep the column definition in the initial migration
aligned with this schema.
In `@osac-metering/metering-service/internal/reconciliation/reconciler.go`:
- Line 176: Update the StateDrift branch to set ps.EverBillable alongside
ps.IsBillable, using the existing wasBillable and isBillable state transition
logic so a drift into a billable state marks the resource as ever billable and
preserves resumed.v1 behavior.
---
Nitpick comments:
In `@osac-metering/metering-service/internal/projection/postgres.go`:
- Line 100: Update the UPSERT assignment for ever_billable to preserve the
stored true value, combining the existing database value with
EXCLUDED.ever_billable rather than replacing it. Keep the column monotonic so
once it is true, later writes cannot downgrade it to false.
In `@osac-metering/metering-service/internal/watch/consumer_test.go`:
- Around line 988-1033: Extend the test around newConsumerWithStore and its mock
stream to cover the complete lifecycle: after the initial STARTING→RUNNING
events, add suspension and subsequent reactivation events, then assert the first
activation is started.v1 and the later activation is resumed.v1. Use the same
store and consumer throughout so buildProjectionState persists EverBillable=true
and the later event reads that value back through the real store path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5443e539-fcec-40e8-a537-451963f812b3
📒 Files selected for processing (13)
osac-metering/metering-service/internal/database/container.goosac-metering/metering-service/internal/database/migrations/0_create_metering_resource_state.up.sqlosac-metering/metering-service/internal/events/cluster.goosac-metering/metering-service/internal/events/cluster_test.goosac-metering/metering-service/internal/events/compute_instance.goosac-metering/metering-service/internal/events/mapper.goosac-metering/metering-service/internal/events/mapper_test.goosac-metering/metering-service/internal/events/transitions.goosac-metering/metering-service/internal/projection/postgres.goosac-metering/metering-service/internal/projection/types.goosac-metering/metering-service/internal/reconciliation/reconciler.goosac-metering/metering-service/internal/watch/consumer.goosac-metering/metering-service/internal/watch/consumer_test.go
💤 Files with no reviewable changes (1)
- osac-metering/metering-service/internal/database/container.go
Reconciler's state-drift path set IsBillable without EverBillable, same bug reintroduced through a different path. Made the Postgres upsert monotonic as a backstop. Extended the consumer test to cover a full start-suspend-resume cycle through the real store. Jira: OSAC-3848 Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Following up on the discussion about reconciling The invariant already holds: every write path in this PR sets is_billable BOOLEAN GENERATED ALWAYS AS (billable_since IS NOT NULL) STOREDThis removes
ON CONFLICT (resource_id) DO UPDATE SET
...
ever_billable = metering_resource_state.ever_billable OR (EXCLUDED.billable_since IS NOT NULL),
...vs. the current: ever_billable = metering_resource_state.ever_billable OR EXCLUDED.ever_billable,The difference matters: the current formula still trusts whatever Happy to open this as a fast-follow PR if useful — didn't want to push a commit onto this branch without checking first. |
| current_state TEXT NOT NULL, | ||
| previous_state TEXT, | ||
| is_billable BOOLEAN NOT NULL DEFAULT FALSE, | ||
| ever_billable BOOLEAN NOT NULL DEFAULT FALSE, |
There was a problem hiding this comment.
This edit is on an existing migration (0_create_...). I assume that we can make this manipulation on an existing migration as we're pre-production? Is there a case for adding another migration for DB "version management"?
There was a problem hiding this comment.
I would say no, we don't want to add more migration files if there is nothing to migrate.
| return nil | ||
| } | ||
|
|
||
| func (i *Instance) URL(ctx context.Context) (string, error) { |
There was a problem hiding this comment.
Is this related to the changes introduced in this PR?
There was a problem hiding this comment.
no, just dead code
is_billable is now a GENERATED column, never independently written. ever_billable derives from billable_since on both insert and conflict update instead of trusting a caller's own computed value, so it self-heals even if a caller gets it wrong (per masayag's review). Jira: OSAC-3848 Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (1)
osac-metering/metering-service/internal/projection/postgres_test.go (1)
151-164: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the
ON CONFLICTbranch in this regression test.This test inserts a new row. It covers only the INSERT expression in
osac-metering/metering-service/internal/projection/postgres.go. It does not execute theON CONFLICTexpression that preserves or derivesever_billable.Add a case that first stores a non-billable row, then upserts the same resource with
BillableSinceset. Assert thatEverBillablebecomes true and remains true after a later non-billable update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@osac-metering/metering-service/internal/projection/postgres_test.go` around lines 151 - 164, The regression test around the “self-heals ever_billable” case only exercises insertion; extend it to execute the Upsert conflict path by first storing a non-billable state, then upserting the same resource with BillableSince set and asserting EverBillable is true, followed by a later non-billable upsert that still leaves EverBillable true. Use the existing makeState, store.Upsert, and store.Get flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@osac-metering/metering-service/internal/projection/postgres_test.go`:
- Around line 151-164: The regression test around the “self-heals ever_billable”
case only exercises insertion; extend it to execute the Upsert conflict path by
first storing a non-billable state, then upserting the same resource with
BillableSince set and asserting EverBillable is true, followed by a later
non-billable upsert that still leaves EverBillable true. Use the existing
makeState, store.Upsert, and store.Get flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8baeff9c-7a00-4525-bde2-3834de0551dd
📒 Files selected for processing (6)
osac-metering/metering-service/internal/database/migrations/0_create_metering_resource_state.up.sqlosac-metering/metering-service/internal/projection/postgres.goosac-metering/metering-service/internal/projection/postgres_test.goosac-metering/metering-service/internal/reconciliation/reconciler.goosac-metering/metering-service/internal/reconciliation/reconciler_test.goosac-metering/metering-service/internal/watch/consumer_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- osac-metering/metering-service/internal/watch/consumer_test.go
- osac-metering/metering-service/internal/projection/postgres.go
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: masayag, omer-vishlitzky The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Watch Consumer mapped every non-empty previous state crossing into billable to
resumed.v1, including a resource's genuine first activation — CREATE always seeds a concrete state before the billable transition fires, so thestarted.v1table row was unreachable. Resolves started vs resumed from a persistedEverBillableflag instead.Jira: https://redhat.atlassian.net/browse/OSAC-3848
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
New Features
startedorresumedstates.Bug Fixes