OSAC-3428: Implement Provider Adapter framework with Kafka consumer lifecycle - #174
Conversation
|
@amito: This pull request references OSAC-3428 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 task 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. |
|
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 PR adds the ChangesMetering adapter runtime
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Kafka as Kafka consumer group
participant Runner
participant EchoAdapter
participant EventStore
participant Metrics as Prometheus metrics
Kafka->>Runner: Deliver CloudEvent
Runner->>Runner: Check duplicate and ordering state
Runner->>EchoAdapter: Submit event with retry handling
EchoAdapter->>EventStore: Store event
Runner->>Metrics: Record processing outcome
Runner->>Kafka: Mark processed offset
Runner->>EchoAdapter: Flush buffered events
Runner->>Kafka: Commit offsets
Possibly related PRs
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 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 |
90215ff to
b3018dd
Compare
b3018dd to
ad563e2
Compare
ad563e2 to
45f9742
Compare
45f9742 to
3ad7d3e
Compare
97d7fc4 to
d554f6d
Compare
masayag
left a comment
There was a problem hiding this comment.
Re-review — v3 (current HEAD d554f6d)
All critical and major findings from my previous reviews and from omer-vishlitzky's comments are resolved in this version. The branch was rebased so the referenced commit SHAs in earlier replies no longer exist, but the fixes are confirmed present in the current HEAD.
Resolved
| Finding | Fix |
|---|---|
| ctx-cancellation data-loss regression | runner.go L290-300: else branch skips trackOffset + V(1) log; test added in runner_test.go |
| Consumer group ACL mismatch | echo-adapter-deployment.yaml now sets KAFKA_CONSUMER_GROUP=osac-metering-echo-adapter |
/healthz hardcoded to 200 |
main.go now calls adapter.HealthCheck(r.Context()), returns 503 on failure |
| Broker addresses logged in clear text | Now logs broker_count=%d instead of actual addresses |
mkdir -p bin missing in Makefile |
Fixed |
| pre-commit hook file pattern too narrow | Now includes .golangci.yml, Makefile, go.{mod,sum} |
values.yaml echo adapter defaults |
Commented out in base values; environment-specific vmaas-ci/values.yaml provides the real config |
Minor suggestions for follow-up (non-blocking)
-
newAdapterMetrics(provider string)— unused parameter (metrics.goL31). Theproviderarg is never referenced in the function body. Either remove it or use it in metric Help strings. -
consumerLaggauge registered but never Set() (metrics.goL56-59). It appears as a permanent zero in/metricsoutput — either wire it fromclaim.HighWaterMarkOffset() - msg.OffsetinConsumeClaim, or drop it until a follow-up implements it. -
SubmitResultfromFlush()discarded (runner.goL174). The echo-adapter returns{Idempotent: true}but the Runner ignores it. Worth at least a debug-level log for future adapters that return aProviderEventID. -
echoAdapter.HealthCheckis a no-op (main.goL80) — always returns nil, so probes will always report healthy even if Kafka is unreachable. Fine for a smoke-test binary, but add a comment so real adapters don't copy the pattern. -
RetryableErrorexported but functionally unused (adapter.goL48-52). The framework retries ALL non-NonRetryableErrorerrors, so wrapping inRetryableErroris optional. Document this in adoc.goor inline comment to avoid confusion for future adapter contributors. -
No contributor README yet. A
doc.goorREADME.mddescribing the Submit/Flush contract, error classification, and how to wire a new adapter binary would lower the bar for M360 and Cost Management implementations.
The framework is solid — offset tracking, dedup, retry, graceful shutdown, and the echo-adapter reference are all working correctly. Approving with the above as follow-up suggestions.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amito, masayag 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 |
…ce and types Signed-off-by: Amit Oren <amoren@redhat.com>
- Change go.work from 1.26.4 to 1.26.3 to match global constraint - Change osac-metering/adapters/go.mod from 1.26.4 to 1.26.3 - Run go mod tidy to add missing github.com/cloudevents/sdk-go/v2 v2.16.2 - All transitive dependencies now properly resolved Signed-off-by: Amit Oren <amoren@redhat.com>
Signed-off-by: Amit Oren <amoren@redhat.com>
Signed-off-by: Amit Oren <amoren@redhat.com>
Signed-off-by: Amit Oren <amoren@redhat.com>
Signed-off-by: Amit Oren <amoren@redhat.com>
…t commit Signed-off-by: Amit Oren <amoren@redhat.com>
Signed-off-by: Amit Oren <amoren@redhat.com>
Signed-off-by: Amit Oren <amoren@redhat.com>
go.work now lists osac-metering/adapters as a workspace member, but the Containerfiles did not copy its go.mod/go.sum into the build context, causing go mod download to fail. Signed-off-by: Amit Oren <amoren@redhat.com>
Add TLSEnabled field to KafkaConfig so TLS is only configured when explicitly requested — prevents connection failures to plaintext Kafka brokers. Add unit tests for newConsumerConfig TLS/non-TLS paths and splitAndTrimBrokers. Signed-off-by: Amit Oren <amoren@redhat.com>
Add CI scaffolding matching the sibling metering-service module convention: ginkgo test runner, golangci-lint v2.12.1 with errcheck, govet, staticcheck, unused, misspell, revive, and goimports formatter. Signed-off-by: Amit Oren <amoren@redhat.com>
Remove the raw-Sarama test-adapter (metering-service/cmd/test-adapter/)
and replace it with the echo-adapter (adapters/cmd/echo-adapter/) which
exercises the full adapters.Runner framework lifecycle: dedup, out-of-order
detection, retry with backoff, flush, and offset commit.
The echo-adapter exposes an HTTP query API for E2E test assertions:
- GET /events — list events with filters (type, resource_id, since, limit)
- GET /events/count — count matching events
- GET /events/{id} — lookup by CloudEvent ID
- DELETE /events — clear buffer for test isolation
- GET /metrics — Prometheus metrics
- GET /healthz — health check
Events are also logged to stdout for kubectl logs debugging.
Changes:
- Add echo-adapter binary under adapters/cmd/echo-adapter/
- Add Containerfile.echo-adapter for image builds
- Rename chart templates from test-adapter to echo-adapter
- Rename CI workflow to build-metering-echo-adapter-image.yaml
- Update KafkaUser, RBAC, and values.yaml references
- Add build-echo-adapter target to adapters Makefile
- Remove test-adapter source, Containerfile, and Makefile target
Signed-off-by: Amit Oren <amoren@redhat.com>
d554f6d to
f77baae
Compare
|
/lgtm |
Description
Adds a new Go framework package at
osac-metering/adapters/that handles the full Kafka consumer lifecycle for provider metering adapters. Concrete adapter binaries (M360, Cost Management, etc.) import this package, implement theProviderAdapterinterface, and let the framework'sRunnermanage consumption, retries, dedup, and flushing.Architecture:
Key components:
adapter.goProviderAdapterinterface,MeteringEvent,SubmitResult, error typesrunner.goRunnerwith Kafka consumer lifecycle, flush scheduling, graceful shutdownretry.godedup.goorder.gotransition_timeperresource_idmetrics.goosac_adapter_*), handler for adapter HTTP serverskafka.gocmd/echo-adapter/Echo adapter (
adapters/cmd/echo-adapter/):Replaces the previous
metering-service/cmd/test-adapter/(which used raw Sarama and did not exercise the framework). The echo adapter implements the fullProviderAdapterinterface and exercises the completeRunnerlifecycle. It exposes an HTTP query API for E2E test assertions:GET /eventstype,resource_id,since,limit)GET /events/countGET /events/{id}DELETE /eventsGET /metricsGET /healthzConfiguration:
ECHO_BUFFER_SIZE(default 1000),METRICS_ADDR(default:2112), plus standardKAFKA_*env vars.JIRA Tasks
Not Included / Out of Scope
osac_adapter_replay_lag_eventsmetricHow Has This Been Tested?
Unit-testing
68 unit tests using Ginkgo v2 / Gomega, all passing with race detector enabled:
Test coverage by component:
adapter_test.godedup_test.goorder_test.goretry_test.gometrics_test.gorunner_test.gokafka_test.goNo real Kafka broker required — tests use mock Sarama consumer group sessions and claims.
E2E testing
The echo adapter binary (
adapters/cmd/echo-adapter/) is included in this PR with Helm chart templates and CI image build workflow. E2E integration tests against a development cluster are tracked in OSAC-3440.Review follow-up changes
Changes made in response to review feedback (masayag, CodeRabbit):
Amended into existing commits:
adapter.Close()is bounded by a 10s deadline in the Runner's shutdown path (addresses unbounded Close concern while preservingio.Closercompatibility)/healthznow callsadapter.HealthCheck()instead of returning hardcoded 200KAFKA_CONSUMER_GROUPto match KafkaUser ACLbuild-echo-adaptercreatesbin/directory before build.golangci.yml,Makefile,go.mod,go.sum)New commits:
TLSEnabledfield onKafkaConfig; TLS is only configured when explicitly requested.kafka_test.goadded with unit tests for TLS paths, consumer defaults, and broker address parsing.Makefile,.golangci.yml, GitHub Actions unit test job (run-osac-metering-adapters-tests), and pre-commit lint hook (osac-metering-adapters-golangci-lint), matching the siblingmetering-servicemodule convention.metering-service/cmd/test-adapter/withadapters/cmd/echo-adapter/that exercises the full Runner framework. Adds HTTP query API (/events,/events/count,/events/{id},DELETE /events), Containerfile, Helm chart templates, and CI image build workflow (build-metering-echo-adapter-image.yaml).References
docs/superpowers/specs/2026-08-05-provider-adapter-framework-design.mddocs/superpowers/plans/2026-08-05-provider-adapter-framework.mdMerge criteria
Summary by CodeRabbit
New Features
Tests