fix: collect launch properties when some cluster lacks Kuadrant#983
fix: collect launch properties when some cluster lacks Kuadrant#983silvi-t wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughCollectors now use public cluster configurations and consult KubernetesClient.is_reachable and ocp_version. component_metadata includes disconnected-project placeholders and includes images without tags. Tests and fixtures were changed to iterate configured clusters/namespaces and record per-cluster properties, skipping missing namespaces. ChangesCluster metadata and test routing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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 `@testsuite/tests/conftest.py`:
- Around line 429-433: The openshift_version function returns None immediately
if project.change_project("default") is not connected; update it to try a
namespace fallback before returning None: after obtaining project =
testconfig["control_plane"]["cluster"].change_project("default"), if not
project.connected iterate available namespaces (e.g., from
testconfig["control_plane"]["cluster"] or
testconfig["control_plane"]["namespaces"]) and call change_project(namespace)
until you find a connected project; once a connected project is found continue
with the existing version detection logic in openshift_version, otherwise return
None.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b235959-c13d-435e-95f6-9b4fad93e867
📒 Files selected for processing (3)
testsuite/component_metadata.pytestsuite/tests/conftest.pytestsuite/tests/info_collector.py
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _first_connected(namespace): |
There was a problem hiding this comment.
Can you please remind me why this collector wasn't collecting information from every cluster, and is using only the first one it finds available?
There was a problem hiding this comment.
In our multicluster pipeline, all clusters share the same configuration. They're deployed with the same kuadrant-operator image, so the component images and versions are identical across clusters. The launch attributes this collector gathers (mostly Kuadrant component images extracted from the operator) would be duplicated if we collected from every cluster. Using only one cluster avoids that redundancy while still capturing all the relevant information.
This approach can change in the future if we need cluster-specific properties, but right now collecting from multiple clusters would only result in duplicate attributes.
There was a problem hiding this comment.
I would still prefer collecting the real information from each cluster, even if collect task would take 3x time.
I don't like that this already confusing process is getting refactored with the new confusing algorithm, I need to also be aware of now.
This might be the moment to change this, and assign None values to missing configurations instead of blindly trusting the install pipelines.
There was a problem hiding this comment.
I'd like to pull @zkraus into this conversation to get his opinion as well. My concern is that collecting attributes from every cluster would result in unnecessary duplication (since all clusters share the same operator image, the attributes would be identical). We'd either have duplicate keys or need cluster-prefixed names (e.g. cluster1_kuadrant-operator, cluster2_kuadrant-operator), which makes the launch attributes harder to read and filter on in Report Portal without adding useful information.
There was a problem hiding this comment.
I confirm, that the reason was to collect only one, because of assumption of unified environment.
But there should be no harm of collecting it. It will be preparation, if/when we start testing interoperability.
If the versions would be equal, I would not set duplicated attributes (if that is even possible), so maybe checking if there is already equal key and value. This should be simple enough, and we can change that if necessary.
On the other hand, as we will be getting the version and any data, from each cluster -- which is definitely beneficial -- please do use logger, and log the information collected per cluster in the collection test. Log as much as you think would be valuable. In the log, it is accessible, readable, and will not hurt anyone. will not overuse attributes. I think that is nearly ideal place to put a large amount of information, in case we need it later. -- Next upgrade, would be to put this data as an attachment in json/yaml format (next time).
TL;DR: I agree, collect info from all clusters, deduplicate attributes, log everything.
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
silvi-t
force-pushed
the
fix-launch-properties
branch
from
0e89998 to
abad3a7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
testsuite/tests/conftest.py (1)
48-50: 💤 Low valueConsider using a pytest marker instead of filename matching.
Bypassing skip/fail logic by checking
item.fspath.basename == "info_collector.py"works but is fragile—renaming the file or adding similar collector tests would require changes here. A dedicated marker (e.g.,@pytest.mark.skip_capability_check) would be more explicit and maintainable.That said, given the targeted scope of this fix and the likelihood of only one such file, this is acceptable for now.
🤖 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 `@testsuite/tests/conftest.py` around lines 48 - 50, Replace the brittle filename check that looks for item.fspath.basename == "info_collector.py" with a pytest marker-based guard: check whether the test item has a marker (e.g., "skip_capability_check") using item.get_closest_marker or item.iter_markers, and skip/return when that marker is present; also update the corresponding test (info_collector.py) to be decorated with `@pytest.mark.skip_capability_check` so the new marker-based logic in conftest.py (replacing the item.fspath.basename check) correctly identifies and excludes that test.
🤖 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.
Nitpick comments:
In `@testsuite/tests/conftest.py`:
- Around line 48-50: Replace the brittle filename check that looks for
item.fspath.basename == "info_collector.py" with a pytest marker-based guard:
check whether the test item has a marker (e.g., "skip_capability_check") using
item.get_closest_marker or item.iter_markers, and skip/return when that marker
is present; also update the corresponding test (info_collector.py) to be
decorated with `@pytest.mark.skip_capability_check` so the new marker-based logic
in conftest.py (replacing the item.fspath.basename check) correctly identifies
and excludes that test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c82d7df1-68c7-4561-89db-7d3c6a7a74f3
📒 Files selected for processing (4)
testsuite/component_metadata.pytestsuite/kubernetes/client.pytestsuite/tests/conftest.pytestsuite/tests/info_collector.py
🚧 Files skipped from review as they are similar to previous changes (1)
- testsuite/tests/info_collector.py
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
Brief summary of changes
|
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 `@testsuite/tests/info_collector.py`:
- Around line 90-107: The loop over _all_cluster_projects never records a
kube_context for the first usable cluster; add logic so that when you encounter
the first cluster where project is not None you append a single ("kube_context",
cluster_name) entry to properties (or call _record_unique with that pair) and
mark it done so it only records once; update the loop that references
cluster_name, cluster, project (and uses
ReportPortalMetadataCollector.get_kubernetes_version, _record_unique,
record_testsuite_property) to set this flag and add the kube_context tuple
before continuing to collect versions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c7d5c24-adab-4476-ba7b-3adb7a45ae73
📒 Files selected for processing (2)
testsuite/component_metadata.pytestsuite/tests/info_collector.py
3 participants
Description
has_kuadrant()check, and theopenshift_versionfixture cascaded a failure through theclusterfixture (which requires thekuadrantnamespace)make collect/ info_collector more resilient: properties are now scraped from the first cluster that has the required components, so we still get records from the remaining clustersnot installedinstead of being omittedChanges
Bug Fixes
has_kuadrant()check inpytest_runtest_setupforinfo_collector.py— it handles missing components gracefully via try/exceptopenshift_versionfixture from theclusterfixture by connecting to thedefaultnamespace instead, so OCP version detection works regardless of thekuadrantnamespaceEnhancements
_first_connected()helper ininfo_collector.pythat iterates all configured clusters and returns the first(cluster_client, project)pair with a connected namespacegather_cluster_versions(),test_kube_context,test_kuadrant_properties, andtest_istio_propertiesto fall back to secondary clusters when the first lackskuadrant-systemoristio-system_collect_single_clusterincomponent_metadata.pyto fall back to thedefaultnamespace and reportkuadrant_image: not installedfor clusters missing KuadrantVerification steps
kuadrant_image: not installedfor clusters missing KuadrantSummary by CodeRabbit
New Features
Bug Fixes