Skip to content
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

Version details for DR operators #9956

Merged
merged 7 commits into from
Jul 24, 2024

Conversation

OdedViner
Copy link
Contributor

@OdedViner OdedViner requested a review from a team as a code owner June 17, 2024 12:01
@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines label Jun 17, 2024
@OdedViner OdedViner changed the title WIP: Version details for all DR components WIP: Version details for DR components Jun 17, 2024
@OdedViner OdedViner changed the title WIP: Version details for DR components WIP: Version details for DR operators Jun 17, 2024
@pull-request-size pull-request-size bot added size/L PR that changes 100-499 lines and removed size/S PR that changes 10-29 lines labels Jun 17, 2024
@OdedViner OdedViner changed the title WIP: Version details for DR operators Version details for DR operators Jun 18, 2024
Comment on lines 446 to 450
config._metadata["OCP DR Hub Operator"] = get_dr_hub_operator_version()
config._metadata[
"ODF Multicluster Orchestrator"
] = get_odf_multicluster_orchestrator_version()
config._metadata["GitOps Operator"] = get_ocp_gitops_operator_version()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those both on RDR and MDR setup as well?
If not we will need to have some conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the RDR and MDR are the same for versions @sidhant-agrawal correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, GitOps will be present for both

"OCP DR Cluster Operator"
] = get_ocp_dr_cluster_operator_version()
config._metadata["GitOps Operator"] = get_ocp_gitops_operator_version()
config._metadata["VolSync Operator "] = get_volsync_operator_version()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same question about diff between RDR/MDR like above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All operators mentioned in my comment #7904 (comment) are valid for RDR setup.
afaik, MDR doesn't have Submariner operator, rest operators are same.

)
for csv in csv_list:
if "advanced-cluster-management" in csv["metadata"]["name"]:
# extract version string
return csv["spec"]["version"]


