-
Notifications
You must be signed in to change notification settings - Fork 26
fix: collect launch properties when some cluster lacks Kuadrant #983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,19 +30,30 @@ | |
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _first_connected(namespace): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I confirm, that the reason was to collect only one, because of assumption of unified environment. 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. |
||
| """Return the first (cluster_client, project) connected to the given namespace, or (None, None).""" | ||
| for _, cluster in ReportPortalMetadataCollector.get_cluster_configurations(): | ||
| project = cluster.change_project(namespace) | ||
| if project.connected: | ||
| return cluster, project | ||
| return None, None | ||
|
|
||
|
|
||
| pytestmark = pytest.mark.skipif( | ||
| not os.environ.get("COLLECTOR_ENABLE"), | ||
| reason="collector was not explicitly enabled", | ||
| ) | ||
|
|
||
|
|
||
| def gather_cluster_versions() -> dict: | ||
| """gather all particular versions into a dictionary""" | ||
| cluster_client = settings["control_plane"]["cluster"] | ||
| project = cluster_client.change_project(settings["service_protection"]["system_project"]) | ||
| """Gather cluster versions from the first cluster with Kuadrant system namespace.""" | ||
| cluster, project = _first_connected(settings["service_protection"]["system_project"]) | ||
| if project is None: | ||
| return {} | ||
| return { | ||
| "kubernetes": ReportPortalMetadataCollector.get_kubernetes_version(project), | ||
| "openshift": ReportPortalMetadataCollector.get_ocp_version(project), | ||
| "openshift": cluster.ocp_version, | ||
| } | ||
|
|
||
|
|
||
|
|
@@ -79,17 +90,21 @@ def test_cluster_properties(record_testsuite_property): | |
|
|
||
|
|
||
| def test_kube_context(record_testsuite_property): | ||
| """Record current kube context""" | ||
| kube_context = settings["control_plane"]["cluster"].kubeconfig_path | ||
| """Record kube context from the first cluster with kuadrant-system.""" | ||
| cluster_client, _ = _first_connected(settings["service_protection"]["system_project"]) | ||
| if cluster_client is None: | ||
| return | ||
| kube_context = cluster_client.kubeconfig_path | ||
| print(f"{kube_context=}") | ||
| if kube_context: | ||
| record_testsuite_property("kube_context", kube_context) | ||
|
|
||
|
|
||
| def test_kuadrant_properties(record_testsuite_property): | ||
| """Record kuadrant related properties""" | ||
| cluster_client = settings["control_plane"]["cluster"] | ||
| project = cluster_client.change_project("kuadrant-system") | ||
| """Record kuadrant related properties from the first cluster with kuadrant-system.""" | ||
| _, project = _first_connected(settings["service_protection"]["system_project"]) | ||
| if project is None: | ||
| return | ||
| kuadrant_images = ReportPortalMetadataCollector.get_component_images(project) | ||
| if kuadrant_images: | ||
| print(f"Kuadrant images: {kuadrant_images}") | ||
|
|
@@ -98,9 +113,10 @@ def test_kuadrant_properties(record_testsuite_property): | |
|
|
||
|
|
||
| def test_istio_properties(record_testsuite_property): | ||
| """Record Istio related properties""" | ||
| cluster_client = settings["control_plane"]["cluster"] | ||
| project = cluster_client.change_project("istio-system") | ||
| """Record Istio related properties from the first cluster with istio-system.""" | ||
| _, project = _first_connected("istio-system") | ||
| if project is None: | ||
| return | ||
| istio_metadata = ReportPortalMetadataCollector.get_istio_metadata(project) | ||
| for key, value in istio_metadata.items(): | ||
| print(f"{key}: {value}") | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.