OSAC-3410: Add CaaS lifecycle metering with N+1 per-component billing - #172
Merged
omer-vishlitzky merged 19 commits intoAug 9, 2026
Conversation
|
@omer-vishlitzky: This pull request references OSAC-3410 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 epic 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. |
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 05:43 — with 
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 05:43 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 05:43 — with
GitHub Actions
Error
GitHub Actions
Error
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe metering service now supports CaaS cluster events and component-level lifecycle, scaling, heartbeat, and correction events. Reconciliation loads clusters through a new client, while watch and heartbeat paths publish decomposed component events. ChangesCluster metering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WatchStream
participant WatchConsumer
participant ClusterMapper
participant Projection
participant EventPublisher
WatchStream->>WatchConsumer: cluster event
WatchConsumer->>ClusterMapper: map cluster and detect transition
ClusterMapper-->>WatchConsumer: component records and event metadata
WatchConsumer->>EventPublisher: publish component lifecycle or scaling events
WatchConsumer->>Projection: update cluster projection
Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 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: 6
🧹 Nitpick comments (6)
osac-metering/metering-service/internal/watch/consumer_test.go (1)
375-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe filter assertion is updated, but no test exercises the cluster path it enables.
This suite now admits cluster events through the filter, yet every scenario still uses a compute instance. The new behaviour in
osac-metering/metering-service/internal/watch/consumer.gois untested here:
publishLifecycleEventsfan-out: one clusterstarted.v1should produce N+1 published events with flattenedbilling_dimensions.handleScalingEvent: aPROGRESSING→READYtransition with a changed node set size should publishupdated.v1for the changed component only.Add both cases.
🤖 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 375 - 397, Add consumer tests covering cluster events enabled by the updated watch filter: verify one cluster started.v1 event is fanned out by publishLifecycleEvents into N+1 published events with flattened billing_dimensions, and verify handleScalingEvent publishes updated.v1 only for the component whose node-set size changes across a PROGRESSING-to-READY transition. Use the existing mock publisher, watch stream, and event fixtures established in this test suite.osac-metering/metering-service/internal/events/cluster_test.go (1)
462-553: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for duplicate
host_typeand for non-FAILEDrecovery.The suite covers single-node-set changes well. Two gaps remain, both matching issues raised in
osac-metering/metering-service/internal/events/cluster.go:
- Two node sets with the same
host_typeand different sizes.ChangedComponentskeys oncomponent + ":" + host_type, so the records collapse.DELETINGorDELETE_FAILED→READYin the transition matrix at Lines 45-95. That path currently returnsosac.resource.updated.v1.Add both cases so the fixes stay locked in.
🤖 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/events/cluster_test.go` around lines 462 - 553, Add tests in the ChangedComponents suite for two worker node sets sharing the same host_type but having different node counts, asserting both records remain distinct rather than collapsing. Also add a transition test covering DELETING and DELETE_FAILED recovering to READY, asserting the result is osac.resource.updated.v1, using the existing transition-matrix test structure and symbols.osac-metering/metering-service/internal/reconciliation/reconciler.go (1)
264-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe cluster skip is silent.
The guard is correct: without
clusterClient,fulfillmentStatenever holds clusters, so every cluster projection would look deleted. But ifclusterClientis nil by misconfiguration, cluster deletion reconciliation stops with no signal at all.Log once per run when the reconciler runs without a cluster lister.
🤖 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/reconciliation/reconciler.go` around lines 264 - 266, Add a once-per-reconciliation-run warning when the reconciler lacks a cluster lister, before or alongside the cluster skip guard in the reconciliation method. Ensure the warning is emitted once per run—not once per cluster projection—while preserving the existing continue behavior for cluster_order resources when r.clusterClient is nil.osac-metering/metering-service/internal/heartbeat/generator.go (1)
156-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
buildComponentHeartbeatduplicatesbuildHeartbeatEvent.Only two things differ: the event ID and
BillingDimensions. Everything else — source, type, time, extensions, duration, schema version — is copied. Take the ID and the dimensions as parameters on the existing builder instead.♻️ Proposed shape
-func (g *Generator) buildHeartbeatEvent(state *projection.ResourceState, now time.Time) (cloudevents.Event, error) { +func (g *Generator) buildHeartbeatEvent(state *projection.ResourceState, eventID string, dims map[string]any, now time.Time) (cloudevents.Event, error) { ce := cloudevents.NewEvent() - ce.SetID(uuid.NewString()) + ce.SetID(eventID)🤖 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/heartbeat/generator.go` around lines 156 - 184, Update the existing buildHeartbeatEvent builder to accept the event ID and BillingDimensions as parameters, then reuse it from buildComponentHeartbeat instead of duplicating event construction. Preserve the shared source, type, time, extensions, duration, schema version, data-setting, and error-handling behavior while passing the component-specific values.osac-metering/metering-service/internal/reconciliation/reconciler_test.go (1)
162-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEvery call site passes
nil, so no cluster reconciliation path is tested.The mechanical update is correct, but it leaves the entire new surface uncovered:
loadClusterspagination,isBillableForTypeforcluster_order, component-decomposed corrections, and theclusterClient == nilskip inreconcileMissedDeletions.Add a
mockClusterClientand at least these cases:
- a cluster present in fulfillment but absent from the projection, asserting N+1 correction events;
- a cluster projection with
clusterClientset to nil, asserting nomissed_deletionis emitted.A small
newReconciler(client, store, pub)helper would also remove the barenilfrom 15 call sites.🤖 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/reconciliation/reconciler_test.go` at line 162, Expand reconciler tests by adding a mockClusterClient and a newReconciler(client, store, pub) helper, then replace the existing bare nil cluster-client arguments at all call sites. Add coverage for loadClusters pagination, cluster_order billability, component-decomposed corrections, a fulfillment cluster missing from the projection producing N+1 correction events, and a cluster projection with nil clusterClient producing no missed_deletion event.osac-metering/metering-service/internal/watch/consumer.go (1)
299-312: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe scaling payload duplicates
meteringDataas an untyped map.
events.MapWatchEventbuilds the same payload from themeteringDatastruct with JSON tags. This function rebuilds the identical field set by hand. Any future change tometeringData(a renamed field, a newschema_version) will silently skip this path, and the two producers will emit different shapes on the same topic.Export a shared builder in the
eventspackage and call it from both places.🤖 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.go` around lines 299 - 312, The scaling payload construction near the data map must stop duplicating meteringData fields manually. Add an exported shared builder in the events package that produces the tagged meteringData payload, update events.MapWatchEvent and the scaling path to call it, and preserve the existing field values and serialized shape for both producers.
🤖 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 @.github/workflows/build-metering-test-adapter-image.yaml:
- Around line 51-54: Update the version-tagging step around RELEASE_VERSION,
RELEASE_MAJOR_MINOR, and RELEASE_MAJOR so rolling tags are written only when
version has no prerelease suffix. Continue exporting RELEASE_VERSION for all
valid versions, but guard the major/minor tag assignments and corresponding
image publishing so values like v1.2.3-rc.1 cannot produce or overwrite v1.2 or
v1 tags.
- Around line 26-40: Update the workflow so the build job uses only read
permissions, while `packages: write` is granted exclusively to a separate
publish-only job. In the `actions/checkout` step, set `persist-credentials` to
false, and verify `.dockerignore` excludes `.git` so repository metadata is not
included in the Docker build context.
In `@osac-metering/metering-service/internal/events/cluster.go`:
- Around line 110-123: Update the transition switch to classify every
non-billable-to-billable transition as osac.resource.resumed.v1 by using the
computed previousBillable value, while preserving the existing started.v1
handling for an empty previous state and other transition outcomes.
- Around line 185-201: Extend ComponentRecord with a NodeSet field, populate it
from each node-set map key in DecomposeClusterComponents, and include node_set
in the identity key used by ComponentEventID and ChangedComponents. Preserve
deterministic ordering and ensure the key format is component + ":" + node_set +
":" + host_type so distinct node sets cannot collide.
In `@osac-metering/metering-service/internal/reconciliation/correction.go`:
- Around line 76-91: Make component event IDs deterministic per logical emission
so replay deduplication works: in
osac-metering/metering-service/internal/reconciliation/correction.go lines
76-91, derive one base ID from resourceID, reason, and now before the component
loop and reuse it with ComponentEventID; in
osac-metering/metering-service/internal/reconciliation/reconciler.go lines
467-487, update buildSyntheticHeartbeats to derive the base from ps.ResourceID
and now instead of uuid.NewString(); in
osac-metering/metering-service/internal/heartbeat/generator.go lines 105-117,
update buildComponentHeartbeat to derive it from state.ResourceID and now
instead of uuid.NewString().
In `@osac-metering/metering-service/internal/watch/consumer.go`:
- Around line 199-209: Update Consumer.publishLifecycleEvents so cluster_order
delete events do not take the base-event early return; retain that behavior for
created events and non-cluster resources. Route deleted events through
DecomposeClusterComponents and the existing per-component publishing path,
preserving one flattened event for each component while leaving other lifecycle
transitions unchanged.
---
Nitpick comments:
In `@osac-metering/metering-service/internal/events/cluster_test.go`:
- Around line 462-553: Add tests in the ChangedComponents suite for two worker
node sets sharing the same host_type but having different node counts, asserting
both records remain distinct rather than collapsing. Also add a transition test
covering DELETING and DELETE_FAILED recovering to READY, asserting the result is
osac.resource.updated.v1, using the existing transition-matrix test structure
and symbols.
In `@osac-metering/metering-service/internal/heartbeat/generator.go`:
- Around line 156-184: Update the existing buildHeartbeatEvent builder to accept
the event ID and BillingDimensions as parameters, then reuse it from
buildComponentHeartbeat instead of duplicating event construction. Preserve the
shared source, type, time, extensions, duration, schema version, data-setting,
and error-handling behavior while passing the component-specific values.
In `@osac-metering/metering-service/internal/reconciliation/reconciler_test.go`:
- Line 162: Expand reconciler tests by adding a mockClusterClient and a
newReconciler(client, store, pub) helper, then replace the existing bare nil
cluster-client arguments at all call sites. Add coverage for loadClusters
pagination, cluster_order billability, component-decomposed corrections, a
fulfillment cluster missing from the projection producing N+1 correction events,
and a cluster projection with nil clusterClient producing no missed_deletion
event.
In `@osac-metering/metering-service/internal/reconciliation/reconciler.go`:
- Around line 264-266: Add a once-per-reconciliation-run warning when the
reconciler lacks a cluster lister, before or alongside the cluster skip guard in
the reconciliation method. Ensure the warning is emitted once per run—not once
per cluster projection—while preserving the existing continue behavior for
cluster_order resources when r.clusterClient is nil.
In `@osac-metering/metering-service/internal/watch/consumer_test.go`:
- Around line 375-397: Add consumer tests covering cluster events enabled by the
updated watch filter: verify one cluster started.v1 event is fanned out by
publishLifecycleEvents into N+1 published events with flattened
billing_dimensions, and verify handleScalingEvent publishes updated.v1 only for
the component whose node-set size changes across a PROGRESSING-to-READY
transition. Use the existing mock publisher, watch stream, and event fixtures
established in this test suite.
In `@osac-metering/metering-service/internal/watch/consumer.go`:
- Around line 299-312: The scaling payload construction near the data map must
stop duplicating meteringData fields manually. Add an exported shared builder in
the events package that produces the tagged meteringData payload, update
events.MapWatchEvent and the scaling path to call it, and preserve the existing
field values and serialized shape for both producers.
🪄 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: 87c672d3-0b23-4c1d-bb89-d29bff00d787
📒 Files selected for processing (13)
.github/workflows/build-metering-test-adapter-image.yaml.github/workflows/e2e-vmaas-full-install.ymlosac-metering/metering-service/cmd/metering-service/main.goosac-metering/metering-service/internal/events/cluster.goosac-metering/metering-service/internal/events/cluster_test.goosac-metering/metering-service/internal/events/mapper.goosac-metering/metering-service/internal/events/mapper_test.goosac-metering/metering-service/internal/heartbeat/generator.goosac-metering/metering-service/internal/reconciliation/correction.goosac-metering/metering-service/internal/reconciliation/reconciler.goosac-metering/metering-service/internal/reconciliation/reconciler_test.goosac-metering/metering-service/internal/watch/consumer.goosac-metering/metering-service/internal/watch/consumer_test.go
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 06:07 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 06:07 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 06:07 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
force-pushed
the
feat/OSAC-3410-caas-lifecycle-metering
branch
from
August 6, 2026 06:18
8a3a5d7 to
f521523
Compare
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 06:19 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 06:19 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 06:20 — with
GitHub Actions
Error
GitHub Actions
Error
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 06:51 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 06:51 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 06:52 — with
GitHub Actions
Error
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
osac-metering/metering-service/internal/heartbeat/generator_test.go (1)
281-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the N+1 heartbeat tests deterministic.
gen.Runcan execute multiple ticks during the 300 ms timeout. The lower-bound counts and accumulated component map can pass if components publish across separate ticks. The checkpoint assertion can also pass after an incomplete tick.Drive exactly one tick with a controlled ticker or a one-shot store response. Then assert the exact event count and checkpoint state for that tick.
Also applies to: 315-340, 342-358
🤖 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/heartbeat/generator_test.go` around lines 281 - 313, Make the N+1 heartbeat tests deterministic by replacing the real-time 300 ms gen.Run execution with a controlled single-tick mechanism, using the generator’s ticker or a one-shot store response. Update the tests around the N+1 heartbeat case and the related cases at lines 315-340 and 342-358 to assert the exact heartbeat count, component set, and checkpoint state produced by that one tick.osac-metering/metering-service/internal/events/cluster.go (1)
298-304: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winChange detection ignores
host_typechanges.The comparison only tests
NodeCount. If a node set keeps its size but changeshost_type, no component event is emitted. Billing then uses the old host type until the hourly reconciler reportsbilling_dimensions_drift. The comment on Lines 97-104 documents that fallback, so the gap is bounded, not permanent. A direct comparison closes it immediately.♻️ Proposed change
old, exists := oldByKey[r.NodeSet] - if !exists || old.NodeCount != r.NodeCount { + if !exists || old.NodeCount != r.NodeCount || old.HostType != r.HostType { changed = append(changed, r) }🤖 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/events/cluster.go` around lines 298 - 304, Update the change detection loop over newRecords to also compare each record’s host_type against the matching old record, alongside NodeCount. Append the record to changed whenever either value differs, while preserving the existing handling for new node sets.
🤖 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/events/cluster.go`:
- Around line 306-316: Update the removal branch in the oldRecords comparison to
copy r.NodeSet into each zero-count ComponentRecord, preserving the node-set
identity used by ComponentEventID and FlatBillingDimensions for distinct removal
events and billing closure.
In `@osac-metering/metering-service/internal/reconciliation/reconciler.go`:
- Around line 472-492: Update buildSyntheticHeartbeats to derive one
deterministic base event ID from the resource and heartbeat window, truncating
now to the heartbeat interval, then reuse that base ID for every component and
suffix it via ComponentEventID. Ensure retries within the same stale-heartbeat
window regenerate identical component IDs while preserving the single-heartbeat
path for non-cluster resources.
---
Nitpick comments:
In `@osac-metering/metering-service/internal/events/cluster.go`:
- Around line 298-304: Update the change detection loop over newRecords to also
compare each record’s host_type against the matching old record, alongside
NodeCount. Append the record to changed whenever either value differs, while
preserving the existing handling for new node sets.
In `@osac-metering/metering-service/internal/heartbeat/generator_test.go`:
- Around line 281-313: Make the N+1 heartbeat tests deterministic by replacing
the real-time 300 ms gen.Run execution with a controlled single-tick mechanism,
using the generator’s ticker or a one-shot store response. Update the tests
around the N+1 heartbeat case and the related cases at lines 315-340 and 342-358
to assert the exact heartbeat count, component set, and checkpoint state
produced by that one tick.
🪄 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: bf809d1f-4c29-46e0-9073-54e8d576e228
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumosac-metering/metering-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
osac-metering/metering-service/cmd/metering-service/main.goosac-metering/metering-service/go.modosac-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/heartbeat/generator.goosac-metering/metering-service/internal/heartbeat/generator_test.goosac-metering/metering-service/internal/reconciliation/correction.goosac-metering/metering-service/internal/reconciliation/reconciler.goosac-metering/metering-service/internal/reconciliation/reconciler_test.goosac-metering/metering-service/internal/watch/consumer.goosac-metering/metering-service/internal/watch/consumer_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- osac-metering/metering-service/internal/events/mapper_test.go
- osac-metering/metering-service/cmd/metering-service/main.go
- osac-metering/metering-service/internal/reconciliation/correction.go
- osac-metering/metering-service/internal/heartbeat/generator.go
- osac-metering/metering-service/internal/events/mapper.go
- osac-metering/metering-service/internal/watch/consumer.go
- osac-metering/metering-service/internal/events/cluster_test.go
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 07:00 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 07:01 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 07:01 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 07:02 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 07:02 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 07:02 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 07:45 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
had a problem deploying
to
e2e-test
August 6, 2026 07:45 — with
GitHub Actions
Error
GitHub Actions
Error
omer-vishlitzky
temporarily deployed
to
e2e-test
August 7, 2026 21:01 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Implement ResourceMapper for ClusterOrder with CaaS-specific billing: - State machine: PROGRESSING/READY billable, FAILED/DELETING/DELETE_FAILED not - ErrSkipNonBillingTransition sentinel for billable<->billable and non-billable<->non-billable transitions - ClusterBillingDimensions with full components array (control plane + sorted worker node sets from spec) - DecomposeClusterComponents for N+1 fan-out at publish time - ChangedComponents for scaling detection (including removal with NodeCount=0) - ComponentEventID for deterministic adapter-level dedup - 58 new specs covering full state transition matrix, JSONB round-trip, decomposition, and scaling detection Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…aling - Extend Watch filter to include cluster events - ErrSkipNonBillingTransition handling: update projection on non-billing transitions, detect dimension changes for scaling events - publishLifecycleEvents: single event for VMaaS/created/deleted, N+1 decomposition for CaaS lifecycle events - handleScalingEvent: emit updated.v1 only for changed components, update projection even when only non-component dims change - Deterministic CloudEvent IDs via ComponentEventID for adapter dedup Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
Heartbeat Generator: - buildHeartbeatEvents returns 1 event for VMaaS, N+1 for cluster_order - Per-component heartbeats with flat billing dimensions Reconciler: - Add ClusterLister interface and loadClusters pagination - isBillableForType dispatch for per-resource-type billability - Generalize hardcoded "compute_instance" strings to fs.resourceType - N+1 decomposition for correction events via buildCorrectionEvents - N+1 synthetic heartbeats via buildSyntheticHeartbeats - Guard against nil clusterClient in missed deletion detection - Wire ClustersClient in main.go Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
Consumer tests (8 new specs): - Cluster CREATED publishes exactly 1 event (not N+1) - Cluster started.v1 publishes N+1 events (3 for 1 CP + 2 workers) - Each decomposed event has distinct per-component billing_dimensions - Each decomposed event has deterministic component-scoped ID - PROGRESSING→READY skips publish but updates projection - READY→FAILED publishes N+1 suspended.v1 - Scaling publishes updated.v1 only for changed component - Cluster DELETED publishes exactly 1 event Heartbeat tests (4 new specs): - cluster_order produces N+1 heartbeats with flat billing_dimensions - VMaaS and CaaS in same tick produce correct event counts - Cluster ID checkpointed after all component heartbeats - Partial N+1 publish failure does not checkpoint Reconciler tests (5 new specs): - Cluster missed_creation emits N+1 correction events with flat dims - Cluster state_drift emits N+1 correction events - Cluster missed_deletion emits N+1 correction events - Nil clusterClient skips cluster missed_deletion - ListClusters pagination with 600 clusters Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…ient log - Sync go.work.sum (pre-commit fix) - Consolidate buildComponentHeartbeat into buildHeartbeatEvent with eventID and dims params (CodeRabbit feedback) - Log once per reconciliation run when clusterClient is nil Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…nistic correction IDs
State machines (CaaS + VMaaS):
- CaaS: replace default updated.v1 with explicit non-billable→billable
as resumed.v1. Default case now errors on unexpected transitions.
- VMaaS: enumerate all known states explicitly (STARTING, STOPPING,
DELETING, DELETE_FAILED, UNSPECIFIED). Default case errors.
NodeSet-keyed components:
- Add NodeSet field to ComponentRecord, populated from spec.node_sets
map key. Prevents host_type collision when two pools share a type.
- ComponentEventID uses NodeSet as unique key: {eventID}/{nodeSet}
- ChangedComponents keys on NodeSet instead of component:host_type
- FlatBillingDimensions includes node_set field
Deterministic correction event IDs:
- Derive base ID from resourceID + reason + states instead of
uuid.NewString(). Same drift across reconciliation cycles produces
same event IDs, enabling adapter-level dedup.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
- correctionDescription returns error for unknown CorrectionReason - isBillableForType returns error for unknown resource type - No silent fallbacks remain in any switch statement Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…, proto compat - ChangedComponents: preserve NodeSet on removed component records so ComponentEventID produces unique IDs and FlatBillingDimensions carries the correct node_set for billing closure - buildSyntheticHeartbeats: derive deterministic base ID from resourceID + timestamp instead of uuid.NewString, enabling adapter dedup on retry - Adapt to typed proto references (ClusterTemplateReference, ClusterCatalogItemReference, HostTypeReference, VersionName) - Fix goimports formatting Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…tables State transitions are now defined as maps — the table IS the spec. Missing entry = error (fail fast). No default branches to get wrong. Adding BMaaS or any new resource type = adding a new table. VMaaS (compute_instance): transient states (STOPPING/STARTING), DELETING as billing-ending, explicit wildcard fallbacks. CaaS (cluster_order): billable-to-billable skip, non-billable skip, same-state transitions for scaling, explicit per-state entries. Shared lookup in transitions.go: exact (from, to) match first, then (*,to) wildcard, then error. Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
Fixes review findings from 5-agent critical review: 1. publishAndUpsert: all create/update paths now publish events BEFORE committing projection state. If Kafka fails, projection is not committed, replay retries full publish. Prevents permanent event loss on partial N+1 publish failure. Delete path was already correct (publish-first from PR 135 review). 2. Missing initial CaaS transitions: added ""->FAILED/DELETING/ DELETE_FAILED/UNSPECIFIED as Skip. Prevents consumer crash when first-observed cluster is in non-billable state (bootstrap, reconnect after failure, existing deployment). 3. ErrSkipNonBillingTransition renamed to ErrSkipTransition — the old name was misleading for billable-to-billable transitions (PROGRESSING->READY). 4. Strengthened multi-removal test: verifies exact NodeSet->HostType preservation, not just non-empty NodeSet. 5. DimensionsEqual component ordering test: documents that array order matters (ClusterBillingDimensions sorts keys for determinism). 6. Proto enumeration completeness test: iterates every (from, to) state pair from all ClusterState proto values plus empty initial. If someone removes a table entry, this test fails — prevents wildcard masking. 7. Stale version test updated: publish-first means events reach Kafka even when projection upsert is skipped (adapter dedup handles it). Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
Addresses amito's review comment: the N+1 decompose-build-publish pattern repeated across publishLifecycleEvents, buildCorrectionEvents, buildSyntheticHeartbeats, and buildHeartbeatEvents. DecomposeClusterEvents handles only cluster_order — no fallback. Empty components returns ErrDataQuality (fail fast, not silent single-event degradation). Callers branch on resource type and call DecomposeClusterEvents only for cluster_order. Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…rals Zero switch statements in production code. Zero wildcards. Zero hardcoded string literals. All state machines, event types, resource types, timestamps, corrections, and decomposition use map-based dispatch with constants. Missing entry = error, no fallback. Changes: - Constants for all event types (EventCreated, EventSuspended, ...), resource types (ResourceTypeComputeInstance, ResourceTypeClusterOrder), VMaaS states (CIState*), CaaS states (CLState*) - ResolveCloudEventType: map-based, replaces identical switch in both mappers. CREATED/DELETED fixed, UPDATED delegates to transition table - ResolveTransitionTime: map-based, replaces identical switch in both mappers. Unknown event type = error, nil timestamp = ErrDataQuality - BuildResourceEvents: map-based decomposition dispatch, replaces 5 separate "if cluster_order" branches. Unknown resource type = error. Adding BMaaS = one line in the map - VMaaS transition table: 72 explicit entries (9 from × 8 to), no wildcards. Fixes latent bugs: ""→STOPPED was suspended.v1 (should be skip, no interval to close), RUNNING→UNSPECIFIED was updated.v1 (should be suspended.v1, billable→non-billable) - correctionDescription: map replaces switch, unknown reason = error - isBillableForType: map replaces switch, unknown type = false 318 specs pass (was 240). New test files: transitions_test.go (18 specs), correction_internal_test.go (5 specs). VMaaS exhaustive DescribeTable (72 entries) + proto enumeration completeness test. Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…nsumer
The Watch Consumer crashed and reconnected on every OBJECT_SIGNALED
event (7 per E2E run) and every metadata-only OBJECT_UPDATED without
state_transition_time (3 per run). Each crash lost the bad event and
all subsequent events on that stream, since the Watch protocol has
no resume token.
Two targeted fixes — everything else still fails fast:
1. SIGNALED: ResolveCloudEventType and ResolveTransitionTime return
ErrUnsupportedEvent. The consumer skips the event, increments
osac_metering_events_skipped_total{reason=unsupported_event_type},
and continues processing the same stream.
2. Metadata-only updates: when TransitionTime returns ErrDataQuality
but the resource state hasn't changed (same as projection), the
consumer skips silently — no state transition occurred, nothing
to meter.
Real data quality issues (state changed but no timestamp, missing
resource_id, missing tenant_id) still crash the stream so they
surface as fulfillment-service bugs.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
Finding 1 (CRITICAL): VMaaS dimension change was silently dropped. handleScalingEvent called ChangedComponents (CaaS-specific), returned empty for VMaaS, updated projection without publishing. Fixed with BuildDimensionChangeEvents map-based dispatch: VMaaS emits single updated.v1, CaaS emits per-changed-component events. Also fixed ChangedComponents to compare HostType (not just NodeCount). Finding 2 (HIGH): billing dimension field renamed from version_name to release_image, matching the design doc and CaaS events spec gist adapter contract. Finding 3 (MEDIUM): cluster_order created.v1/deleted.v1 now have flat billing_dimensions (cluster_template, release_image only), no nested components array. Matches design spec for audit events. Finding 4 (MEDIUM): non-billable→billable transitions now consistently use resumed.v1 across both VMaaS and CaaS. Only initial ""→billable uses started.v1. Eliminates adapter special-casing between resource types. Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
MapWatchEvent now takes billingDims as a parameter instead of calling mapper.BillingDimensionsMap() internally. Callers pass the right dims for their context: - Audit events (created/deleted): topLevelDims() strips components BEFORE building the CloudEvent (was: build with nested, then deserialize-strip-reserialize after) - Lifecycle events: dims passed through, replaced per-component during decomposition anyway Removes stripComponentsFromAuditEvent (serialize-deserialize round-trip). Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…view
ChangedComponents compared only NodeCount and HostType, so a cluster
upgrade (release_image change) or template change with unchanged
topology was silently dropped: no updated.v1 published, and the
projection still absorbed the new dimensions unconditionally in
handleScalingEvent — which permanently defeats the reconciler's
billing_dimensions_drift safety net, since the projection now matches
fulfillment and no drift is ever detected. Fixed by comparing via
DimensionsEqual on FlatBillingDimensions, the same equality already
used to gate entry into this code path, so the two can't disagree.
Two ID-derivation bugs meant retries didn't dedup the way CAP-16
("duplicate events do not cause double-counting") requires:
- Correction event IDs were derived from resourceID/reason/states only,
with no discriminator. Two distinct billing_dimensions_drift
corrections for the same resource/state collided on ID, so adapter
dedup silently dropped the second one. Now includes a content
fingerprint of the billing dimensions, so distinct drifts get
distinct IDs while repeat detection of the *same* unresolved drift
still dedups as designed.
- Reconciler's synthetic heartbeat IDs were keyed off "now", so a retry
in a later reconciliation cycle for the same unresolved staleness gap
minted a fresh ID for components already delivered in a prior partial
failure. Now keyed off LastHeartbeatAt/BillableSince — the actual
reference point for the gap — so retries of the same gap dedup.
Heartbeat Generator: base ID now keyed to the heartbeat window instead
of a random UUID (consistency/future-proofing for the ID scheme, not
an active bug — each real tick is a legitimately new heartbeat either
way). More importantly, one resource's publish failure no longer
aborts the whole tick; it's isolated so other resources still get
heartbeated.
Also:
- buildComponentEvent guards against a nil base-event data map
(DataAs leaves the target nil, not an error, when data is empty)
- node_count is validated (integral, non-negative, in int32 range)
before narrowing; a corrupt value now fails the whole decomposition
loudly instead of either silently truncating/wrapping the billed
value or (an earlier draft of this fix) silently dropping just the
bad component, which would have been a new, differently-shaped
silent data loss
- Unified the duplicated lifecycle/scaling event payload construction
(meteringData / buildScalingEvent's hand-rolled map) into one
exported LifecycleData/BuildLifecycleData used by both, so the two
producers can't drift into different shapes on the same topic
- Removed the dead StateContext.NewDimensions field (assigned, never read)
Assisted-by: Claude Code <noreply@anthropic.com>
omer-vishlitzky
force-pushed
the
feat/OSAC-3410-caas-lifecycle-metering
branch
from
August 9, 2026 09:43
fef95f7 to
655c4db
Compare
omer-vishlitzky
temporarily deployed
to
e2e-test
August 9, 2026 09:44 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
omer-vishlitzky
temporarily deployed
to
e2e-test
August 9, 2026 09:44 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
omer-vishlitzky
temporarily deployed
to
e2e-test
August 9, 2026 09:44 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
…idation gaps from review - Bump private-api pin to v0.0.84 and regenerate: fulfillment-service's version_name->ClusterVersionReference conversion made metering's stale proto silently decode the wrong field. Read spec.GetVersion().GetName(). - Track per-component billable-since (ComponentBillableSince) instead of one cluster-wide timestamp, so staggered scaling of independent node sets no longer understates duration_seconds for whichever component didn't cause the most recent reset. Threaded through Reconciler's missed_creation/state_drift paths too, since they write projection rows directly. Folded into the initial migration (no live database yet). - Remove node_count validation from DecomposeClusterComponents entirely: fulfillment-service already rejects non-positive node set sizes at the API layer, and ClusterBillingDimensions always writes a valid typed value, so the check (and JSONB-corruption-only failure mode a stricter version of it would have added) defended against a scenario that can't occur through any real code path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amito, 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 |
Contributor
|
/lgtm |
Contributor
Author
|
/unhold |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends the Metering Service to track CaaS (Kubernetes cluster) lifecycle events with per-component billing. Each cluster generates N+1 Kafka records per heartbeat or lifecycle transition: one for the control plane plus one per distinct worker node set.
What this PR includes
TransitionTablemaps instead of switch statements. The table IS the spec — missing entry = error (fail fast). Adding BMaaS = adding a new table.ErrSkipTransitionsentinel for transitions with no billing effect.ErrTransientState— projection updates only FulfillmentVersion+TransitionTime, preserving CurrentState and billing context. DELETING emitssuspended.v1(billing-ending, not transient).publishAndUpsertenforces publish-before-commit by construction. Prevents permanent event loss on partial N+1 Kafka failure. Delete path was already correct.created.v1anddeleted.v1are single records (audit only).DecomposeClusterEventsshared helper: Extracted from 4 call sites (Watch Consumer, Heartbeat Generator, Reconciler corrections, Reconciler synthetic heartbeats). Cluster-order only, no fallback — empty components =ErrDataQuality.ChangedComponentsemitsupdated.v1only for components whosenode_countdiffers or were added/removed.ClusterListerinterface, paginatedListClusters,isBillableForTypedispatch, N+1 correction events, N+1 synthetic heartbeats.{eventID}/{nodeSet}for adapter-level dedup on replay.Tickets covered
Design reference
Key design decisions
TransitionTablemapspublishAndUpsertDecomposeClusterEventsshared helpercomponentsarray in JSONB{eventID}/{nodeSet}spec.node_setsprevious_state=RUNNINGonsuspended.v1, matching design exactlysuspended.v1suspended.v1only from billable states""→FAILED/DELETING/etc= SkipReview fixes included
Fixes from 5-agent critical review + amito's inline comments:
publishAndUpsertenforces publish before commit, prevents event loss on partial N+1 Kafka failure (affected both VMaaS and CaaS)""→FAILED/DELETING/DELETE_FAILED/UNSPECIFIEDadded as Skip (prevents crash on first-observed non-billable cluster)ErrSkipNonBillingTransitionrenamed toErrSkipTransition— old name misleading for billable→billable transitionsDecomposeClusterEventsshared helper — extracted from 4 call sites per amito's review; cluster-only, no fallback, empty components =ErrDataQuality(from, to)state pair, catches removed table entriesTest plan
go build ./...passesSKIP_DB_TESTS=1 ginkgo run -r --timeout=1m internal/— 240 specs pass (7 suites, ~4s)(from, to)state pair coveredDecomposeClusterEvents: happy path, empty components error, buildFn error propagationpublishAndUpsert: stale version test updated for publish-first semanticsAssisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit