OTA-1974: Teach Cincinnati to include product information when serving /graph-data - #1073
OTA-1974: Teach Cincinnati to include product information when serving /graph-data#1073racheljpg wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@racheljpg: This pull request references OTA-1974 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 story 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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:
WalkthroughAdds configurable product lifecycle polling, shared product-data state, graph-builder propagation, and inclusion of ChangesProduct lifecycle data integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProductFetcher
participant ProductAPI
participant GraphState
participant GraphRunner
participant MetadataScraper
ProductFetcher->>ProductAPI: Fetch product lifecycle JSON with ETag
ProductAPI-->>ProductFetcher: Return JSON or 304 response
ProductFetcher->>GraphState: Persist data and saved path
GraphRunner->>GraphState: Read product data path
GraphRunner->>MetadataScraper: Pass PRODUCT_DATA_PARAM_KEY
MetadataScraper->>MetadataScraper: Copy file as products.json
MetadataScraper-->>GraphRunner: Continue graph archive creation
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (5)
graph-builder/src/config/settings.rs (1)
86-101: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftDefaulting
product_enabledtotruemakes external network fetching mandatory by default.Prior to this change, graph-builder had no dependency on an external Red Hat API. With
product_enableddefaulting totrue, every existing deployment (including disconnected/airgapped OpenShift clusters, a common Cincinnati deployment scenario per the project's architecture) will now attempt to pollaccess.redhat.comunless explicitly disabled via config. Worth confirming whether fetch failures degrade gracefully (background thread) rather than blocking startup or/graph-dataavailability — the fetcher implementation isn't in this review batch.Consider defaulting
product_enabledtofalse(opt-in) to avoid surprising existing deployments with a new mandatory external dependency, or clearly document this behavior change in release/migration notes.🤖 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 `@graph-builder/src/config/settings.rs` around lines 86 - 101, Default product lifecycle fetching to opt-in so existing and disconnected deployments do not contact the external Red Hat API unexpectedly. Update the default value for product_enabled in the settings configuration from true to false, while preserving the existing API URL, polling interval, and timeout defaults.graph-builder/src/graph.rs (2)
155-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProduct-lifecycle fields are
pub, unlike the rest ofState.
json,mandatory_params,live,ready,secondary_metadataare all private with accessor methods (is_live,is_ready), but the newproduct_data/product_etag/product_data_patharepub, directly exposing mutable internal state outside the module. Since the only external writer isproduct_lifecycle.rswithin the same crate,pub(crate)preserves current usage while restoring encapsulation.♻️ Proposed fix
/// Product lifecycle JSON data - pub product_data: Arc<RwLock<String>>, + pub(crate) product_data: Arc<RwLock<String>>, /// Last ETag from product lifecycle API - pub product_etag: Arc<RwLock<Option<String>>>, + pub(crate) product_etag: Arc<RwLock<Option<String>>>, /// Path to temporary products.json file - pub product_data_path: Arc<RwLock<Option<std::path::PathBuf>>>, + pub(crate) product_data_path: Arc<RwLock<Option<std::path::PathBuf>>>,🤖 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 `@graph-builder/src/graph.rs` around lines 155 - 160, Change the visibility of State fields product_data, product_etag, and product_data_path from pub to pub(crate), preserving access for product_lifecycle.rs within the crate while preventing external direct mutation and matching the encapsulation of the other State fields.
241-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent drop when product data path isn't valid UTF-8.
If
path.to_str()returnsNone, the parameter is silently omitted with no log, making it hard to diagnose whyproducts.jsonnever appears in the archive. Worth a warning log for visibility.♻️ Proposed fix
if let Some(ref path) = *state.product_data_path.read() { if let Some(path_str) = path.to_str() { parameters.insert( commons::PRODUCT_DATA_PARAM_KEY.to_string(), path_str.to_string(), ); + } else { + warn!("product data path is not valid UTF-8, skipping: {:?}", path); } }🤖 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 `@graph-builder/src/graph.rs` around lines 241 - 257, Add a warning log in the product-data parameter preparation block before or when `path.to_str()` returns `None`, identifying the invalid UTF-8 path and explaining that `commons::PRODUCT_DATA_PARAM_KEY` was omitted. Keep the existing insertion behavior for valid paths and use the component’s existing logging facility.cincinnati/src/plugins/internal/graph_builder/dkrv2_openshift_secondary_metadata_scraper/plugin.rs (1)
365-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated product-copy logic across both scraper plugins.
This exact block (lookup
PRODUCT_DATA_PARAM_KEY, check existence, copy toproducts.json, log success/failure) is duplicated verbatim ingithub_openshift_secondary_metadata_scraper/plugin.rs(lines 526-542). Since both scrapers already sharecommons::create_tar, consider extracting this into a sharedcommonshelper to avoid the two copies diverging over time.♻️ Proposed shared helper
// commons/src/lib.rs pub async fn copy_product_data(parameters: &std::collections::HashMap<String, String>, dest_dir: &std::path::Path) { if let Some(product_path) = parameters.get(PRODUCT_DATA_PARAM_KEY) { let source_path = std::path::Path::new(product_path); if source_path.exists() { let dest_path = dest_dir.join("products.json"); match tokio::fs::copy(source_path, &dest_path).await { Ok(_) => log::info!("Copied product lifecycle data to graph-data tarball"), Err(e) => log::warn!("Failed to copy product lifecycle data: {}", e), } } } }- // Copy product lifecycle data if available - if let Some(product_path) = io.parameters.get(PRODUCT_DATA_PARAM_KEY) { - let source_path = std::path::Path::new(product_path); - if source_path.exists() { - let dest_path = graph_data_path.join("products.json"); - match tokio::fs::copy(source_path, &dest_path).await { - Ok(_) => { - log::info!("Copied product lifecycle data to graph-data tarball"); - } - Err(e) => { - log::warn!("Failed to copy product lifecycle data: {}", e); - // Don't fail - continue without product data - } - } - } - } + commons::copy_product_data(&io.parameters, &graph_data_path).await;🤖 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 `@cincinnati/src/plugins/internal/graph_builder/dkrv2_openshift_secondary_metadata_scraper/plugin.rs` around lines 365 - 381, Product lifecycle data copying is duplicated across both scraper plugins. Extract the shared lookup, existence check, copy, and logging logic into a reusable async commons helper such as copy_product_data, then replace the duplicate blocks in both scraper plugins with calls to it, passing their parameters and graph-data destination directory.graph-builder/src/product_lifecycle.rs (1)
140-198: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo backoff on repeated fetch failures.
On error the loop simply continues to the next
poll_intervalsleep with no backoff, so a persistently failing upstream API is retried at the same fixed cadence indefinitely. Consider a simple backoff (capped) on consecutive failures to reduce load on a struggling upstream.🤖 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 `@graph-builder/src/product_lifecycle.rs` around lines 140 - 198, Implement capped exponential backoff for consecutive failures in the product lifecycle polling loop. Track consecutive fetch or save/validation failures around the loop using the existing fetch logic, increase the delay after each failure up to a maximum, and reset the failure count/backoff after a successful fetch and save; use the backoff delay for the next retry while preserving the normal poll interval on success.
🤖 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 `@graph-builder/src/product_lifecycle.rs`:
- Around line 97-106: Update save_to_file to avoid the predictable shared path
and non-atomic File::create write: create a securely randomized tempfile using
tempfile::NamedTempFile, write and sync the JSON, then atomically persist/rename
it to the expected cincinnati-products.json path, handling cleanup and errors
appropriately.
---
Nitpick comments:
In
`@cincinnati/src/plugins/internal/graph_builder/dkrv2_openshift_secondary_metadata_scraper/plugin.rs`:
- Around line 365-381: Product lifecycle data copying is duplicated across both
scraper plugins. Extract the shared lookup, existence check, copy, and logging
logic into a reusable async commons helper such as copy_product_data, then
replace the duplicate blocks in both scraper plugins with calls to it, passing
their parameters and graph-data destination directory.
In `@graph-builder/src/config/settings.rs`:
- Around line 86-101: Default product lifecycle fetching to opt-in so existing
and disconnected deployments do not contact the external Red Hat API
unexpectedly. Update the default value for product_enabled in the settings
configuration from true to false, while preserving the existing API URL, polling
interval, and timeout defaults.
In `@graph-builder/src/graph.rs`:
- Around line 155-160: Change the visibility of State fields product_data,
product_etag, and product_data_path from pub to pub(crate), preserving access
for product_lifecycle.rs within the crate while preventing external direct
mutation and matching the encapsulation of the other State fields.
- Around line 241-257: Add a warning log in the product-data parameter
preparation block before or when `path.to_str()` returns `None`, identifying the
invalid UTF-8 path and explaining that `commons::PRODUCT_DATA_PARAM_KEY` was
omitted. Keep the existing insertion behavior for valid paths and use the
component’s existing logging facility.
In `@graph-builder/src/product_lifecycle.rs`:
- Around line 140-198: Implement capped exponential backoff for consecutive
failures in the product lifecycle polling loop. Track consecutive fetch or
save/validation failures around the loop using the existing fetch logic,
increase the delay after each failure up to a maximum, and reset the failure
count/backoff after a successful fetch and save; use the backoff delay for the
next retry while preserving the normal poll interval on success.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f5e2a557-dfda-456e-b123-f9efe1318dfa
📒 Files selected for processing (11)
cincinnati/src/plugins/internal/graph_builder/dkrv2_openshift_secondary_metadata_scraper/plugin.rscincinnati/src/plugins/internal/graph_builder/github_openshift_secondary_metadata_scraper/plugin.rscommons/src/lib.rsgraph-builder/src/config/cli.rsgraph-builder/src/config/file.rsgraph-builder/src/config/options.rsgraph-builder/src/config/settings.rsgraph-builder/src/graph.rsgraph-builder/src/lib.rsgraph-builder/src/main.rsgraph-builder/src/product_lifecycle.rs
ankitathomas
left a comment
ankitathomas
left a comment
There was a problem hiding this comment.
This looks solid, I'd say it should be okay to merge once the tests and the coderabbit comments are addressed for the tempfile.
Great work!
adcf7e8 to
5a6afd6
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@cincinnati/src/plugins/internal/graph_builder/github_openshift_secondary_metadata_scraper/plugin.rs`:
- Around line 526-542: The archive refresh decision must also detect changes to
the file configured by PRODUCT_DATA_PARAM_KEY, rather than relying only on
secondary-metadata commit changes. Update the plugin’s refresh/build logic to
compare product-data freshness and regenerate products.json and the tarball when
it changes, while preserving existing behavior when neither source changes. Add
a regression test covering a product-data-only update.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d8a7864e-3ffe-4a49-ba8f-48f21fb21549
📒 Files selected for processing (11)
cincinnati/src/plugins/internal/graph_builder/dkrv2_openshift_secondary_metadata_scraper/plugin.rscincinnati/src/plugins/internal/graph_builder/github_openshift_secondary_metadata_scraper/plugin.rscommons/src/lib.rsgraph-builder/src/config/cli.rsgraph-builder/src/config/file.rsgraph-builder/src/config/options.rsgraph-builder/src/config/settings.rsgraph-builder/src/graph.rsgraph-builder/src/lib.rsgraph-builder/src/main.rsgraph-builder/src/product_lifecycle.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- cincinnati/src/plugins/internal/graph_builder/dkrv2_openshift_secondary_metadata_scraper/plugin.rs
- graph-builder/src/config/file.rs
- graph-builder/src/product_lifecycle.rs
- commons/src/lib.rs
- graph-builder/src/config/options.rs
- graph-builder/src/config/cli.rs
- graph-builder/src/config/settings.rs
- graph-builder/src/graph.rs
- graph-builder/src/main.rs
65e0673 to
a415e48
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
graph-builder/src/main.rs (2)
79-81: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUpdate all
State::newcallers.
State::newnow requiresproduct_data,product_etag, andproduct_data_path, butmock_stateat Lines 241-249 still passes the old seven-argument form. The test target will not compile until those shared fields are initialized there.🤖 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 `@graph-builder/src/main.rs` around lines 79 - 81, Update the mock_state State::new call to use the current signature by supplying initialized product_data, product_etag, and product_data_path values alongside the existing arguments. Ensure all State::new callers, especially mock_state, compile with the shared product fields populated.
95-99: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAvoid using
settingsafter moving it into the graph thread.The
moveclosure at Lines 87-90 capturessettingsby value, but this new block accessessettings.product_enabled,settings.product_api_url, and related fields afterward. Rust will reject this with a use-after-move error. Extract these values before spawning the graph thread or clonesettings.🤖 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 `@graph-builder/src/main.rs` around lines 95 - 99, Move the product configuration extraction before the graph thread’s move closure captures settings, then use the extracted product_enabled, product_api_url, product_poll_interval_secs, and product_timeout_secs values afterward. Do not access settings after it has been moved into the graph thread.
🤖 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 `@graph-builder/src/product_lifecycle.rs`:
- Line 187: Update the HTTP polling flow’s Ok((None, new_etag)) branch to set
PRODUCT_LIFECYCLE_LAST_FETCH to the current timestamp, matching the successful
JSON-response path while preserving the existing ETag handling.
---
Outside diff comments:
In `@graph-builder/src/main.rs`:
- Around line 79-81: Update the mock_state State::new call to use the current
signature by supplying initialized product_data, product_etag, and
product_data_path values alongside the existing arguments. Ensure all State::new
callers, especially mock_state, compile with the shared product fields
populated.
- Around line 95-99: Move the product configuration extraction before the graph
thread’s move closure captures settings, then use the extracted product_enabled,
product_api_url, product_poll_interval_secs, and product_timeout_secs values
afterward. Do not access settings after it has been moved into the graph thread.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e16b81dd-d07f-4fae-af6e-ed3e9fbed5d9
📒 Files selected for processing (4)
cincinnati/src/plugins/internal/graph_builder/dkrv2_openshift_secondary_metadata_scraper/plugin.rscincinnati/src/plugins/internal/graph_builder/github_openshift_secondary_metadata_scraper/plugin.rsgraph-builder/src/main.rsgraph-builder/src/product_lifecycle.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- cincinnati/src/plugins/internal/graph_builder/github_openshift_secondary_metadata_scraper/plugin.rs
- cincinnati/src/plugins/internal/graph_builder/dkrv2_openshift_secondary_metadata_scraper/plugin.rs
a415e48 to
01f1378
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
graph-builder/src/product_lifecycle.rs (2)
96-115: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse a private temp directory for product data.
graph-builder/src/product_lifecycle.rs:96-115writes every instance to<TMPDIR>/cincinnati-products.json, so concurrent graph-builder processes can overwrite each other’s file andgraph.rsmay package the wrong product data. Persistproducts.jsoninside a per-taskTempDirinstead.🤖 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 `@graph-builder/src/product_lifecycle.rs` around lines 96 - 115, Update save_to_file to create a per-task tempfile::TempDir and place products.json inside it, rather than using the shared system temporary directory and fixed cincinnati-products.json path. Persist the temporary file within that private directory and return the resulting path while keeping the existing write and sync error handling.
72-91: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the decoded response body.
response.text().awaitbuffers the full gzip-decoded payload, so a large or malicious product API response can exhaust memory; stream it with a configurable maximum decoded size and reject overflow.🤖 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 `@graph-builder/src/product_lifecycle.rs` around lines 72 - 91, Update the response-body handling in the product lifecycle request flow to stream decoded bytes incrementally instead of calling response.text().await. Enforce a configurable maximum decoded body size, reject responses exceeding that limit, and preserve the existing parsing and error behavior for bodies within the limit.
🤖 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.
Outside diff comments:
In `@graph-builder/src/product_lifecycle.rs`:
- Around line 96-115: Update save_to_file to create a per-task tempfile::TempDir
and place products.json inside it, rather than using the shared system temporary
directory and fixed cincinnati-products.json path. Persist the temporary file
within that private directory and return the resulting path while keeping the
existing write and sync error handling.
- Around line 72-91: Update the response-body handling in the product lifecycle
request flow to stream decoded bytes incrementally instead of calling
response.text().await. Enforce a configurable maximum decoded body size, reject
responses exceeding that limit, and preserve the existing parsing and error
behavior for bodies within the limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d82b0e91-2509-47d5-8f29-6c1389f3f3c9
📒 Files selected for processing (4)
cincinnati/src/plugins/internal/graph_builder/dkrv2_openshift_secondary_metadata_scraper/plugin.rscincinnati/src/plugins/internal/graph_builder/github_openshift_secondary_metadata_scraper/plugin.rsgraph-builder/src/main.rsgraph-builder/src/product_lifecycle.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- cincinnati/src/plugins/internal/graph_builder/github_openshift_secondary_metadata_scraper/plugin.rs
- graph-builder/src/main.rs
- cincinnati/src/plugins/internal/graph_builder/dkrv2_openshift_secondary_metadata_scraper/plugin.rs
01f1378 to
b0b669f
Compare
|
Hello! Now that all tests are passing I did some testing to make sure this PR is doing what we want. This looks good! Also queried the metrics endpoint to make sure the new metrics were being populated, and they are, under the cincinnato_gb_ prefix: If anyone with some more experience than me with Cincinnati has any other testing suggestions, I am happy to hear them :) |
|
Thanks for your previous review @ankitathomas - would appreciate if you had the chance to look again now that the tests are passing and I've done some testing too :) thanks! |
b0b669f to
256234e
Compare
|
Had to rebase to merge two commits together so the tests will have to run again |
|
/retest |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ankitathomas, racheljpg The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
@racheljpg hey, nice work on this! Left a few suggestions below, mostly cleanup stuff. Happy to discuss any of them.
1. product_data and product_etag don't need to live on State
Both product_data (the full JSON body) and product_etag are only read/written inside product_lifecycle::run() - nothing else in the codebase touches them. Only product_data_path is actually shared (read in graph.rs to pass the path to plugins).
These two can be local variables inside the run() function instead of Arc<RwLock<>> fields on the shared State struct. That drops State::new from 10 params to 8 and removes two unnecessary locks.
// product_lifecycle.rs – run()
let mut last_etag: Option<String> = None;
// ... in the polling loop:
// instead of: let last_etag = state.product_etag.read().clone();
// just use the local `last_etag` directly
// on success:
last_etag = new_etag;
*state.product_data_path.write() = Some(path.clone());
// on 304:
if let Some(etag) = new_etag {
last_etag = Some(etag);
}2. Duplicated product copy logic in both scrapers
The 15-line copy block in dkrv2_openshift_secondary_metadata_scraper/plugin.rs and github_openshift_secondary_metadata_scraper/plugin.rs is essentially identical. Worth extracting to a shared helper ... something like:
// commons/src/lib.rs (or graph_builder commons)
pub async fn copy_product_data_if_available(
io: &cincinnati::plugins::InternalIO,
dest_dir: &std::path::Path,
) {
if let Some(product_path) = io.parameters.get(PRODUCT_DATA_PARAM_KEY) {
let source_path = std::path::Path::new(product_path);
if source_path.exists() {
let dest_path = dest_dir.join("products.json");
match tokio::fs::copy(source_path, &dest_path).await {
Ok(_) => log::info!("Copied product lifecycle data to graph-data tarball"),
Err(e) => log::warn!("Failed to copy product lifecycle data: {}", e),
}
}
}
}Then both scrapers become a one-liner before create_tar.
3. Don't spawn the polling task when disabled
When product_enabled is false, the run() function sits in loop { sleep(86400) } forever.... occupying a tokio task slot doing nothing. Simpler to just not spawn it:
// main.rs
if product_enabled {
let product_state = state.clone();
tokio::spawn(async move {
graph_builder::product_lifecycle::run(
product_api_url,
product_poll_interval_secs,
product_timeout_secs,
product_state,
)
.await
});
} else {
info!("Product lifecycle fetching is disabled");
}Then run() can drop the enabled parameter and the disabled-branch entirely, and the return type doesn't need to be -> !.
4. Default product_enabled to false
Currently defaults to true in settings.rs, which means existing deployments will start hitting access.redhat.com on upgrade with no opt-in. Suggest defaulting to false so it's explicitly enabled in deployments that want it:
#[default(false)]
pub product_enabled: bool,5. chrono usage
chrono::Utc::now().timestamp() is used once here and once in graph.rs. Both can use stdlib instead, which would let you eventually drop the chrono direct dependency from graph-builder/Cargo.toml. Not urgent, but since we're touching the area:
use std::time::{SystemTime, UNIX_EPOCH};
PRODUCT_LIFECYCLE_LAST_FETCH.set(
SystemTime::now()
.duration_since(UNIX_EPOCH)
.unwrap_or_default()
.as_secs() as f64,
);(Same pattern could be applied to graph.rs:309 in a follow-up.)
|
New changes are detected. LGTM label has been removed. |
1c9f50e to
71d951d
Compare
e02604e to
f367cef
Compare
|
/retest |
|
Thanks Jakub, I implemented all your feedback and all the tests are passing again :) |
|
|
||
| GRAPH_LAST_SUCCESSFUL_REFRESH.set(chrono::Utc::now().timestamp() as i64); | ||
| GRAPH_LAST_SUCCESSFUL_REFRESH.set( | ||
| std::time::SystemTime::now() |
There was a problem hiding this comment.
What's going on with this change away from chrono? We've been using chrono since 43f39e8 (#136), and while I like being able to use the stdlib and have one fewer dep to keep track of, I'd like to see some notes in the commit message explaining what this change is about. Was the use of a non-stdlib dep unneccessary from the start? Or has the stdlib grown functionality that obsoleted a previously-valid need for a dep? Or is this actually changing user-visible behavior? Or...?
There was a problem hiding this comment.
Hi! It doesn't change anything for the user, both these outputs produce the same Unix timestamp value.
I don't have any strong opinions on this at all. Do you think it's best that it's kept as it is rather than removing it?
| // Product lifecycle fetcher | ||
| if product_enabled { | ||
| let product_state = Arc::new(state.clone()); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Comparing with a few lines up where we're using thread::spawn, is there a reason to use tokio::spawn here? Is the expectation that graph::run is heavy (on memory?) and needs its own OS-level thread, while graph_builder::product_lifecycle::run is light, and can use... whatever Tokio is configured to use?
There was a problem hiding this comment.
My understanding here is that since this is an async function, tokio::spawn is used instead of graph::run, because graph::run is synchronous and would block the runtime. I'm new to this codebase though, so if that's incorrect/not an inefficient way to do things, let me know
| // 304 Not Modified - data hasn't changed | ||
| PRODUCT_LIFECYCLE_NOT_MODIFIED.inc(); | ||
| info!("Product lifecycle data not modified (304)"); | ||
| return Ok((None, new_etag)); |
There was a problem hiding this comment.
I got nervous about "why would we have a changed etag here in a 304 response?", and found:
If a stored response is selected for update, the cache MUST:
...
- use other header fields provided in the 304 (Not Modified) response to replace all instances of the corresponding header fields in the stored response.
So returning the new etag here is required, even if I don't understand why a 304 response would have a different etag.
| /// Save product JSON to a temporary file | ||
| fn save_to_file(json: &str) -> Fallible<PathBuf> { | ||
| let temp_dir = std::env::temp_dir(); | ||
| let final_path = temp_dir.join("cincinnati-products.json"); |
There was a problem hiding this comment.
We can diverge from copy_product_data_if_available's products.json target here, but personally, I don't see the need for using a new name. Can we just use products.json here too?
| api_url: &str, | ||
| last_etag: Option<&str>, | ||
| ) -> Fallible<(Option<String>, Option<String>)> { | ||
| let mut request = client.get(api_url); |
There was a problem hiding this comment.
products.json in copy_product_data_if_available assumes the response is JSON. Can we make that an explicit part of the request by setting Accept: application/json in our request?
| .context("Failed to sync temporary file")?; | ||
|
|
||
| temp_file | ||
| .persist(&final_path) |
There was a problem hiding this comment.
Writing to a temporary file and then atomically renaming it to a final path is a good pattern when the final path is consumed by other parallel readers. But in this save_to_file function, final_path is inside a temporary directory that's unique to this function, and the consumer blocks until save_to_file completes. So I don't see the upside to doing the write/sync/rename dance in this situation. Can't we just write (and maybe sync?).
There was a problem hiding this comment.
But in this
save_to_filefunction,final_pathis inside a temporary directory that's unique to this function...
Ah, I'd misread. std::env::temp_dir(); is just giving us the standard temporary directory (like /tmp) and not a new temporary directory unique to this function.
... and the consumer blocks until
save_to_filecompletes.
But this is still true, right? So the rename dance would be protecting us against... a later poll starting to write the file while an existing copy_product_data_if_available reader was copying it out into a graph-data directory? In that case, I'm on board with the write/sync/rename dance, but think we probably want to explain this situation in the commit message to make it more accessable to future developers.
There was a problem hiding this comment.
Yes, that's correct, it is to save us from reads during a write.
Do you think that leaving some relevant comments in the code is sufficient, or do you think it would be best to break the PR into smaller commits with commit messages? Personally I thought it was tidy to keep this all in one commit, but what are your thoughts?
f367cef to
b5d2246
Compare
b5d2246 to
e82a7ef
Compare
|
@racheljpg: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Hello! This is a PR to for OTA-1974. Thanks!
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Bug Fixes / Validation
Assisted-by: Claude Code