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

WIP: Create Cluster via ACM #8935

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

OdedViner
Copy link
Contributor

@OdedViner OdedViner commented Nov 26, 2023

multicluster 2 tests/ecosystem/deployment/test_acm.py::test_acm_create --dev-mode  --skip-download-client --default-cluster-context-index 1 --cluster1 --cluster-name oviner-hub --cluster-path ~/ClusterPath --ocsci-conf /home/oviner/vSphere7-DC-CP_VC1.yaml --ocsci-conf  conf/deployment/vsphere/acm/ipi_1az_rhcos_vmfs_3m_3w_acm_hub.yaml  --cluster2  --cluster-name oviner-slave --cluster-path ~/ClusterPath --ocsci-conf /home/oviner/vSphere7-DC-CP_VC1.yaml --ocsci-conf  conf/deployment/vsphere/acm/ipi_1az_rhcos_vmfs_3m_3w_acm_slave.yam

@OdedViner OdedViner requested a review from a team as a code owner November 26, 2023 11:03
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines label Nov 26, 2023
Copy link

openshift-ci bot commented Nov 26, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: OdedViner

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

@OdedViner OdedViner changed the title Create Cluster via ACM WIP: Create Cluster via ACM Nov 26, 2023
@pull-request-size pull-request-size bot added size/XL and removed size/L PR that changes 100-499 lines labels Dec 12, 2023


@purple_squad
@acm_import
Copy link
Member

Choose a reason for hiding this comment

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

Suppose to be acm_install and not import.

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/ocs/acm/acm.py Outdated Show resolved Hide resolved
@@ -334,3 +334,18 @@ MULTICLUSTER:
acm_cluster: False
primary_cluster: False
active_acm_cluster: False

ACM_CONFIG:
ocp_image: "img4.14.3-multi-appsub"
Copy link
Member

Choose a reason for hiding this comment

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

Is there an option to use 4.15? As master is default to 4.15.
Also I would move this option to OCP version config file.

Please document all of this values and section in the config/README.md

memoryMB: 65536
diskSizeGB: 120
replicas: 3
cluster_name: "cluster-name"
Copy link
Member

Choose a reason for hiding this comment

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

Should not be defined here but rather used from ENV_DATA["cluster_name"]

Comment on lines +342 to +357
cpus: 16
coresPerSocket: 2
memoryMB: 65536
diskSizeGB: 120
replicas: 3
Copy link
Member

Choose a reason for hiding this comment

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

This we already have in env_data section, we should not duplicate it here but rather use what we already have at's it's a per cluster configuration which will be loaded per cluster.

If you need extra different configuration for cluster being deployed via ACM I suggest to create extra config file for it which will be loaded for that specific cluster index . E.g. conf/deployment/vsphere/acm_1az_rhcos_vsan_3m_3w.yaml

You can also add flag in this config file like:

ENV_DATA:
  amc_deployed: true
  all_other_values like (worker_replicas, worker_num_cpus) # Follow same naming convention like we do in other vsphere config files:  https://github.com/red-hat-storage/ocs-ci/blob/master/conf/deployment/vsphere/upi_1az_rhcos_vsan_3m_3w.yaml

Maybe you don't even need new ACM_CONFIG but you can just use ENV_DATA.
We should not put those values in default config but move to one specific mentioned above for this kind of deployment via ACM over vsphere for example.

@@ -41,6 +44,11 @@
from ocs_ci.ocs.resources.ocs import OCS
from ocs_ci.helpers.helpers import create_project

# from ocs_ci.utility.retry import retry
Copy link
Member

Choose a reason for hiding this comment

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

Leftovers?

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

with open(constants.VSPHERE_CA_FILE_PATH, "r") as fp:
self.vsphere_ca = fp.read()

with open("/home/oviner/OCS-AP/ocs-ci/data/pull-secret", "r") as fp:
Copy link
Member

Choose a reason for hiding this comment

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

Don't forgot to read it from the framework constants/ defaults or config or where we have it defined.

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/ocs/acm/acm.py Outdated Show resolved Hide resolved
logging.info(f"Tmp file name: {manifest.name}")
templating.dump_data_to_temp_yaml(data, manifest.name)
self.file_paths.append(manifest.name)
# run_cmd(f"oc apply -f {manifest.name}")
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 store those manifests file under cluster_path/manifests/manifest-tmpX.yaml e.g. .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dahorak how to get access to the relevant path?

] = f"{self.cluster_name}-ssh-private-key"
cluster_deployment["spec"]["provisioning"]["imageSetRef"][
"name"
] = "img4.14.3-multi-appsub"
Copy link
Member

Choose a reason for hiding this comment

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

From config value

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

Comment on lines 935 to 947
ocp_obj = OCP(kind=constants.ACM_MANAGEDCLUSTER)
ocp_obj.wait_for_resource(
timeout=10,
condition="True",
column="AVAILABLE",
resource_name=self.cluster_name,
)
ocp_obj.wait_for_resource(
timeout=10,
condition="True",
column="JOINED",
resource_name=self.cluster_name,
)
Copy link
Member

Choose a reason for hiding this comment

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

I suggest you to use the context manage (in python using with block) to make sure the context for this block of the code is running on the ACM hub.

In this context manager you need to do:

  1. Sore current config context - as you will need to return back to original config context after you finish this block of code
  2. Switch to ACM hub config object
  3. Do operations defined within with block
  4. Switch config context back to original stale - even though the code in the block will fail.
Suggested change
ocp_obj = OCP(kind=constants.ACM_MANAGEDCLUSTER)
ocp_obj.wait_for_resource(
timeout=10,
condition="True",
column="AVAILABLE",
resource_name=self.cluster_name,
)
ocp_obj.wait_for_resource(
timeout=10,
condition="True",
column="JOINED",
resource_name=self.cluster_name,
)
with acmHubContext():
ocp_obj = OCP(kind=constants.ACM_MANAGEDCLUSTER)
ocp_obj.wait_for_resource(
timeout=10,
condition="True",
column="AVAILABLE",
resource_name=self.cluster_name,
)
ocp_obj.wait_for_resource(
timeout=10,
condition="True",
column="JOINED",
resource_name=self.cluster_name,
)

Define context manager, see doc here: https://book.pythontips.com/en/latest/context_managers.html#implementing-a-context-manager-as-a-class

The same you will use the same in create cluster via acm as you need to run also from acm context.

I think we can have generic context manager like:

class RunWithConfigContext(object):
    def __init__(self, config_index):
        self.original_config_index = config.current_index
        self.config_index = config_index
    def __enter__(self):
        config.switch_context(self.config_index)
    def __exit__(self, type, value, traceback):
        config.switch_context(self.original_config_index)

than you can have the specific one which will reuse the above but will make sure it will find the acm_hub_config_index for you.

class RunWithAcmConfigContext(RunWithConfigContext):
    def __init__(self):
        config_index = findAcmContext() # you will need to make sure here to automatically get the index for acm hub cluster
         super(RunWithAcmConfigContext, self).__init__(config_index)

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 created one class RunWithConfigContext


class RunWithConfigContext(object):
    def __init__(self):
        self.original_config_index = config.cur_index
        self.acm_index = get_all_acm_indexes()[0]

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

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

what do you think?

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

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs.

@github-actions github-actions bot added the lifecycle/stale No recent activity label May 15, 2024
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

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 kubernetes-sigs/prow repository.

@github-actions github-actions bot removed the lifecycle/stale No recent activity label Jul 29, 2024
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs.

@github-actions github-actions bot added the lifecycle/stale No recent activity label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants