feat: yg advertisement address, curio log lvl to debug, remove unnecessary scripts#62
feat: yg advertisement address, curio log lvl to debug, remove unnecessary scripts#62redpanda-f wants to merge 11 commits intomainfrom
Conversation
|
@rjan90 you may need to tag this properly, this is 4.2 |
There was a problem hiding this comment.
Pull request overview
This PR addresses Yugabyte CQL connectivity issues with Curio's gocql driver, enables debug logging for PDP operations, and introduces a comprehensive end-to-end test workflow for PDP caching functionality. The changes fix a critical networking issue where gocql would fail to connect to YugabyteDB due to an incorrect advertise_address configuration.
Changes:
- Fixed Yugabyte networking by removing
--advertise_address=0.0.0.0to allow gocql to properly discover and connect to YugabyteDB instances - Added debug logging (
GOLOG_LOG_LEVEL=pdp=debug) to Curio containers for better observability of PDP operations - Introduced a new GitHub Actions workflow for end-to-end testing of PDP caching layers, including storage operations, cache validation, and database state verification
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/commands/start/yugabyte/mod.rs | Removed --advertise_address=0.0.0.0 flag and added detailed explanation of why this fixes gocql connection issues |
| src/commands/start/curio/db_setup.rs | Added debug log level environment variable for PDP logging in database setup containers; introduced unused import |
| src/commands/start/curio/daemon.rs | Added debug log level environment variable for PDP logging in Curio daemon containers |
| .github/workflows/e2e-test-pdp-caching-layers.yml | New comprehensive workflow testing PDP caching functionality through multiple phases including cluster startup, Synapse SDK e2e tests, and cache validation |
src/commands/start/curio/daemon.rs
Outdated
| docker_args.push("-e".to_string()); | ||
| docker_args.push("GOLOG_LOG_LEVEL=pdp=debug".to_string()); |
There was a problem hiding this comment.
According to the coding guidelines, magic strings should be defined as constants. The string "GOLOG_LOG_LEVEL=pdp=debug" is duplicated in three locations (db_setup.rs lines 206 and 319, and here). This should be defined as a constant in the constants module (e.g., pub const CURIO_PDP_DEBUG_LOG_LEVEL: &str = "GOLOG_LOG_LEVEL=pdp=debug";) and referenced from there.
There was a problem hiding this comment.
Up to phase 4 this workflow is nearly identical to ci.yml, but without caching. I think we would benefit from having this logic repeated only once where possible, otherwise this is going to be a maintenance headache.
You could make a composite action so it's all reusable, e.g. .github/actions/devnet-setup/action.yml and then uses: ./.github/actions/devnet-setup within both workflows that need those common components. You can even parameterise it (with:) if you need subtle differences. I think the main catches with composite actions are needing to take care with getting caching right (do it within the composite, don't rely on external), and it won't inherit shell, so shell: bash inside it. Otherwise it's going to save the pain of having two almost the same workflows and one of them just having additional steps.
There's an alternative that may just be simpler: add workflow_dispatch to ci.yml with a boolean input defaulting to true (e.g. pdp_caching_test), then add the additional steps in there but if: github.event_name == 'workflow_dispatch' && inputs.pdp_caching_test. It would need the component override logic from here duplicated in but gated by github.event_name == 'workflow_dispatch', and there's a --notest diff between the two, but again, could be gated if that's necessary.
There was a problem hiding this comment.
@copilot this sounds interesting. Can you raise a PR doing this?
|
Seems fine to me, the Copilot notes are worth dealing with, and the huge semi-copypasta into the new workflow is a maintenance risk that we could minimise with a bit of crafting. |
|
@redpanda-f I've opened a new pull request, #63, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@redpanda-f I've opened a new pull request, #64, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@redpanda-f : I assume you'll re-request review when this is ready for others to look at again. |
…for synapse E2E Replace JS post-deploy-setup.js dependency with native cast send calls for client payment setup (ERC20 approve, FP deposit, FWSS operator approval). Switch from individual contract address env vars to NETWORK=devnet with mounted devnet-info.json. Update default synapse-sdk commit to v0.38.0.
redpanda-f
changed the title
redpanda-f
changed the title
feat: workflow testing for pdp-caching-layer being populated feat: devnet-setup is a separate composable action add: matrix for CI, allow testing with and without synapse-sdk remove: synapse-sdk dependency, part of #70 remove: e2e-test-pdp-caching-layers.yml remove: Testing Procedure document fix: include StdErr in logs, go uses it fix: matrix uses notest fix: constants add: fix: CURIO_LOG_LEVEL hoist fix: remove advertised_address so CQL does not fail on DDL Update src/commands/start/curio/db_setup.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
redpanda-f
force-pushed
the
feat/redpanda/allow-curio-debug-logs
branch
from
32be733 to
f7e15e2
Compare
|
@redpanda-f : since you changed the status from "⌨️ In Progress" to "🔎 Awaiting review", I assume you want developers to look at it? I know it's an extra step, but so they get notificatin, we have to request them to review again. I have done this. |
One-line summary
scripts/What changed (short)
src/commands/start/yugabyte/mod.rs— removed--advertise_address=0.0.0.0to fix gocql discovery/reconnect issues.src/commands/start/curio/{db_setup.rs,daemon.rs}— injectGOLOG_LOG_LEVEL=pdp=debuginto Curio containers for better PDP debugging.