def get_dr_hub_operator_version(namespace="openshift"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we all of those version related functions move to this modul?
https://github.com/red-hat-storage/ocs-ci/blob/master/ocs_ci/utility/version.py

I hope it will also resolve circular dependency so you will not need to have :

rom ocs_ci.ocs.resources.csv import get_csvs_start_with_prefix

in each of those functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue reproduced when import get_csvs_start_with_prefix function
I moved the new functions to version.py

 E   ImportError: cannot import name 'get_semantic_version' from partially initialized module 'ocs_ci.utility.version' (most likely due to a circular import) (/home/runner/work/ocs-ci/ocs-ci/ocs_ci/utility/version.py)

https://github.com/red-hat-storage/ocs-ci/actions/runs/9598093695/job/26468573010?pr=9956

@@ -432,6 +441,14 @@ def pytest_configure(config):
check_clusters()
if ocsci_config.RUN.get("cephcluster"):
gather_version_info_for_report(config)
if ocsci_config.MULTICLUSTER["acm_cluster"]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are just running deployment of OCP - will this try to collect also those details there, then it will probably fail as we don't have yet the deployment done yet?

If there will be any exception it will lead I guess to fail whole pytest as pytest_configure hook is very sensitive for whatever error causing pytest fail with Fatal error IIRC.

So we will need to check this please if it will cause any issue for whole deployment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the get_version function returns None if the operator does not exist

return csv["spec"]["version"]


def get_ocp_dr_cluster_operator_version(namespace="openshift"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will also need to add all of those values to test suite property via: record_testsuite_property

E.g. like we do record markers and other values:
0884315

See other usages of record_testsuite_property in code via git grep record_testsuite_property please .

As main goal is have those values recorded in email, and report portal and xunit file as well.

record_testsuite_property("rp_ocs_build", get_ocs_build_number())

IIRC whatever we want to have in Report portal we need to prefix with rp_ than our script will remove rp_ prefix and report the attribute without this prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@OdedViner
Copy link
Contributor Author

OdedViner commented Jun 26, 2024

XML examlpe:

<testsuites>
<testsuite name="OCPtestsuite" errors="0" failures="0" skipped="0" tests="1" time="634.377" timestamp="2024-06-26T09:00:32.514033" hostname="1223">
<properties>
<property name="run_id" value="1719381629"/>
<property name="rp_launch_name" value="OCS4-16-Downstream-OCP4-16-LSO-MON-HOSTPATH-OSD-NVME-BAREMETAL-UPI-1AZ-RHCOS-3M-3W-oviner"/>
<property name="rp_launch_description" value=""/>
<property name="rp_platform" value="baremetal"/>
<property name="rp_deployment_type" value="upi"/>
<property name="rp_downstream" value="True"/>
<property name="rp_worker_instance_type" value="m5.4xlarge"/>
<property name="rp_ocp_version" value="4.16"/>
<property name="rp_ocs_version" value="4.16"/>
<property name="rp_run_id" value="1719381629"/>
<property name="rp_production" value="True"/>
<property name="rp_markers" value=""/>
<property name="rp_acm_version" value="1.1"/>
<property name="rp_dr_hub_version" value="2.2"/>
<property name="rp_odf_multicluster_orchestrator_version" value="3.3"/>
<property name="rp_gitops_version" value="7.7"/>
<property name="rp_oadp_version" value="5.5"/>
<property name="rp_ocp_dr_cluster_version" value="6.6"/>
<property name="rp_gitops_version" value="7.7"/>
<property name="rp_volsync_version" value="8.8"/>
<property name="rp_submariner_version" value="9.9"/>
<property name="rp_ocs_build" value="4.16.0-130"/>
<property name="polarion-project-id" value="OpenShiftContainerStorage"/>
<property name="polarion-testrun-id" value="OCS4-16-Downstream-OCP4-16-LSO-MON-HOSTPATH-OSD-NVME-BAREMETAL-UPI-1AZ-RHCOS-3M-3W"/>
<property name="polarion-testrun-status-id" value="inprogress"/>
<property name="polarion-custom-isautomated" value="True"/>
</properties>
<testcase classname="tests.manage.z_cluster.test_bz.TestAcceptance" name="test_acceptance" time="634.322"/>
</testsuite>
</testsuites>

@vavuthu vavuthu added the team/ecosystem Ecosystem team related issues/PRs label Jun 26, 2024
@@ -152,3 +153,115 @@ def compare_versions(expression):
)
v1, op, v2 = m.groups()
return eval(f"get_semantic_version(v1, True){op}get_semantic_version(v2, True)")


def get_dr_hub_operator_version(namespace="openshift"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is "openshift" namespace for DR ? don't make namespace as hardcode since we support multi namespace feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is "openshift" namespace for DR ? -> DR operator applies on all namespaces, so I choose "openshift" ns

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you keep ns as constant. All the NS are defined here https://github.com/red-hat-storage/ocs-ci/blob/master/ocs_ci/ocs/constants.py#L251

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

ocs_ci/utility/version.py Show resolved Hide resolved
return csv["spec"]["version"]


def get_ocp_dr_cluster_operator_version(namespace="openshift"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is "openshift" namespace for DR ? don't make namespace as hardcode since we support multi namespace feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. "openshift" ns exist on all ocp cluster

ocs_ci/utility/version.py Show resolved Hide resolved

def get_dr_hub_operator_version(namespace="openshift"):
"""
Get DR Hub Opertaor Version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Get DR Hub Opertaor Version
Get DR Hub Operator Version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

ocs_ci/utility/version.py Show resolved Hide resolved
ocs_ci/utility/version.py Show resolved Hide resolved
Comment on lines 1594 to 1605
from ocs_ci.utility.version import (
get_dr_hub_operator_version,
get_ocp_dr_cluster_operator_version,
get_odf_multicluster_orchestrator_version,
get_ocp_gitops_operator_version,
get_submariner_operator_version,
get_volsync_operator_version,
)
from ocs_ci.utility.utils import (
get_oadp_version,
get_acm_version,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why its imported in the middle of function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

get_acm_version,
)

if ocsci_config.MULTICLUSTER["acm_cluster"]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is "acm_cluster" always loaded into MULTICLUSTER even normal deployment? I suggest to use get()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is configured in default_config.yaml but I will change it ... https://github.com/red-hat-storage/ocs-ci/blob/master/ocs_ci/framework/conf/default_config.yaml#L347

gitops_operator_version = get_ocp_gitops_operator_version()
if gitops_operator_version:
record_testsuite_property("rp_gitops_version", gitops_operator_version)
if ocsci_config.MULTICLUSTER["primary_cluster"]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use get method to avoid unnecessary failures

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link

@ocs-ci ocs-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR validation

Cluster Name:
Cluster Configuration:
PR Test Suite: deployment
PR Test Path: tests/
Additional Test Params:
OCP VERSION: 4.16
OCS VERSION: 4.16
tested against branch: master

Job PASSED.

Comment on lines 634 to 656
class RunWithConfigContext(object):
def __init__(self, config_index):
self.original_config_index = config.cur_index
self.config_index = config_index

def __enter__(self):
config.switch_ctx(self.config_index)
return self

def __exit__(self, exc_type, exc_value, exc_traceback):
config.switch_ctx(self.original_config_index)


class RunWithAcmConfigContext(RunWithConfigContext):
def __init__(self):
acm_index = get_all_acm_indexes()[0]
super().__init__(acm_index)


class RunWithPrimaryConfigContext(RunWithConfigContext):
def __init__(self):
primary_index = get_primary_cluster_config()
super().__init__(primary_index)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather see those in config class as those would be reused for other scenarios like provider/client mode.

https://github.com/red-hat-storage/ocs-ci/blob/master/ocs_ci/framework/__init__.py#L25

So user might access those like:

with config.RunWithAcmConfigContext: # or RunWithConfigContext directly or define new ones leveraging the generic one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

bool: return True if it is rdr or mdr setup otherwise False

"""
for cluster in ocsci_config.clusters:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OdedViner we don't have to iterate over all the clusters, since scenario would be passed as common conf in run-ci, it should be available in all contexts.

Suggested change
for cluster in ocsci_config.clusters:
if cluster.MULTICLUSTER['multicluster_mode'] in ("metro-dr", "regional-dr"):
return True

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -1592,6 +1603,49 @@ def additional_testsuite_properties(record_testsuite_property, pytestconfig):
# add markers as separated property
markers = ocsci_config.RUN["cli_params"].get("-m", "").replace(" ", "-")
record_testsuite_property("rp_markers", markers)
if is_dr_scenario():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OdedViner , looks like this block of code is mostly repetition of the block above https://github.com/red-hat-storage/ocs-ci/pull/9956/files#diff-715cca595e4d8f8493e6afe4e6605ebb2b8f91d3cb1be10495f2ec06e3826261R218, is it possible to optimize this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

petr-balogh
petr-balogh previously approved these changes Jul 24, 2024
Copy link

@ocs-ci ocs-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR validation

Cluster Name:
Cluster Configuration:
PR Test Suite: deployment
PR Test Path: tests/
Additional Test Params:
OCP VERSION: 4.16
OCS VERSION: 4.16
tested against branch: master

Job PASSED.

@OdedViner OdedViner added the Verified Mark when PR was verified and log provided label Jul 24, 2024
@OdedViner
Copy link
Contributor Author

Tested on MDR job:
https://url.corp.redhat.com/bb980c9

@@ -205,7 +205,11 @@ def get_version_info(namespace=None):
csv_name = package_manifest.get_current_csv(channel)
csv_pre = CSV(resource_name=csv_name, namespace=namespace)
info = get_images(csv_pre.get())
return info
from ocs_ci.ocs.utils import get_dr_operator_versions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move this to import sections

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you updated this function, could we add doc string as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got a Circular Import Error when i move to import section.
Added doc string.

ocs_ci/ocs/utils.py Show resolved Hide resolved
from ocs_ci.ocs.resources.csv import get_csvs_start_with_prefix

csv_list = get_csvs_start_with_prefix(
"openshift-gitops-operator", namespace=namespace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"openshift-gitops-operator", namespace=namespace
constants.GITOPS_OPERATOR_NAME, namespace=namespace

"openshift-gitops-operator", namespace=namespace
)
for csv in csv_list:
if "openshift-gitops-operator" in csv["metadata"]["name"]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if "openshift-gitops-operator" in csv["metadata"]["name"]:
if constants.GITOPS_OPERATOR_NAME in csv["metadata"]["name"]:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

versions_dic["oadp_version"] = oadp_operator_version
ocp_dr_cluster_operator_version = get_ocp_dr_cluster_operator_version()
if ocp_dr_cluster_operator_version:
versions_dic["ocp_dr_cluster_version"] = ocp_dr_cluster_operator_version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to dr_hub_version, we can use dr_cluster_version instead of ocp_dr_cluster_version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Signed-off-by: oviner <[email protected]>
Signed-off-by: oviner <[email protected]>
shylesh
shylesh previously approved these changes Jul 24, 2024
Copy link
Contributor

@shylesh shylesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Signed-off-by: oviner <[email protected]>
return csv["spec"]["version"]


def get_ocp_dr_cluster_operator_version(namespace=constants.OPENSHIFT_NAMESPACE):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to get_dr_hub_operator_version, can change to get_dr_cluster_operator_version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Signed-off-by: oviner <[email protected]>

def get_ocp_dr_cluster_operator_version(namespace=constants.OPENSHIFT_NAMESPACE):
"""
Get DR Hub Operator Version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Get DR Hub Operator Version
Get DR Cluster Operator Version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

vavuthu
vavuthu previously approved these changes Jul 24, 2024
Signed-off-by: oviner <[email protected]>
Copy link

openshift-ci bot commented Jul 24, 2024

New changes are detected. LGTM label has been removed.

Copy link

openshift-ci bot commented Jul 24, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: OdedViner, petr-balogh, sidhant-agrawal

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@petr-balogh petr-balogh merged commit ace9c72 into red-hat-storage:master Jul 24, 2024
5 of 6 checks passed
amr1ta pushed a commit to amr1ta/ocs-ci that referenced this pull request Jul 25, 2024
* Version details for all DR components

Signed-off-by: oviner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm size/L PR that changes 100-499 lines team/ecosystem Ecosystem team related issues/PRs Verified Mark when PR was verified and log provided
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants