Repository navigation
OSAC-3434: Add VMaaS metering lifecycle E2E tests - #301
omer-vishlitzky merged 4 commits into
Conversation
|
@omer-vishlitzky: This pull request references OSAC-3434 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. |
GitHub Actions
Error
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 PR replaces Kafka metering collection with synchronous HTTP polling, adds pytest fixture and marker support, extends VM lifecycle tests with metering assertions, wires the adapter into CI, and documents the testing pattern. ChangesMetering test infrastructure
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant VMTests
participant PytestFixture
participant MeteringCollector
participant MeteringAdapter
VMTests->>PytestFixture: Register expected events
PytestFixture->>MeteringCollector: Start collector
VMTests->>MeteringCollector: Add lifecycle expectations
PytestFixture->>MeteringCollector: Verify expectations during teardown
MeteringCollector->>MeteringAdapter: Poll events endpoint
MeteringAdapter-->>MeteringCollector: Return matching events
MeteringCollector-->>PytestFixture: Validate event fields
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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 |
f15181c to
1964c00
Compare
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/e2e-bmaas-full-install.yml:
- Around line 415-435: PINNED_SHA selection must match the installer repository
before pinning the clone commit. In .github/workflows/e2e-bmaas-full-install.yml
lines 415-435, .github/workflows/e2e-caas-full-install.yml lines 470-493, and
.github/workflows/e2e-vmaas-full-install.yml lines 453-476, update the
COMPONENT_BUILD_RESULTS jq query to pass INSTALLER_REPO via --arg repo and
select entries whose repo equals that value before taking the first sha; leave
PINNED_SHA empty when no match exists so the existing ref-based clone path is
used.
In `@CLAUDE.md`:
- Around line 121-173: Update the directory-structure fenced code block in the
“Directory structure” section of CLAUDE.md to include an appropriate language
identifier, such as text, while leaving its contents unchanged.
In `@tests/core/metering.py`:
- Around line 53-56: Update MeteringCollector.start and the _consume startup
flow to synchronize before returning: create the KafkaConsumer, begin polling,
and wait until at least one partition is assigned, using a threading.Event or
equivalent readiness signal shared with start. Ensure startup failures do not
block indefinitely, while preserving the existing consumer thread and collection
behavior after assignment.
- Around line 118-126: Update the outer loop in the event collection logic
around self._events.append and the “Collected metering event” log to avoid
binding the unused tp value, while preserving iteration over records.items() and
the existing message processing behavior.
In `@tests/vmaas/test_metering_lifecycle.py`:
- Around line 12-44: Update test_metering_vm_lifecycle_events to use
k8s_virt_client for workload-cluster verification: retain the hub-cluster
wait_for_running check, then invoke the established workload-cluster VM/VMI
state wait helper to confirm the VirtualMachineInstance reaches Running. Follow
the existing multi-cluster pattern used by VM-based tests and eliminate the
unused-argument warning.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: d8e08c57-64eb-4b6e-acc1-93ac9a0e7298
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/scripts/discover-audit-runs.sh.github/scripts/replace-installer-submodule.sh.github/workflows/audit-workflow-logs.yml.github/workflows/e2e-bmaas-full-install-caller.yml.github/workflows/e2e-bmaas-full-install.yml.github/workflows/e2e-caas-full-install-caller.yml.github/workflows/e2e-caas-full-install.yml.github/workflows/e2e-vmaas-caller.yml.github/workflows/e2e-vmaas-full-install-caller.yml.github/workflows/e2e-vmaas-full-install.yml.github/workflows/e2e-vmaas.ymlAGENTS.mdCLAUDE.mdMakefilepyproject.tomltests/concerns/__init__.pytests/concerns/conftest.pytests/core/metering.pytests/vmaas/test_metering_lifecycle.py
💤 Files with no reviewable changes (3)
- .github/workflows/audit-workflow-logs.yml
- .github/scripts/replace-installer-submodule.sh
- .github/scripts/discover-audit-runs.sh
| @pytest.mark.metering | ||
| def test_metering_vm_lifecycle_events( | ||
| cli: OsacCLI, | ||
| grpc: GRPCClient, | ||
| k8s_hub_client: K8sClient, | ||
| k8s_virt_client: K8sClient, | ||
| vm_template: str, | ||
| default_subnet: str, | ||
| metering: MeteringCollector, | ||
| ) -> None: | ||
| """Verify that VMaaS lifecycle events are published to Kafka. | ||
|
|
||
| Creates a ComputeInstance, waits for it to reach Running, then | ||
| deletes it. Verifies that created.v1, started.v1, and deleted.v1 | ||
| CloudEvents appear on the osac.metering.lifecycle Kafka topic. | ||
| """ | ||
| uuid: str = cli.create_compute_instance( | ||
| template=vm_template, | ||
| network_attachments=[{"subnet": default_subnet}], | ||
| ) | ||
| metering.expect("osac.resource.created.v1", resource_id=uuid) | ||
|
|
||
| ci_name: str = wait_for_cr(k8s=k8s_hub_client, uuid=uuid) | ||
| wait_for_provision(k8s=k8s_hub_client, name=ci_name) | ||
| wait_for_running(k8s=k8s_hub_client, name=ci_name) | ||
|
|
||
| metering.expect("osac.resource.started.v1", resource_id=uuid) | ||
|
|
||
| cli.delete_compute_instance(uuid=uuid) | ||
| metering.expect("osac.resource.deleted.v1", resource_id=uuid) | ||
|
|
||
| wait_for_deletion(k8s=k8s_hub_client, name=ci_name) | ||
| wait_for_grpc_removal(grpc=grpc, uuid=uuid) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify VM state on the workload cluster, not only the hub CR phase.
k8s_virt_client is injected but never used, and Ruff flags it as an unused argument. wait_for_running only checks the ComputeInstance CR phase on the hub cluster; it does not confirm the VirtualMachineInstance itself is Running on the workload cluster.
Use k8s_virt_client to verify the VM state, matching the multi-cluster pattern documented for VM-based tests.
🔧 Proposed fix
ci_name: str = wait_for_cr(k8s=k8s_hub_client, uuid=uuid)
wait_for_provision(k8s=k8s_hub_client, name=ci_name)
wait_for_running(k8s=k8s_hub_client, name=ci_name)
+ vm_namespace = k8s_hub_client.get_compute_instance_vm_namespace(name=ci_name)
+ assert k8s_virt_client.get_vm_printable_status(name=ci_name, vm_namespace=vm_namespace) == "Running"
+
metering.expect("osac.resource.started.v1", resource_id=uuid)As per path instructions, "Use k8s_virt_client for workload-cluster resources in VM-based tests, and verify both hub-cluster and workload-cluster state."
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 17-17: Unused function argument: k8s_virt_client
(ARG001)
🤖 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 `@tests/vmaas/test_metering_lifecycle.py` around lines 12 - 44, Update
test_metering_vm_lifecycle_events to use k8s_virt_client for workload-cluster
verification: retain the hub-cluster wait_for_running check, then invoke the
established workload-cluster VM/VMI state wait helper to confirm the
VirtualMachineInstance reaches Running. Follow the existing multi-cluster
pattern used by VM-based tests and eliminate the unused-argument warning.
Source: Path instructions
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🤖 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/e2e-bmaas-full-install.yml:
- Around line 415-435: PINNED_SHA selection must match the installer repository
before pinning the clone commit. In .github/workflows/e2e-bmaas-full-install.yml
lines 415-435, .github/workflows/e2e-caas-full-install.yml lines 470-493, and
.github/workflows/e2e-vmaas-full-install.yml lines 453-476, update the
COMPONENT_BUILD_RESULTS jq query to pass INSTALLER_REPO via --arg repo and
select entries whose repo equals that value before taking the first sha; leave
PINNED_SHA empty when no match exists so the existing ref-based clone path is
used.
In `@CLAUDE.md`:
- Around line 121-173: Update the directory-structure fenced code block in the
“Directory structure” section of CLAUDE.md to include an appropriate language
identifier, such as text, while leaving its contents unchanged.
In `@tests/core/metering.py`:
- Around line 53-56: Update MeteringCollector.start and the _consume startup
flow to synchronize before returning: create the KafkaConsumer, begin polling,
and wait until at least one partition is assigned, using a threading.Event or
equivalent readiness signal shared with start. Ensure startup failures do not
block indefinitely, while preserving the existing consumer thread and collection
behavior after assignment.
- Around line 118-126: Update the outer loop in the event collection logic
around self._events.append and the “Collected metering event” log to avoid
binding the unused tp value, while preserving iteration over records.items() and
the existing message processing behavior.
In `@tests/vmaas/test_metering_lifecycle.py`:
- Around line 12-44: Update test_metering_vm_lifecycle_events to use
k8s_virt_client for workload-cluster verification: retain the hub-cluster
wait_for_running check, then invoke the established workload-cluster VM/VMI
state wait helper to confirm the VirtualMachineInstance reaches Running. Follow
the existing multi-cluster pattern used by VM-based tests and eliminate the
unused-argument warning.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: d8e08c57-64eb-4b6e-acc1-93ac9a0e7298
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/scripts/discover-audit-runs.sh.github/scripts/replace-installer-submodule.sh.github/workflows/audit-workflow-logs.yml.github/workflows/e2e-bmaas-full-install-caller.yml.github/workflows/e2e-bmaas-full-install.yml.github/workflows/e2e-caas-full-install-caller.yml.github/workflows/e2e-caas-full-install.yml.github/workflows/e2e-vmaas-caller.yml.github/workflows/e2e-vmaas-full-install-caller.yml.github/workflows/e2e-vmaas-full-install.yml.github/workflows/e2e-vmaas.ymlAGENTS.mdCLAUDE.mdMakefilepyproject.tomltests/concerns/__init__.pytests/concerns/conftest.pytests/core/metering.pytests/vmaas/test_metering_lifecycle.py
💤 Files with no reviewable changes (3)
- .github/workflows/audit-workflow-logs.yml
- .github/scripts/replace-installer-submodule.sh
- .github/scripts/discover-audit-runs.sh
🛑 Comments failed to post (1)
.github/workflows/e2e-bmaas-full-install.yml (1)
415-435: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
PINNED_SHAselection ignores repo identity in all three full-install workflows. Each workflow computesPINNED_SHA=$(echo "${COMPONENT_BUILD_RESULTS:-[]}" | jq -r '.[0].sha // empty')without checking that the first component'srepomatchesINSTALLER_REPO. Each workflow also supports building a component from a different repo (theaap.bootstrap.imageoverride), so this is a reachable failure mode, not a theoretical edge case: when that differently-sourced component is first in the array, the followinggit fetch --depth 1 "https://github.com/${INSTALLER_REPO}.git" "${PINNED_SHA}"requests a commit that does not exist inINSTALLER_REPO, and the job fails.
.github/workflows/e2e-bmaas-full-install.yml#L415-L435: filterCOMPONENT_BUILD_RESULTSbyselect(.repo == $repo)(with--arg repo "${INSTALLER_REPO}") before taking[0].sha, soPINNED_SHAstays empty (falling back to ref-based clone) when no component matchesINSTALLER_REPO..github/workflows/e2e-caas-full-install.yml#L470-L493: apply the same repo-filteredjqselection forPINNED_SHA..github/workflows/e2e-vmaas-full-install.yml#L453-L476: apply the same repo-filteredjqselection forPINNED_SHA.📍 Affects 3 files
.github/workflows/e2e-bmaas-full-install.yml#L415-L435(this comment).github/workflows/e2e-caas-full-install.yml#L470-L493.github/workflows/e2e-vmaas-full-install.yml#L453-L476🤖 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 @.github/workflows/e2e-bmaas-full-install.yml around lines 415 - 435, PINNED_SHA selection must match the installer repository before pinning the clone commit. In .github/workflows/e2e-bmaas-full-install.yml lines 415-435, .github/workflows/e2e-caas-full-install.yml lines 470-493, and .github/workflows/e2e-vmaas-full-install.yml lines 453-476, update the COMPONENT_BUILD_RESULTS jq query to pass INSTALLER_REPO via --arg repo and select entries whose repo equals that value before taking the first sha; leave PINNED_SHA empty when no match exists so the existing ref-based clone path is used.
1964c00 to
15d4cb1
Compare
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/vmaas/test_metering_short_lived_vm.py (1)
1-40: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFile name deviates from the
test_<resource>_<scenario>.pyconvention.Sibling files in
tests/vmaas/followtest_<resource>_<scenario>.py, for exampletest_compute_instance_restart.py. This file is namedtest_metering_short_lived_vm.py, leading with the concern name instead of the resource. Rename to something liketest_compute_instance_short_lived_metering.pyfor consistency.As per path instructions, "Name test files using
test_<resource>_<scenario>.py."🤖 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 `@tests/vmaas/test_metering_short_lived_vm.py` around lines 1 - 40, Rename the test file from test_metering_short_lived_vm.py to test_compute_instance_short_lived_metering.py so it follows the test_<resource>_<scenario>.py convention; leave the test_short_lived_vm_metering function and its implementation unchanged.Source: Path instructions
tests/conftest.py (1)
25-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the function docstring.
config.addinivalue_line(...)is now the first statement inpytest_configure. The triple-quoted string that follows is no longer the function's docstring; it becomes a no-op expression statement, andpytest_configure.__doc__is lost. Move the new call after the docstring.🧹 Proposed fix
def pytest_configure(config: pytest.Config) -> None: - config.addinivalue_line("markers", "metering: test verifies metering events on Kafka") """Give each xdist worker its own log_file so records stay chronologically ordered. A shared log_file is interleaved across workers as they report at their own pace; gather-osac-logs.sh merges the per-worker files back into one sorted e2e.log artifact. """ + config.addinivalue_line("markers", "metering: test verifies metering events on Kafka") worker_id = os.environ.get("PYTEST_XDIST_WORKER")🤖 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 `@tests/conftest.py` around lines 25 - 32, Move the existing docstring to the first statement in pytest_configure, then execute config.addinivalue_line(...) afterward so pytest_configure.__doc__ is preserved while the metering marker remains registered.
♻️ Duplicate comments (2)
tests/core/metering.py (2)
141-153: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDrop the unused loop variable
tp.
tpis never used in the loop body, and onlyrecords.values()is needed. This is the same finding raised previously on this file and it still triggers Ruff B007/PERF102.As per coding guidelines, "Follow the configured Python style: Ruff formatting with a 120-character line length and Ruff rules E, F, W, I, UP, ANN, B, SIM, and RUF targeting Python 3.11."🧹 Proposed fix
- for tp, messages in records.items(): + for messages in records.values(): for msg in messages:🤖 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 `@tests/core/metering.py` around lines 141 - 153, Remove the unused tp loop variable in the metering consumer loop by iterating over records.values() directly, while preserving the existing message processing and logging behavior.Sources: Coding guidelines, Linters/SAST tools
54-60: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftConfirm real partition assignment before signaling readiness, and surface consumer-construction failures.
_ready.set()runs unconditionally right after a singleconsumer.poll(timeout_ms=0)call. A zero-timeoutpoll()call returns almost immediately and does not guarantee that partition assignment or metadata fetch has completed; it can return before the consumer's read position is established, since atimeout_ms=0budget generally is not enough to complete a broker round trip. This means the original race the prior review addressed (an event published early in the test occurring before the consumer's position is set) can still occur, just less often.Separately,
KafkaConsumer(...)is constructed outside atryblock in_consume. If it raises (bad credentials, unreachable broker, TLS failure), the exception is lost in the background thread.start()then only reports a generic"Kafka consumer failed to become ready within 30s"timeout, hiding the real cause and slowing down CI debugging.Loop on
consumer.assignment()with a bounded deadline before setting_ready, and capture exceptions from the thread sostart()can re-raise the real error.Since this touches `kafka-python-ng` consumer semantics, please confirm the assignment-polling behavior against the installed version's documentation.🔧 Proposed fix to confirm assignment and propagate startup errors
def __init__( self, *, bootstrap_servers: str, topic: str, sasl_username: str, sasl_password: str, ca_cert_path: str | None = None, ) -> None: self._bootstrap_servers = bootstrap_servers self._topic = topic self._sasl_username = sasl_username self._sasl_password = sasl_password self._ca_cert_path = ca_cert_path self._events: list[dict[str, Any]] = [] self._expectations: list[ExpectedEvent] = [] self._lock = threading.Lock() self._stop = threading.Event() self._ready = threading.Event() self._thread: threading.Thread | None = None + self._start_error: BaseException | None = None def start(self) -> None: self._stop.clear() self._ready.clear() + self._start_error = None self._thread = threading.Thread(target=self._consume, daemon=True) self._thread.start() if not self._ready.wait(timeout=30): raise RuntimeError("Kafka consumer failed to become ready within 30s") + if self._start_error is not None: + raise RuntimeError("Kafka consumer failed to start") from self._start_errordef _consume(self) -> None: - ssl_context = ssl.create_default_context() - if self._ca_cert_path: - ssl_context.load_verify_locations(self._ca_cert_path) - - consumer = KafkaConsumer( - self._topic, - bootstrap_servers=self._bootstrap_servers, - security_protocol="SASL_SSL", - sasl_mechanism="SCRAM-SHA-512", - sasl_plain_username=self._sasl_username, - sasl_plain_password=self._sasl_password, - ssl_context=ssl_context, - auto_offset_reset="latest", - consumer_timeout_ms=2000, - group_id=None, - value_deserializer=lambda v: json.loads(v.decode("utf-8")), - ) + try: + ssl_context = ssl.create_default_context() + if self._ca_cert_path: + ssl_context.load_verify_locations(self._ca_cert_path) + + consumer = KafkaConsumer( + self._topic, + bootstrap_servers=self._bootstrap_servers, + security_protocol="SASL_SSL", + sasl_mechanism="SCRAM-SHA-512", + sasl_plain_username=self._sasl_username, + sasl_plain_password=self._sasl_password, + ssl_context=ssl_context, + auto_offset_reset="latest", + consumer_timeout_ms=2000, + group_id=None, + value_deserializer=lambda v: json.loads(v.decode("utf-8")), + ) + except Exception as exc: # noqa: BLE001 + self._start_error = exc + self._ready.set() + return try: - consumer.poll(timeout_ms=0) - self._ready.set() + deadline = time.monotonic() + 30 + while not consumer.assignment() and time.monotonic() < deadline: + consumer.poll(timeout_ms=200) + self._ready.set() while not self._stop.is_set():Also applies to: 119-140
🤖 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 `@tests/core/metering.py` around lines 54 - 60, Update the Metering consumer startup flow in start and _consume to wait, until a bounded deadline, for consumer.assignment() to become non-empty before setting _ready; use the installed kafka-python-ng semantics to ensure polling drives assignment. Wrap KafkaConsumer construction and startup logic in exception capture, store the original failure for the owning thread, and have start re-raise it instead of reporting only the generic readiness timeout.
🤖 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 `@AGENTS.md`:
- Around line 381-383: Update the concern-testing documentation references in
AGENTS.md lines 381-383 and 60, and CLAUDE.md lines 127-129, to point to
tests/conftest.py instead of tests/concerns/conftest.py. Keep the documented
three-part workflow aligned with the repository’s existing metering fixture,
marker registration, filtering, and infrastructure-skip implementation; do not
move code unless necessary.
In `@CLAUDE.md`:
- Around line 146-154: Document all Kafka environment variables read by the
metering fixture in the filtering/skipping guidance in CLAUDE.md (lines 146-154)
and the corresponding metering guidance in AGENTS.md (line 394). Since
collection currently fails when KAFKA_BOOTSTRAP_SERVERS is missing, explicitly
document it as required rather than claiming the tests auto-skip; preserve the
existing marker examples and clarify the unavailable-infrastructure behavior
consistently in both files.
In `@Makefile`:
- Line 34: Update the metering test recipe to prevent shell injection from TEST:
export TEST before the recipe command, reference it as $${TEST} inside single
quotes for the pytest filter, and quote the REPORTS_DIR expansion when
constructing the JUnit report path.
In `@tests/conftest.py`:
- Around line 253-258: Update the collector lifecycle setup around
collector.start() so startup runs inside the try/finally block, ensuring
collector.stop() executes when startup raises as well as after the yielded test
completes. Preserve the existing collector.verify() behavior for successful
startup.
- Line 228: Update pytest_collection_modifyitems to remove the unused config
parameter and retain only items in its signature; leave the hook’s existing
behavior unchanged.
In `@tests/core/metering.py`:
- Around line 75-97: Update _poll_for_event so each matching event is marked
consumed and cannot satisfy later expectations. Track consumed events across
polling calls on the metering instance, skip them during find(), and record the
matched event before returning; preserve existing matching and timeout behavior.
---
Outside diff comments:
In `@tests/conftest.py`:
- Around line 25-32: Move the existing docstring to the first statement in
pytest_configure, then execute config.addinivalue_line(...) afterward so
pytest_configure.__doc__ is preserved while the metering marker remains
registered.
In `@tests/vmaas/test_metering_short_lived_vm.py`:
- Around line 1-40: Rename the test file from test_metering_short_lived_vm.py to
test_compute_instance_short_lived_metering.py so it follows the
test_<resource>_<scenario>.py convention; leave the test_short_lived_vm_metering
function and its implementation unchanged.
---
Duplicate comments:
In `@tests/core/metering.py`:
- Around line 141-153: Remove the unused tp loop variable in the metering
consumer loop by iterating over records.values() directly, while preserving the
existing message processing and logging behavior.
- Around line 54-60: Update the Metering consumer startup flow in start and
_consume to wait, until a bounded deadline, for consumer.assignment() to become
non-empty before setting _ready; use the installed kafka-python-ng semantics to
ensure polling drives assignment. Wrap KafkaConsumer construction and startup
logic in exception capture, store the original failure for the owning thread,
and have start re-raise it instead of reporting only the generic readiness
timeout.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 6a62ba3c-4dad-46dd-92ec-511e9e804801
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
AGENTS.mdCLAUDE.mdMakefilepyproject.tomltests/conftest.pytests/core/metering.pytests/vmaas/test_compute_instance_creation.pytests/vmaas/test_compute_instance_delete_during_provision.pytests/vmaas/test_compute_instance_restart.pytests/vmaas/test_metering_short_lived_vm.py
|
|
||
| test-metering: | ||
| mkdir -p $(REPORTS_DIR) | ||
| pytest -m metering tests/ -v $(if $(TEST),-k "$(TEST)") --junitxml=$(REPORTS_DIR)/metering.xml |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'test-metering|TEST=' .github Makefile AGENTS.md CLAUDE.md || trueRepository: osac-project/osac-test-infra
Length of output: 1773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Makefile lines 1-45 =="
sed -n '1,45p' Makefile | cat -n
echo
echo "== GitHub workflows invoking make/test-metering =="
if git ls-files .github/workflows/. | grep -q .; then
rg -n -C 3 'make (test|test-metering)|TEST=|test-metering|REPORTS_DIR' .github/workflows || true
else
echo "no .github/workflows files found"
fi
echo
echo "== Documentation references to TEST/REPORTS_DIR =="
rg -n -C 3 'TEST=|test-metering|REPORTS_DIR|make test|make test-vmaas|make test-bmaas' README.md AGENTS.md CLAUDE.md Makefile || true
echo
echo "== Repository files =="
git ls-files Makefile AGENTS.md CLAUDE.md README.md | sortRepository: osac-project/osac-test-infra
Length of output: 7171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
makefile = Path("Makefile").read_text()
tests = [
"test:",
"test-vmaas:",
"test-caas:",
"test-storage:",
"test-bmaas:",
"test-metering:",
]
for target in tests:
lines = makefile.splitlines()
idx = next(i + 1 for i, line in enumerate(lines) if line.startswith(target))
recipe_lines = []
for j in range(idx, len(lines)):
if (not lines[j].startswith("\t")) or j == len(lines) - 1:
break
recipe_lines.append(j + 1)
print(f"{target} recipe lines: {', '.join(map(str, recipe_lines[:4]))}")
print(" contains $(if $(TEST) pattern:", "$TEST" in "".join(lines[i] for i in range(idx - 1, idx + 4)))
print(" contains REPORTS_DIR expansions:", [f"${{{line}:{j+1}:{line.find('REPORTS_DIR')} + len('REPORTS_DIR')}...}" for j,line in enumerate(lines[idx-1:idx+4], start=idx) if "REPORTS_DIR" in line] or "yes")
print()
# Behavioral probe using temporary Makefile snippet: Python's mkstemp cleanup is local; file not inside repo.
import tempfile, subprocess, pathlib
snippet = ["REPORTS_DIR ?= reports", "REPORTS_DIR_EXPANSION: mkdir -p $(REPORTS_DIR); true", "SHELL_INJECTION: true $(if $(TEST),-k \"$(TEST)\")\"]
with tempfile.TemporaryDirectory() as d:
p = pathlib.Path(d) / "Makefile"
p.write_text("\n".join(snippet + [""])
)
r = subprocess.run(["make", "-f", str(p), "SHELL_INJECTION", "-e", "TEST=foo\"; touch $TMPDIR/pwn; #"])
print("Make recipe with SHELL_INJECTION TEST=... rc=", r.returncode)
print("tmp pwn created:", (pathlib.Path(d) / "pwn").exists())
PYRepository: osac-project/osac-test-infra
Length of output: 576
Prevent shell injection through TEST.
TEST is documented as a Make parameter and is expanded in recipe shell commands. A value such as TEST='foo"; touch /tmp/pwn; #' can run a second command. If callers may supply TEST, pass it through export TEST before expansion and expand it as $${TEST} with single quotes in the recipe; also quote REPORTS_DIR expansions.
🤖 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 `@Makefile` at line 34, Update the metering test recipe to prevent shell
injection from TEST: export TEST before the recipe command, reference it as
$${TEST} inside single quotes for the pytest filter, and quote the REPORTS_DIR
expansion when constructing the JUnit report path.
There was a problem hiding this comment.
Still technically valid on the current Makefile (TEST is expanded unquoted into -k "$(TEST)" without export/escaping), but I'd downgrade the severity here — TEST is a local developer-supplied make variable for filtering test runs, not attacker-controlled/untrusted input reachable from CI triggers, so this is a hygiene nit rather than an exploitable vulnerability. Worth a quick export TEST + $${TEST} fix, but not blocking.
There was a problem hiding this comment.
@masayag, agreed. The shell-expansion concern is valid, but the stated usage makes it a non-blocking hygiene issue rather than an exploitable vulnerability.
The review severity should be downgraded. The minimal export TEST and $${TEST} change remains appropriate for the Make target.
You are interacting with an AI system.
GitHub Actions
Error
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E BMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E VMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E CaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
|
/retest |
|
Re-triggered failed runs:
|
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E VMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E BMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E VMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E CaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
|
/retest |
|
Re-triggered failed runs:
|
|
[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 |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E VMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
…tructure Establish the composable fixture pattern for testing features that span multiple domains (metering, storage, networking, catalog): - Verifier class in tests/core/<concern>.py - Fixture + marker + skip logic in tests/conftest.py - Tests annotate with @pytest.mark.<concern> and inject fixture Metering implementation: - kafka-python-ng dependency for Kafka consumption - MeteringCollector: background Kafka consumer with expect/verify. Validates CloudEvent structure (specversion, source, tenant, billing dimensions, schema version) on every matched event. - metering fixture: starts collector, yields, verifies + stops - @pytest.mark.metering marker, auto-skip without KAFKA_BOOTSTRAP_SERVERS - test-metering Makefile target (pytest -m metering) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> OSAC-3434: Add metering verification to VMaaS lifecycle tests Annotate existing VMaaS tests with @pytest.mark.metering and inject the metering fixture. No new test files — metering verification is composable on top of existing lifecycle tests. Annotated tests: - test_compute_instance_lifecycle: created.v1 + started.v1 + deleted.v1 - test_compute_instance_delete_during_provision: created.v1 + deleted.v1 - test_compute_instance_restart: created.v1 + started.v1 When KAFKA_BOOTSTRAP_SERVERS is not set, these tests run without metering verification (marker auto-skips the fixture). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> OSAC-3434: Fix consumer race condition and billing_dimensions assertion - Wait for Kafka partition assignment before returning from start(). First poll() triggers partition assignment; threading.Event gates start() until consumer is ready. Prevents missing early events. - Assert billing_dimensions contains instance_type, image_ref, and boot_disk_size_gib per OSAC-3434 AC, not just key existence. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> OSAC-3434: Add missing metering ACs — restart events and short-lived VM - Restart test: add suspended.v1 + started.v1 expectations during the restart cycle (VM stops then starts — both transitions must produce metering events) - Short-lived VM test (CAP-4): create and immediately delete a VM without waiting for Running. Verify created.v1 + deleted.v1 appear on Kafka — validates sub-minute billing granularity Note: AC2 checks suspended.v1 event type but not previous_state field (always null in Phase 1, no state tracking). Full AC2 coverage in Phase 2 when previous_state is populated. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> OSAC-3434: Fail hard when metering tests run without Kafka If @pytest.mark.metering tests are collected but KAFKA_BOOTSTRAP_SERVERS is not set, fail immediately instead of silently skipping. Missing Kafka when metering is expected is a configuration bug, not a graceful degradation. Use -m 'not metering' to explicitly exclude metering tests. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> OSAC-3434: Fix review findings — event dedup, consumer robustness, docstring - Track matched events: _matched set prevents the same Kafka event from satisfying multiple expectations. Restart test expecting two started.v1 events now requires two distinct events. - Propagate consumer errors: if KafkaConsumer constructor fails (bad creds, TLS, unreachable), the exception is saved and re-raised from start() instead of a generic timeout message. - Wait for partition assignment: loop on consumer.assignment() with bounded retries instead of a single poll(timeout_ms=0). - Guard leaked thread: fixture wraps start() in try/except, calls stop() on failure before re-raising. - Fix displaced docstring in pytest_configure. - Fix unused loop variable (records.values() not records.items()). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> OSAC-3434: Signal ready on both success and failure paths - Set _ready on exception path so start() surfaces the error immediately instead of waiting 30s timeout - Check assignment after poll loop — fail if no partitions assigned instead of silently proceeding with no assignment Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> OSAC-3434: Pass Kafka credentials to VMaaS E2E test container Extract Kafka bootstrap address, SASL password, and CA cert from the deployed cluster and pass them to the test container. Without these, the MeteringCollector can't connect to Kafka and metering tests fail at collection time. Only set when Kafka is deployed (kafka cluster exists in osac-kafka namespace). SASL password masked in workflow logs. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> fix(pr-review): switch to test adapter HTTP API, add heartbeat test Replace direct Kafka consumer with HTTP client querying the metering test adapter's /events endpoint. The test adapter runs inside the cluster — CI creates an OpenShift Route to make it externally accessible. Eliminates the cluster-internal DNS resolution issue that caused all metering tests to ERROR on teardown. MeteringCollector: - HTTP-based: queries GET /events?type=...&resource_id=...&since=... - No background thread, no Kafka dependency, no SSL config - Per-expectation timeout (default 60s, heartbeat uses 120s) - Enhanced structure validation: numeric boot_disk_size_gib, project_id presence, RFC3339 transition_time, previous_state/duration_seconds for state transition events Tests: - New: test_compute_instance_heartbeat — verifies heartbeat events appear for a RUNNING VM within 120s (Phase 2 heartbeat generator) - Fix: restart test expects resumed.v1 (not started.v1) for STOPPED→RUNNING transition - Rename: test_metering_short_lived_vm → test_compute_instance_short_lived_metering (AGENTS.md naming convention) CI: - Replace Kafka credential extraction with oc expose route for test adapter; single METERING_ADAPTER_URL env var replaces 5 Kafka vars - Remove kafka-python-ng dependency Review feedback (masayag): - Remove unused config param from pytest_collection_modifyitems - Fix AGENTS.md stale path (tests/concerns/conftest.py → tests/conftest.py) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> fix(pr-review): HTTP resilience, stop-VM test, structure validation HTTP resilience: - Wrap _fetch_events in try/except inside poll_until find() closure. Transient HTTP errors (URLError, JSONDecodeError, OSError) are logged and retried instead of crashing the test immediately. Stop-VM metering test (AC 2): - New test_compute_instance_stop_metering: creates VM, waits for RUNNING, patches runStrategy to Halted, waits for Stopped, verifies suspended.v1 event. Covers the "stop VM" acceptance criterion that was missing (only restart exercised suspended.v1 before). Structure validation: - Assert osacresourceid extension matches expected resource_id (previously only used for matching, not validated) Heartbeat test: - Call metering.verify() explicitly before deleting VM to ensure heartbeat assertion is checked while VM is still RUNNING Cleanup: - Remove phantom kafka-python-ng from uv.lock - Restore grpcurl health check explanatory comments in CI workflow - Add name=unique_name() to short-lived test for consistency Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> feat: comprehensive metering E2E coverage — heartbeat stop, reconciliation, deep validation Metering collector enhancements: - assert_no_events: negative assertion for verifying events do NOT appear within a time window (heartbeat stops after non-billable) - get_event: returns event data for value assertions beyond structure - verify() clears expectations after completion for multi-phase tests - suspended.v1 deep validation: previous_state must be RUNNING, duration_seconds must be positive (closes a billing interval) New tests: - test_compute_instance_stop_metering: enhanced with heartbeat-stops verification — after stopping VM, asserts no heartbeat events appear within 90s (design doc line 815) - test_compute_instance_reconciliation_metering: restarts metering pod, waits for readiness (startup reconciliation), verifies heartbeats resume for still-running VM (design doc CAP-15, line 839) CI workflow: - Remove conditional metering exclusion — metering is mandatory for vmaas, missing adapter is an infra bug that should fail immediately. Always pass METERING_ADAPTER_URL to test container. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> fix(pr-review): contract mismatches, validation depth, robustness Contract fixes (blocking): - Skip transition_time assertion for heartbeat events (heartbeatData has no transition_time field — generator.go:179) - Accept "osac-metering/reconciler" as valid source (synthetic heartbeats from reconciler use different source — reconciler.go:389) Validation depth: - osacresourcetype asserts exact value "compute_instance" (was presence) - resumed.v1 previous_state asserts value in (STOPPED, PAUSED) - assert_no_events uses datetime parsing instead of string comparison (Z vs +00:00 format difference) - assert_no_events error includes event IDs and timestamps - verify() warns when called with no expectations Robustness: - Workflow: check route host non-empty before setting URL; emit ::error:: if adapter unavailable; quote METERING_ADAPTER_URL - Heartbeat timeout 180s (was 120s) — 3x interval for margin - Reconciliation test VM cleanup in try/finally block Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> fix: get_event after verify, billing dimensions truthiness get_event() after verify() was dead code — verify() added matched event IDs to _matched, then get_event() skipped them and timed out. The billing dimensions value check in test_compute_instance_lifecycle never executed. Fix: verify() now stores matched events in _verified dict; get_event() returns from cache if available. billing_dimensions: instance_type and image_ref now checked for truthiness (not just key presence), catching empty string values. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> fix: enable metering test adapter in CI install Add --set metering.testAdapter.enabled=true to Helm install args. The test adapter pod is required for metering E2E tests — without it, the route creation step fails and pytest_collection_modifyitems correctly aborts the session (metering is mandatory for vmaas). Also fix get_event after verify (verified events cached in _verified dict) and billing_dimensions truthiness checks. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> Revert "fix: enable metering test adapter in CI install" This reverts commit 2d0d014. fix: suspended.v1 previous_state accepts transient states, K8sClient API fix suspended.v1 validation: - Accept RUNNING, STOPPING, STARTING as valid previous_state values (was: RUNNING only). A VM goes RUNNING→STOPPING→STOPPED, so the suspended.v1 event may capture STOPPING as previous_state. Reconciliation test: - Use runner.run_unchecked instead of K8sClient.run (which doesn't exist) for pod deletion and readiness polling. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> fix: allow null duration_seconds in suspended.v1 When a VM transitions RUNNING→STOPPING→STOPPED, the billing interval closes on the RUNNING→STOPPING transition (updated.v1 with duration). The subsequent STOPPING→STOPPED transition emits suspended.v1 with null duration_seconds because no billing interval was open. This is correct behavior — the assertion should accept null OR positive. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> fix: relax instance_type value check (proto mismatch), stop previous_state instance_type value check removed from lifecycle test — metering service proto needs regeneration after PR osac-project#85 changed instance_type from optional string to InstanceTypeReference. The generic _validate_structure still checks instance_type is present and non-empty. Value check can be restored after metering proto is regenerated. Stop test: accept STOPPING as valid previous_state alongside RUNNING. VM transitions RUNNING→STOPPING→STOPPED; the suspended.v1 event may capture either as previous_state depending on timing. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> Revert "fix: relax instance_type value check (proto mismatch), stop previous_state" This reverts commit 1e7aae7. fix: resolve echo-adapter Service by label, fail fast if missing oc expose svc/osac-metering-test-adapter targets a name that no longer exists after the test-adapter to echo-adapter rename — the Service never gets exposed, and the swallowed errors (2>/dev/null || true, || echo "") let the job limp forward with an empty METERING_ADAPTER_URL instead of failing at the source. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> fix: match resource_id field from echo-adapter response The echo-adapter returns resource_id at the top level (extracted from the osacresourceid extension). Add it as first match check alongside osacresourceid and data.resource_id. Depends on osac-project/osac#250 Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com> fix: heartbeat race in stop metering test, ensure cleanup on failure Wait one heartbeat interval after suspended.v1 before asserting no more heartbeats — the projection DB lags behind Kafka, so one stale heartbeat sweep can fire after the stop is confirmed. Wrap post-stop assertions in try/finally so the compute instance is always cleaned up, matching test_compute_instance_reconciliation_metering. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E BMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E CaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E VMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
Remove test_compute_instance_reconciliation_metering — it restarts the metering pod, which disrupts all other metering tests running in parallel via xdist (Watch stream replay causes spurious resumed.v1 events on stopped VMs in other workers). Will re-enable once a disruptive test isolation pattern is established. Tracked in OSAC-3918. Restore test_compute_instance_stop_metering to original form — the heartbeat race was caused by the reconciliation test's pod restart, not by projection lag. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
|
/lgtm |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E BMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E VMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E CaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
The heartbeat generator queries the projection DB, which lags behind Kafka publication (publishAndUpsert publishes first, updates DB second). One more heartbeat sweep can fire against the stale projection after suspended.v1 is confirmed. Wait 90s (> heartbeat interval) to let in-flight heartbeats drain before starting the no-heartbeat assertion window. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
|
New changes are detected. LGTM label has been removed. |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E BMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E VMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
The test patched the ComputeInstance K8s CR directly to set runStrategy=Halted. The fulfillment-service API record still had run_strategy=Always, so the controller reconciled the discrepancy and restarted the VM — causing a spurious resumed.v1 event and legitimate heartbeats that failed the assert_no_events check. Use the private gRPC UpdateComputeInstance RPC with an update mask on spec.run_strategy instead. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
🚨 Credential leak found in e2e run logs/artifactsWorkflow: E2E BMaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Inspect the redacted artifact (and the scan job summary) for finding details. Rotate any real credentials. |
This branch was previously deployed
3 participants
Summary
Cross-cutting concern pattern
Tests for features like metering that span multiple domains use a composable fixture pattern:
tests/core/metering.py—MeteringCollector(Kafka consumer with expect/verify)tests/concerns/conftest.py—meteringfixture +@pytest.mark.meteringmarker + auto-skipTests
test_metering_vm_lifecycle_events— Create VM → Running → Delete, verify created.v1/started.v1/deleted.v1 on Kafkatest_metering_event_structure— Verify CloudEvent fields: specversion, source, resource type, tenant, billing dimensionsRequirements
KAFKA_BOOTSTRAP_SERVERSenv var (auto-skip if not set)KAFKA_SASL_USERNAME/KAFKA_SASL_PASSWORDfor SCRAM authKAFKA_CA_CERT_PATHfor TLS CAReferences
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
New Features
Documentation
CI Improvements