From 150ed6f613356d19bd3a4ab8ecdbb0cd5b976185 Mon Sep 17 00:00:00 2001 From: Bobbins228 Date: Wed, 26 Jul 2023 16:46:52 +0100 Subject: [PATCH 1/6] Added support for ingress over routes on cluster creation --- poetry.lock | 10 + src/codeflare_sdk/cluster/cluster.py | 103 +++---- src/codeflare_sdk/cluster/config.py | 2 + .../templates/base-template.yaml | 54 ++-- src/codeflare_sdk/utils/generate_yaml.py | 267 +++++++++++++++--- src/codeflare_sdk/utils/openshift_oauth.py | 4 +- tests/test-case-prio.yaml | 22 +- tests/test-case.yaml | 22 +- tests/unit_test.py | 194 ++++++++++--- tests/unit_test_support.py | 11 +- 10 files changed, 517 insertions(+), 172 deletions(-) diff --git a/poetry.lock b/poetry.lock index a02a42613..137227661 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1187,6 +1187,16 @@ files = [ {file = "MarkupSafe-2.1.3-cp311-cp311-musllinux_1_1_x86_64.whl", hash = "sha256:5bbe06f8eeafd38e5d0a4894ffec89378b6c6a625ff57e3028921f8ff59318ac"}, {file = "MarkupSafe-2.1.3-cp311-cp311-win32.whl", hash = "sha256:dd15ff04ffd7e05ffcb7fe79f1b98041b8ea30ae9234aed2a9168b5797c3effb"}, {file = "MarkupSafe-2.1.3-cp311-cp311-win_amd64.whl", hash = "sha256:134da1eca9ec0ae528110ccc9e48041e0828d79f24121a1a146161103c76e686"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-macosx_10_9_universal2.whl", hash = "sha256:f698de3fd0c4e6972b92290a45bd9b1536bffe8c6759c62471efaa8acb4c37bc"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:aa57bd9cf8ae831a362185ee444e15a93ecb2e344c8e52e4d721ea3ab6ef1823"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:ffcc3f7c66b5f5b7931a5aa68fc9cecc51e685ef90282f4a82f0f5e9b704ad11"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:47d4f1c5f80fc62fdd7777d0d40a2e9dda0a05883ab11374334f6c4de38adffd"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:1f67c7038d560d92149c060157d623c542173016c4babc0c1913cca0564b9939"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-musllinux_1_1_aarch64.whl", hash = "sha256:9aad3c1755095ce347e26488214ef77e0485a3c34a50c5a5e2471dff60b9dd9c"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-musllinux_1_1_i686.whl", hash = "sha256:14ff806850827afd6b07a5f32bd917fb7f45b046ba40c57abdb636674a8b559c"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:8f9293864fe09b8149f0cc42ce56e3f0e54de883a9de90cd427f191c346eb2e1"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-win32.whl", hash = "sha256:715d3562f79d540f251b99ebd6d8baa547118974341db04f5ad06d5ea3eb8007"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-win_amd64.whl", hash = "sha256:1b8dd8c3fd14349433c79fa8abeb573a55fc0fdd769133baac1f5e07abf54aeb"}, {file = "MarkupSafe-2.1.3-cp37-cp37m-macosx_10_9_x86_64.whl", hash = "sha256:8e254ae696c88d98da6555f5ace2279cf7cd5b3f52be2b5cf97feafe883b58d2"}, {file = "MarkupSafe-2.1.3-cp37-cp37m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:cb0932dc158471523c9637e807d9bfb93e06a95cbf010f1a38b98623b929ef2b"}, {file = "MarkupSafe-2.1.3-cp37-cp37m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:9402b03f1a1b4dc4c19845e5c749e3ab82d5078d16a2a4c2cd2df62d57bb0707"}, diff --git a/src/codeflare_sdk/cluster/cluster.py b/src/codeflare_sdk/cluster/cluster.py index 664752783..808aa0079 100644 --- a/src/codeflare_sdk/cluster/cluster.py +++ b/src/codeflare_sdk/cluster/cluster.py @@ -28,7 +28,9 @@ from .auth import config_check, api_config_handler from ..utils import pretty_print -from ..utils.generate_yaml import generate_appwrapper +from ..utils.generate_yaml import ( + generate_appwrapper, +) from ..utils.kube_api_helpers import _kube_api_error_handling from ..utils.openshift_oauth import ( create_openshift_oauth_objects, @@ -175,6 +177,8 @@ def create_app_wrapper(self): local_interactive = self.config.local_interactive image_pull_secrets = self.config.image_pull_secrets dispatch_priority = self.config.dispatch_priority + ingress_domain = self.config.ingress_domain + ingress_options = self.config.ingress_options return generate_appwrapper( name=name, namespace=namespace, @@ -198,6 +202,8 @@ def create_app_wrapper(self): dispatch_priority=dispatch_priority, priority_val=priority_val, openshift_oauth=self.config.openshift_oauth, + ingress_domain=ingress_domain, + ingress_options=ingress_options, ) # creates a new cluster with the provided or default spec @@ -399,27 +405,22 @@ def cluster_dashboard_uri(self) -> str: """ try: config_check() - api_instance = client.CustomObjectsApi(api_config_handler()) - routes = api_instance.list_namespaced_custom_object( - group="route.openshift.io", - version="v1", - namespace=self.config.namespace, - plural="routes", - ) - except Exception as e: # pragma: no cover + api_instance = client.NetworkingV1Api(api_config_handler()) + ingresses = api_instance.list_namespaced_ingress(self.config.namespace) + except Exception as e: return _kube_api_error_handling(e) - for route in routes["items"]: - if route["metadata"][ - "name" - ] == f"ray-dashboard-{self.config.name}" or route["metadata"][ - "name" - ].startswith( - f"{self.config.name}-ingress" - ): - protocol = "https" if route["spec"].get("tls") else "http" - return f"{protocol}://{route['spec']['host']}" + for ingress in ingresses.items: + annotations = ingress.metadata.annotations + if ingress.metadata.name == f"ray-dashboard-{self.config.name}" or ingress.metadata.name.startswith( + f"{self.config.name}-ingress" ): + if annotations == None: + protocol = "http" + elif "route.openshift.io/termination" in annotations: + protocol = "https" + return f"{protocol}://{ingress.spec.rules[0].host}" return "Dashboard route not available yet, have you run cluster.up()?" + def list_jobs(self) -> List: """ @@ -498,8 +499,8 @@ def from_k8_cluster_object(rc, mcad=True): def local_client_url(self): if self.config.local_interactive == True: - ingress_domain = _get_ingress_domain() - return f"ray://rayclient-{self.config.name}-{self.config.namespace}.{ingress_domain}" + ingress_domain = _get_ingress_domain(self) + return f"ray://{ingress_domain}" else: return "None" @@ -655,16 +656,23 @@ def _check_aw_exists(name: str, namespace: str) -> bool: return False -def _get_ingress_domain(): +# Cant test this until get_current_namespace is fixed +def _get_ingress_domain(self): # pragma: no cover try: config_check() - api_client = client.CustomObjectsApi(api_config_handler()) - ingress = api_client.get_cluster_custom_object( - "config.openshift.io", "v1", "ingresses", "cluster" - ) - except Exception as e: # pragma: no cover + api_client = client.NetworkingV1Api(api_config_handler()) + if self.config.namespace != None: + namespace = self.config.namespace + else: + namespace = get_current_namespace() + ingresses = api_client.list_namespaced_ingress(namespace) + except Exception as e: # pragma: no cover return _kube_api_error_handling(e) - return ingress["spec"]["domain"] + domain = None + for ingress in ingresses.items: + if ingress.spec.rules[0].http.paths[0].backend.service.port.number == 10001: + domain = ingress.spec.rules[0].host + return domain def _app_wrapper_status(name, namespace="default") -> Optional[AppWrapper]: @@ -756,27 +764,22 @@ def _map_to_ray_cluster(rc) -> Optional[RayCluster]: status = RayClusterStatus(rc["status"]["state"].lower()) else: status = RayClusterStatus.UNKNOWN - - config_check() - api_instance = client.CustomObjectsApi(api_config_handler()) - # UPDATE THIS - routes = api_instance.list_namespaced_custom_object( - group="route.openshift.io", - version="v1", - namespace=rc["metadata"]["namespace"], - plural="routes", - ) - ray_route = None - for route in routes["items"]: - if route["metadata"][ - "name" - ] == f"ray-dashboard-{rc['metadata']['name']}" or route["metadata"][ - "name" - ].startswith( - f"{rc['metadata']['name']}-ingress" - ): - protocol = "https" if route["spec"].get("tls") else "http" - ray_route = f"{protocol}://{route['spec']['host']}" + try: + config_check() + api_instance = client.NetworkingV1Api(api_config_handler()) + ingresses = api_instance.list_namespaced_ingress(rc["metadata"]["namespace"]) + except Exception as e: + return _kube_api_error_handling(e) + ray_ingress = None + for ingress in ingresses.items: + annotations = ingress.metadata.annotations + if ingress.metadata.name == f"ray-dashboard-{rc['metadata']['name']}" or ingress.metadata.name.startswith( + f"{rc['metadata']['name']}-ingress" ): + if annotations == None: + protocol = "http" + elif "route.openshift.io/termination" in annotations: + protocol = "https" + ray_ingress = f"{protocol}://{ingress.spec.rules[0].host}" return RayCluster( name=rc["metadata"]["name"], @@ -794,7 +797,6 @@ def _map_to_ray_cluster(rc) -> Optional[RayCluster]: ]["resources"]["limits"]["cpu"], worker_gpu=0, # hard to detect currently how many gpus, can override it with what the user asked for namespace=rc["metadata"]["namespace"], - dashboard=ray_route, head_cpus=rc["spec"]["headGroupSpec"]["template"]["spec"]["containers"][0][ "resources" ]["limits"]["cpu"], @@ -804,6 +806,7 @@ def _map_to_ray_cluster(rc) -> Optional[RayCluster]: head_gpu=rc["spec"]["headGroupSpec"]["template"]["spec"]["containers"][0][ "resources" ]["limits"]["nvidia.com/gpu"], + dashboard=ray_ingress, ) diff --git a/src/codeflare_sdk/cluster/config.py b/src/codeflare_sdk/cluster/config.py index a21318abc..192097646 100644 --- a/src/codeflare_sdk/cluster/config.py +++ b/src/codeflare_sdk/cluster/config.py @@ -53,3 +53,5 @@ class ClusterConfiguration: image_pull_secrets: list = field(default_factory=list) dispatch_priority: str = None openshift_oauth: bool = False # NOTE: to use the user must have permission to create a RoleBinding for system:auth-delegator + ingress_options: dict = field(default_factory=dict) + ingress_domain: str = None diff --git a/src/codeflare_sdk/templates/base-template.yaml b/src/codeflare_sdk/templates/base-template.yaml index cf4ec4966..8f309630d 100644 --- a/src/codeflare_sdk/templates/base-template.yaml +++ b/src/codeflare_sdk/templates/base-template.yaml @@ -289,38 +289,50 @@ spec: emptyDir: {} - replicas: 1 generictemplate: - kind: Route - apiVersion: route.openshift.io/v1 + apiVersion: networking.k8s.io/v1 + kind: Ingress metadata: - name: ray-dashboard-deployment-name + name: ray-dashboard-raytest namespace: default - labels: - # allows me to return name of service that Ray operator creates - odh-ray-cluster-service: deployment-name-head-svc + annotations: + annotations-example:annotations-example spec: - to: - kind: Service - name: deployment-name-head-svc - port: - targetPort: dashboard + ingressClassName: nginx + rules: + - http: + paths: + - backend: + service: + name: raytest-head-svc + port: + number: 8265 + pathType: Prefix + path: / + host: ray-dashboard-raytest. - replicas: 1 generictemplate: - apiVersion: route.openshift.io/v1 - kind: Route + apiVersion: networking.k8s.io/v1 + kind: Ingress metadata: name: rayclient-deployment-name namespace: default + annotations: + annotations-example:annotations-example labels: - # allows me to return name of service that Ray operator creates odh-ray-cluster-service: deployment-name-head-svc spec: - port: - targetPort: client - tls: - termination: passthrough - to: - kind: Service - name: deployment-name-head-svc + ingressClassName: nginx + rules: + - http: + paths: + - backend: + service: + name: deployment-name-head-svc + port: + number: 10001 + path: '' + pathType: ImplementationSpecific + host: rayclient-raytest. - replicas: 1 generictemplate: apiVersion: v1 diff --git a/src/codeflare_sdk/utils/generate_yaml.py b/src/codeflare_sdk/utils/generate_yaml.py index 4e4d44921..e5ca4bd6a 100755 --- a/src/codeflare_sdk/utils/generate_yaml.py +++ b/src/codeflare_sdk/utils/generate_yaml.py @@ -50,28 +50,191 @@ def gen_names(name): else: return name, name - -def update_dashboard_route(route_item, cluster_name, namespace): - metadata = route_item.get("generictemplate", {}).get("metadata") - metadata["name"] = gen_dashboard_route_name(cluster_name) - metadata["namespace"] = namespace - metadata["labels"]["odh-ray-cluster-service"] = f"{cluster_name}-head-svc" - spec = route_item.get("generictemplate", {}).get("spec") - spec["to"]["name"] = f"{cluster_name}-head-svc" - - def gen_dashboard_route_name(cluster_name): return f"ray-dashboard-{cluster_name}" +# Check if the ingress api cluster resource exists +def is_openshift_cluster(): + try: + config_check() + api_instance = client.CustomObjectsApi(api_config_handler()) + api_instance.get_cluster_custom_object( + "config.openshift.io", "v1", "ingresses", "cluster" + ) -# ToDo: refactor the update_x_route() functions -def update_rayclient_route(route_item, cluster_name, namespace): - metadata = route_item.get("generictemplate", {}).get("metadata") - metadata["name"] = f"rayclient-{cluster_name}" - metadata["namespace"] = namespace - metadata["labels"]["odh-ray-cluster-service"] = f"{cluster_name}-head-svc" - spec = route_item.get("generictemplate", {}).get("spec") - spec["to"]["name"] = f"{cluster_name}-head-svc" + return True + except client.ApiException as e: # pragma: no cover + if e.status == 404 or e.status == 403: + return False + else: + print(f"Error detecting cluster type defaulting to Kubernetes: {e}") + return False + + +def update_dashboard_ingress( + ingress_item, cluster_name, namespace, ingress_options, ingress_domain +): # pragma: no cover + metadata = ingress_item.get("generictemplate", {}).get("metadata") + spec = ingress_item.get("generictemplate", {}).get("spec") + if ingress_options != {}: + for index, ingress_option in enumerate(ingress_options["ingresses"]): + if "ingressName" not in ingress_option.keys(): + raise ValueError( + f"Error: 'ingressName' is missing or empty for ingress item at index {index}" + ) + if "port" not in ingress_option.keys(): + raise ValueError( + f"Error: 'port' is missing or empty for ingress item at index {index}" + ) + elif not isinstance(ingress_option["port"], int): + raise ValueError( + f"Error: 'port' is not of type int for ingress item at index {index}" + ) + if ingress_option["port"] == 8265: + metadata["name"] = ingress_option["ingressName"] + metadata["namespace"] = namespace + if "annotations" not in ingress_option.keys(): + del metadata["annotations"] + else: + metadata["annotations"] = ingress_option["annotations"] + if "path" not in ingress_option.keys(): + del spec["rules"][0]["http"]["paths"][0]["path"] + else: + spec["rules"][0]["http"]["paths"][0]["path"] = ingress_option[ + "path" + ] + if "pathType" not in ingress_option.keys(): + spec["rules"][0]["http"]["paths"][0][ + "pathType" + ] = "ImplementationSpecific" + if "host" not in ingress_option.keys(): + del spec["rules"][0]["host"] + else: + spec["rules"][0]["host"] = ingress_option["host"] + if "ingressClassName" not in ingress_option.keys(): + del spec["ingressClassName"] + else: + spec["ingressClassName"] = ingress_option["ingressClassName"] + + spec["rules"][0]["http"]["paths"][0]["backend"]["service"][ + "name" + ] = f"{cluster_name}-head-svc" + else: + metadata["name"] = f"ray-dashboard-{cluster_name}" + metadata["namespace"] = namespace + spec["rules"][0]["http"]["paths"][0]["backend"]["service"][ + "name" + ] = f"{cluster_name}-head-svc" + if is_openshift_cluster(): + try: + config_check() + api_client = client.CustomObjectsApi(api_config_handler()) + ingress = api_client.get_cluster_custom_object( + "config.openshift.io", "v1", "ingresses", "cluster" + ) + del spec["ingressClassName"] + except Exception as e: # pragma: no cover + return _kube_api_error_handling(e) + domain = ingress["spec"]["domain"] + elif ingress_domain is None: + raise ValueError( + "ingress_domain is invalid. For Kubernetes Clusters please specify an ingress domain" + ) + else: + domain = ingress_domain + del metadata["annotations"] + spec["rules"][0]["host"] = f"ray-dashboard-{cluster_name}-{namespace}.{domain}" + + +def update_rayclient_ingress( + ingress_item, cluster_name, namespace, ingress_options, ingress_domain +): # pragma: no cover + metadata = ingress_item.get("generictemplate", {}).get("metadata") + spec = ingress_item.get("generictemplate", {}).get("spec") + if ingress_options != {}: + for index, ingress_option in enumerate(ingress_options["ingresses"]): + if "ingressName" not in ingress_option.keys(): + raise ValueError( + f"Error: 'ingressName' is missing or empty for ingress item at index {index}" + ) + if "port" not in ingress_option.keys(): + raise ValueError( + f"Error: 'port' is missing or empty for ingress item at index {index}" + ) + elif not isinstance(ingress_option["port"], int): + raise ValueError( + f"Error: 'port' is not of type int for ingress item at index {index}" + ) + if ingress_option["port"] == 10001: + metadata["name"] = ingress_option["ingressName"] + metadata["namespace"] = namespace + if "annotations" not in ingress_option.keys(): + del metadata["annotations"] + else: + metadata["annotations"] = ingress_option["annotations"] + if "path" not in ingress_option.keys(): + del spec["rules"][0]["http"]["paths"][0]["path"] + else: + spec["rules"][0]["http"]["paths"][0]["path"] = ingress_option[ + "path" + ] + if "pathType" not in ingress_option.keys(): + spec["rules"][0]["http"]["paths"][0][ + "pathType" + ] = "ImplementationSpecific" + if "host" not in ingress_option.keys(): + del spec["rules"][0]["host"] + else: + spec["rules"][0]["host"] = ingress_option["host"] + if "ingressClassName" not in ingress_option.keys(): + del spec["ingressClassName"] + else: + spec["ingressClassName"] = ingress_option["ingressClassName"] + + spec["rules"][0]["http"]["paths"][0]["backend"]["service"][ + "name" + ] = f"{cluster_name}-head-svc" + else: + metadata["name"] = f"rayclient-{cluster_name}" + metadata["namespace"] = namespace + metadata["labels"]["odh-ray-cluster-service"] = f"{cluster_name}-head-svc" + + spec["rules"][0]["http"]["paths"][0]["backend"]["service"][ + "name" + ] = f"{cluster_name}-head-svc" + + if is_openshift_cluster(): + try: + config_check() + api_client = client.CustomObjectsApi(api_config_handler()) + ingress = api_client.get_cluster_custom_object( + "config.openshift.io", "v1", "ingresses", "cluster" + ) + ingressClassName = "openshift-default" + annotations = { + "nginx.ingress.kubernetes.io/rewrite-target": "/", + "nginx.ingress.kubernetes.io/ssl-redirect": "true", + "route.openshift.io/termination": "passthrough", + } + except Exception as e: # pragma: no cover + return _kube_api_error_handling(e) + domain = ingress["spec"]["domain"] + elif ingress_domain is None: + raise ValueError( + "ingress_domain is invalid. For Kubernetes Clusters please specify an ingress domain" + ) + else: + domain = ingress_domain + ingressClassName = "nginx" + annotations = { + "nginx.ingress.kubernetes.io/rewrite-target": "/", + "nginx.ingress.kubernetes.io/ssl-redirect": "true", + "nginx.ingress.kubernetes.io/ssl-passthrough": "true", + } + + metadata["annotations"] = annotations + spec["ingressClassName"] = ingressClassName + spec["rules"][0]["host"] = f"rayclient-{cluster_name}-{namespace}.{domain}" def update_names(yaml, item, appwrapper_name, cluster_name, namespace): @@ -274,11 +437,12 @@ def update_ca_secret(ca_secret_item, cluster_name, namespace): data["ca.key"], data["ca.crt"] = generate_cert.generate_ca_cert(365) -def enable_local_interactive(resources, cluster_name, namespace): - rayclient_route_item = resources["resources"].get("GenericItems")[2] +def enable_local_interactive( + resources, cluster_name, namespace, ingress_options, ingress_domain +): + rayclient_ingress_item = resources["resources"].get("GenericItems")[2] ca_secret_item = resources["resources"].get("GenericItems")[3] item = resources["resources"].get("GenericItems")[0] - update_rayclient_route(rayclient_route_item, cluster_name, namespace) update_ca_secret(ca_secret_item, cluster_name, namespace) # update_ca_secret_volumes item["generictemplate"]["spec"]["headGroupSpec"]["template"]["spec"]["volumes"][0][ @@ -300,16 +464,47 @@ def enable_local_interactive(resources, cluster_name, namespace): ][0].get("command")[2] command = command.replace("deployment-name", cluster_name) - try: - config_check() - api_client = client.CustomObjectsApi(api_config_handler()) - ingress = api_client.get_cluster_custom_object( - "config.openshift.io", "v1", "ingresses", "cluster" - ) - except Exception as e: # pragma: no cover - return _kube_api_error_handling(e) - domain = ingress["spec"]["domain"] + + if ingress_options != {}: + for index, ingress_option in enumerate(ingress_options["ingresses"]): + if ingress_option["port"] == 10001: + if "host" not in ingress_option.keys(): + raise ValueError( + f"Client host is not specified please include a host for the ingress item at index {index}" + ) + else: + host = ingress_option["host"] + domain_split = host.split(".", 1) + if len(domain_split) > 1: + domain = domain_split[1] + else: + raise ValueError( + f"The client ingress host is configured incorrectly please specify a host with a correct domain for the ingress item at index {index}" + ) + + else: + if is_openshift_cluster(): + # We can try get the domain through checking ingresses.config.openshift.io + try: + config_check() + api_client = client.CustomObjectsApi(api_config_handler()) + ingress = api_client.get_cluster_custom_object( + "config.openshift.io", "v1", "ingresses", "cluster" + ) + except Exception as e: # pragma: no cover + return _kube_api_error_handling(e) + domain = ingress["spec"]["domain"] + elif ingress_domain is None: + raise ValueError( + "ingress_domain is invalid. For Kubernetes Clusters please specify an ingress domain" + ) + else: + domain = ingress_domain + command = command.replace("server-name", domain) + update_rayclient_ingress( + rayclient_ingress_item, cluster_name, namespace, ingress_options, domain + ) item["generictemplate"]["spec"]["headGroupSpec"]["template"]["spec"][ "initContainers" @@ -493,12 +688,14 @@ def generate_appwrapper( dispatch_priority: str, priority_val: int, openshift_oauth: bool, + ingress_domain: str, + ingress_options: dict, ): user_yaml = read_template(template) appwrapper_name, cluster_name = gen_names(name) resources = user_yaml.get("spec", "resources") item = resources["resources"].get("GenericItems")[0] - route_item = resources["resources"].get("GenericItems")[1] + ingress_item = resources["resources"].get("GenericItems")[1] update_names(user_yaml, item, appwrapper_name, cluster_name, namespace) update_labels(user_yaml, instascale, instance_types) update_priority(user_yaml, item, dispatch_priority, priority_val) @@ -531,9 +728,13 @@ def generate_appwrapper( head_memory, head_gpus, ) - update_dashboard_route(route_item, cluster_name, namespace) + update_dashboard_ingress( + ingress_item, cluster_name, namespace, ingress_options, ingress_domain + ) if local_interactive: - enable_local_interactive(resources, cluster_name, namespace) + enable_local_interactive( + resources, cluster_name, namespace, ingress_options, ingress_domain + ) else: disable_raycluster_tls(resources["resources"]) diff --git a/src/codeflare_sdk/utils/openshift_oauth.py b/src/codeflare_sdk/utils/openshift_oauth.py index 5c3fc55aa..022e9adbd 100644 --- a/src/codeflare_sdk/utils/openshift_oauth.py +++ b/src/codeflare_sdk/utils/openshift_oauth.py @@ -1,5 +1,5 @@ from urllib3.util import parse_url -from .generate_yaml import gen_dashboard_route_name +from .generate_yaml import gen_dashboard_ingress_name from .kube_api_helpers import _get_api_host from base64 import b64decode @@ -19,7 +19,7 @@ def create_openshift_oauth_objects(cluster_name, namespace): host = _get_api_host(api_client) # replace "^api" with the expected host - host = f"{gen_dashboard_route_name(cluster_name)}-{namespace}.apps" + host.lstrip( + host = f"{gen_dashboard_ingress_name(cluster_name)}-{namespace}.apps" + host.lstrip( "api" ) diff --git a/tests/test-case-prio.yaml b/tests/test-case-prio.yaml index 36cc3a408..9278e2c04 100644 --- a/tests/test-case-prio.yaml +++ b/tests/test-case-prio.yaml @@ -180,18 +180,22 @@ spec: priorityClassName: default replicas: 1 - generictemplate: - apiVersion: route.openshift.io/v1 - kind: Route + apiVersion: networking.k8s.io/v1 + kind: Ingress metadata: - labels: - odh-ray-cluster-service: prio-test-cluster-head-svc name: ray-dashboard-prio-test-cluster namespace: ns spec: - port: - targetPort: dashboard - to: - kind: Service - name: prio-test-cluster-head-svc + rules: + - host: ray-dashboard-prio-test-cluster-ns.apps.cluster.awsroute.org + http: + paths: + - backend: + service: + name: prio-test-cluster-head-svc + port: + number: 8265 + path: / + pathType: Prefix replicas: 1 Items: [] diff --git a/tests/test-case.yaml b/tests/test-case.yaml index 8ab27f018..d6701eb4a 100644 --- a/tests/test-case.yaml +++ b/tests/test-case.yaml @@ -177,18 +177,22 @@ spec: name: init-myservice replicas: 1 - generictemplate: - apiVersion: route.openshift.io/v1 - kind: Route + apiVersion: networking.k8s.io/v1 + kind: Ingress metadata: - labels: - odh-ray-cluster-service: unit-test-cluster-head-svc name: ray-dashboard-unit-test-cluster namespace: ns spec: - port: - targetPort: dashboard - to: - kind: Service - name: unit-test-cluster-head-svc + rules: + - host: ray-dashboard-unit-test-cluster-ns.apps.cluster.awsroute.org + http: + paths: + - backend: + service: + name: unit-test-cluster-head-svc + port: + number: 8265 + path: / + pathType: Prefix replicas: 1 Items: [] diff --git a/tests/unit_test.py b/tests/unit_test.py index 8900df5b3..0b96fe185 100644 --- a/tests/unit_test.py +++ b/tests/unit_test.py @@ -19,6 +19,7 @@ import filecmp import os import re +import uuid parent = Path(__file__).resolve().parents[1] sys.path.append(str(parent) + "/src") @@ -77,6 +78,7 @@ ) import codeflare_sdk.utils.kube_api_helpers +from codeflare_sdk.utils.generate_yaml import gen_names, is_openshift_cluster import openshift from openshift.selector import Selector @@ -247,8 +249,8 @@ def test_config_creation(): assert config.local_interactive == False -def test_cluster_creation(): - cluster = createClusterWithConfig() +def test_cluster_creation(mocker): + cluster = createClusterWithConfig(mocker) assert cluster.app_wrapper_yaml == "unit-test-cluster.yaml" assert cluster.app_wrapper_name == "unit-test-cluster" assert filecmp.cmp( @@ -279,6 +281,10 @@ def test_cluster_creation_priority(mocker): config = createClusterConfig() config.name = "prio-test-cluster" config.dispatch_priority = "default" + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": "apps.cluster.awsroute.org"}}, + ) cluster = Cluster(config) assert cluster.app_wrapper_yaml == "prio-test-cluster.yaml" assert cluster.app_wrapper_name == "prio-test-cluster" @@ -292,6 +298,10 @@ def test_default_cluster_creation(mocker): "codeflare_sdk.cluster.cluster.get_current_namespace", return_value="opendatahub", ) + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": ""}}, + ) default_config = ClusterConfiguration( name="unit-test-default-cluster", ) @@ -302,6 +312,25 @@ def test_default_cluster_creation(mocker): assert cluster.config.namespace == "opendatahub" +def test_gen_names_with_name(mocker): + mocker.patch.object( + uuid, "uuid4", return_value=uuid.UUID("00000000-0000-0000-0000-000000000001") + ) + name = "myname" + appwrapper_name, cluster_name = gen_names(name) + assert appwrapper_name == name + assert cluster_name == name + + +def test_gen_names_without_name(mocker): + mocker.patch.object( + uuid, "uuid4", return_value=uuid.UUID("00000000-0000-0000-0000-000000000001") + ) + appwrapper_name, cluster_name = gen_names(None) + assert appwrapper_name.startswith("appwrapper-") + assert cluster_name.startswith("cluster-") + + def arg_check_apply_effect(group, version, namespace, plural, body, *args): assert namespace == "ns" assert args == tuple() @@ -350,6 +379,10 @@ def arg_check_del_effect(group, version, namespace, plural, name, *args): def test_cluster_up_down(mocker): mocker.patch("kubernetes.config.load_kube_config", return_value="ignore") + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": ""}}, + ) mocker.patch( "kubernetes.client.CustomObjectsApi.create_namespaced_custom_object", side_effect=arg_check_apply_effect, @@ -362,7 +395,7 @@ def test_cluster_up_down(mocker): "kubernetes.client.CustomObjectsApi.list_cluster_custom_object", return_value={"items": []}, ) - cluster = cluster = createClusterWithConfig() + cluster = cluster = createClusterWithConfig(mocker) cluster.up() cluster.down() @@ -446,41 +479,29 @@ def test_rc_status(mocker): assert rc == None -def uri_retreival(group, version, namespace, plural, *args): - assert group == "route.openshift.io" - assert version == "v1" - assert namespace == "ns" - assert plural == "routes" - assert args == tuple() - return { - "items": [ - { - "metadata": {"name": "ray-dashboard-unit-test-cluster"}, - "spec": { - "host": "ray-dashboard-unit-test-cluster-ns.apps.cluster.awsroute.org" - }, - } - ] - } - - def test_cluster_uris(mocker): mocker.patch("kubernetes.config.load_kube_config", return_value="ignore") mocker.patch( - "kubernetes.client.CustomObjectsApi.list_namespaced_custom_object", - side_effect=uri_retreival, + "codeflare_sdk.cluster.cluster._get_ingress_domain", + return_value="apps.cluster.awsroute.org", + ) + cluster = cluster = createClusterWithConfig(mocker) + mocker.patch( + "kubernetes.client.NetworkingV1Api.list_namespaced_ingress", + return_value=ingress_retrieval(port=8265), ) - - cluster = cluster = createClusterWithConfig() assert cluster.cluster_uri() == "ray://unit-test-cluster-head-svc.ns.svc:10001" assert ( cluster.cluster_dashboard_uri() == "http://ray-dashboard-unit-test-cluster-ns.apps.cluster.awsroute.org" ) cluster.config.name = "fake" + mocker.patch( + "kubernetes.client.NetworkingV1Api.list_namespaced_ingress", + ) assert ( cluster.cluster_dashboard_uri() - == "Dashboard route not available yet, have you run cluster.up()?" + == "Dashboard ingress not available yet, have you run cluster.up()?" ) @@ -491,7 +512,7 @@ def test_local_client_url(mocker): ) mocker.patch( "codeflare_sdk.cluster.cluster._get_ingress_domain", - return_value="apps.cluster.awsroute.org", + return_value="rayclient-unit-test-cluster-localinter-ns.apps.cluster.awsroute.org", ) mocker.patch( "codeflare_sdk.cluster.cluster.Cluster.create_app_wrapper", @@ -512,14 +533,41 @@ def ray_addr(self, *args): return self._address -def test_ray_job_wrapping(mocker): - mocker.patch("kubernetes.config.load_kube_config", return_value="ignore") - mocker.patch( - "kubernetes.client.CustomObjectsApi.list_namespaced_custom_object", - side_effect=uri_retreival, - ) - cluster = cluster = createClusterWithConfig() +def ingress_retrieval(port): + if port == 10001: + serviceName = "client" + else: + serviceName = "dashboard" + mock_ingress = client.V1Ingress( + metadata=client.V1ObjectMeta(name=f"ray-{serviceName}-unit-test-cluster"), + spec=client.V1IngressSpec( + rules=[ + client.V1IngressRule( + host=f"ray-{serviceName}-unit-test-cluster-ns.apps.cluster.awsroute.org", + http=client.V1HTTPIngressRuleValue( + paths=[ + client.V1HTTPIngressPath( + path_type="Prefix", + path="/", + backend=client.V1IngressBackend( + service=client.V1IngressServiceBackend( + name="head-svc-test", + port=client.V1ServiceBackendPort(number=port), + ) + ), + ) + ] + ), + ) + ], + ), + ) + mock_ingress_list = client.V1IngressList(items=[mock_ingress]) + return mock_ingress_list + +def test_ray_job_wrapping(mocker): + cluster = cluster = createClusterWithConfig(mocker) mocker.patch( "ray.job_submission.JobSubmissionClient._check_connection_and_version_with_url", return_value="None", @@ -528,6 +576,14 @@ def test_ray_job_wrapping(mocker): ray.job_submission.JobSubmissionClient, "list_jobs", autospec=True ) mock_res.side_effect = ray_addr + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": ""}}, + ) + mocker.patch( + "kubernetes.client.NetworkingV1Api.list_namespaced_ingress", + return_value=ingress_retrieval(8265), + ) assert cluster.list_jobs() == cluster.cluster_dashboard_uri() mock_res = mocker.patch.object( @@ -604,6 +660,10 @@ def test_print_appwrappers(capsys): def test_ray_details(mocker, capsys): + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": ""}}, + ) ray1 = RayCluster( name="raytest1", status=RayClusterStatus.READY, @@ -1664,6 +1724,10 @@ def get_aw_obj(group, version, namespace, plural): def test_get_cluster(mocker): mocker.patch("kubernetes.config.load_kube_config", return_value="ignore") + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": ""}}, + ) mocker.patch( "kubernetes.client.CustomObjectsApi.list_namespaced_custom_object", side_effect=get_ray_obj, @@ -1692,6 +1756,9 @@ def test_list_clusters(mocker, capsys): "kubernetes.client.CustomObjectsApi.list_namespaced_custom_object", side_effect=get_obj_none, ) + mocker.patch( + "kubernetes.client.NetworkingV1Api.list_namespaced_ingress", + ) list_all_clusters("ns") captured = capsys.readouterr() assert captured.out == ( @@ -1764,6 +1831,10 @@ def test_list_queue(mocker, capsys): def test_cluster_status(mocker): mocker.patch("kubernetes.config.load_kube_config", return_value="ignore") + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": ""}}, + ) fake_aw = AppWrapper( "test", AppWrapperStatus.FAILED, can_run=True, job_state="unused" ) @@ -1845,6 +1916,14 @@ def test_cluster_status(mocker): def test_wait_ready(mocker, capsys): + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": ""}}, + ) + mocker.patch( + "kubernetes.client.NetworkingV1Api.list_namespaced_ingress", + return_value=ingress_retrieval(8265), + ) mocker.patch("kubernetes.config.load_kube_config", return_value="ignore") mocker.patch("codeflare_sdk.cluster.cluster._app_wrapper_status", return_value=None) mocker.patch("codeflare_sdk.cluster.cluster._ray_cluster_status", return_value=None) @@ -1893,9 +1972,13 @@ def test_wait_ready(mocker, capsys): ) -def test_jobdefinition_coverage(): +def test_jobdefinition_coverage(mocker): + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": ""}}, + ) abstract = JobDefinition() - cluster = createClusterWithConfig() + cluster = createClusterWithConfig(mocker) abstract._dry_run(cluster) abstract.submit(cluster) @@ -1937,8 +2020,8 @@ def test_DDPJobDefinition_dry_run(mocker: MockerFixture): ) mocker.patch.object(Cluster, "job_client") ddp = createTestDDP() - cluster = createClusterWithConfig() - ddp_job, _ = ddp._dry_run(cluster) + cluster = createClusterWithConfig(mocker) + ddp_job, _ = ddp._dry_run(mocker, cluster) assert type(ddp_job) == AppDryRunInfo assert ddp_job._fmt is not None assert type(ddp_job.request) == RayJob @@ -2006,11 +2089,15 @@ def test_DDPJobDefinition_dry_run_no_resource_args(mocker): when the job definition does not specify resources. """ mocker.patch.object(Cluster, "job_client") + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": ""}}, + ) mocker.patch( "codeflare_sdk.cluster.cluster.Cluster.cluster_dashboard_uri", return_value="", ) - cluster = createClusterWithConfig() + cluster = createClusterWithConfig(mocker) ddp = DDPJobDefinition( script="test.py", m=None, @@ -2099,7 +2186,7 @@ def test_DDPJobDefinition_submit(mocker: MockerFixture): mock_schedule.return_value = "fake-dashboard-url" mocker.patch.object(Cluster, "job_client") ddp_def = createTestDDP() - cluster = createClusterWithConfig() + cluster = createClusterWithConfig(mocker) mocker.patch( "codeflare_sdk.job.jobs.get_current_namespace", side_effect="opendatahub", @@ -2131,9 +2218,9 @@ def test_DDPJob_creation(mocker: MockerFixture): Cluster, "cluster_dashboard_uri", return_value="fake-dashboard-url" ) ddp_def = createTestDDP() - cluster = createClusterWithConfig() + cluster = createClusterWithConfig(mocker) mock_schedule.return_value = "fake-dashboard-url" - ddp_job = createDDPJob_with_cluster(ddp_def, cluster) + ddp_job = createDDPJob_with_cluster(mocker, ddp_def, cluster) assert type(ddp_job) == DDPJob assert type(ddp_job.job_definition) == DDPJobDefinition assert type(ddp_job.cluster) == Cluster @@ -2179,8 +2266,8 @@ def test_DDPJob_status(mocker: MockerFixture): mocker.patch.object(Runner, "status", mock_status) test_DDPJob_creation(mocker) ddp_def = createTestDDP() - cluster = createClusterWithConfig() - ddp_job = createDDPJob_with_cluster(ddp_def, cluster) + cluster = createClusterWithConfig(mocker) + ddp_job = createDDPJob_with_cluster(mocker, ddp_def, cluster) mock_status.return_value = "fake-status" assert ddp_job.status() == "fake-status" _, args, kwargs = mock_status.mock_calls[0] @@ -2193,8 +2280,8 @@ def test_DDPJob_logs(mocker: MockerFixture): # Setup the neccesary mock patches test_DDPJob_creation(mocker) ddp_def = createTestDDP() - cluster = createClusterWithConfig() - ddp_job = createDDPJob_with_cluster(ddp_def, cluster) + cluster = createClusterWithConfig(mocker) + ddp_job = createDDPJob_with_cluster(mocker, ddp_def, cluster) mock_log.return_value = "fake-logs" assert ddp_job.logs() == "fake-logs" _, args, kwargs = mock_log.mock_calls[0] @@ -2337,6 +2424,21 @@ def secret_ca_retreival(secret_name, namespace): return client.models.V1Secret(data=data) +def test_is_openshift_cluster(mocker): + mocker.patch("kubernetes.config.load_kube_config", return_value="ignore") + mocker.patch.object( + client.CustomObjectsApi, + "get_cluster_custom_object", + side_effect=client.ApiException(status=404), + ) + assert is_openshift_cluster() == False + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": ""}}, + ) + assert is_openshift_cluster() == True + + def test_generate_tls_cert(mocker): """ test the function codeflare_sdk.utils.generate_ca_cert generates the correct outputs diff --git a/tests/unit_test_support.py b/tests/unit_test_support.py index a4ea056a0..fea4ceafd 100644 --- a/tests/unit_test_support.py +++ b/tests/unit_test_support.py @@ -46,14 +46,21 @@ def createClusterConfig(): instascale=True, machine_types=["cpu.small", "gpu.large"], image_pull_secrets=["unit-test-pull-secret"], + ingress_domain="apps.cluster.awsroute.org", ) return config -def createClusterWithConfig(): +def createClusterWithConfig(mocker): + mocker.patch("kubernetes.config.load_kube_config", return_value="ignore") + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": "apps.cluster.awsroute.org"}}, + ) cluster = Cluster(createClusterConfig()) return cluster -def createDDPJob_with_cluster(ddp_def, cluster=createClusterWithConfig()): +def createDDPJob_with_cluster(mocker, ddp_def, cluster=None): + cluster = createClusterWithConfig(mocker) return DDPJob(ddp_def, cluster) From 25ce6cb55013ffb6591bfc5adf2d063e7ec78f62 Mon Sep 17 00:00:00 2001 From: Bobbins228 Date: Mon, 23 Oct 2023 18:37:24 +0100 Subject: [PATCH 2/6] Fixed unit tests and ingress related methods --- src/codeflare_sdk/cluster/cluster.py | 21 +++++++++++++-------- src/codeflare_sdk/utils/generate_yaml.py | 6 ++++-- tests/test-case-no-mcad.yamls | 22 +++++++++++++--------- tests/unit_test.py | 22 ++++++++++++++++++---- 4 files changed, 48 insertions(+), 23 deletions(-) diff --git a/src/codeflare_sdk/cluster/cluster.py b/src/codeflare_sdk/cluster/cluster.py index 808aa0079..4ddfcfd37 100644 --- a/src/codeflare_sdk/cluster/cluster.py +++ b/src/codeflare_sdk/cluster/cluster.py @@ -412,15 +412,17 @@ def cluster_dashboard_uri(self) -> str: for ingress in ingresses.items: annotations = ingress.metadata.annotations - if ingress.metadata.name == f"ray-dashboard-{self.config.name}" or ingress.metadata.name.startswith( - f"{self.config.name}-ingress" ): + protocol = "http" + if ( + ingress.metadata.name == f"ray-dashboard-{self.config.name}" + or ingress.metadata.name.startswith(f"{self.config.name}-ingress") + ): if annotations == None: protocol = "http" elif "route.openshift.io/termination" in annotations: protocol = "https" return f"{protocol}://{ingress.spec.rules[0].host}" - return "Dashboard route not available yet, have you run cluster.up()?" - + return "Dashboard ingress not available yet, have you run cluster.up()?" def list_jobs(self) -> List: """ @@ -665,8 +667,8 @@ def _get_ingress_domain(self): # pragma: no cover namespace = self.config.namespace else: namespace = get_current_namespace() - ingresses = api_client.list_namespaced_ingress(namespace) - except Exception as e: # pragma: no cover + ingresses = api_client.list_namespaced_ingress(namespace) + except Exception as e: # pragma: no cover return _kube_api_error_handling(e) domain = None for ingress in ingresses.items: @@ -773,8 +775,11 @@ def _map_to_ray_cluster(rc) -> Optional[RayCluster]: ray_ingress = None for ingress in ingresses.items: annotations = ingress.metadata.annotations - if ingress.metadata.name == f"ray-dashboard-{rc['metadata']['name']}" or ingress.metadata.name.startswith( - f"{rc['metadata']['name']}-ingress" ): + protocol = "http" + if ( + ingress.metadata.name == f"ray-dashboard-{rc['metadata']['name']}" + or ingress.metadata.name.startswith(f"{rc['metadata']['name']}-ingress") + ): if annotations == None: protocol = "http" elif "route.openshift.io/termination" in annotations: diff --git a/src/codeflare_sdk/utils/generate_yaml.py b/src/codeflare_sdk/utils/generate_yaml.py index e5ca4bd6a..3cf886cf6 100755 --- a/src/codeflare_sdk/utils/generate_yaml.py +++ b/src/codeflare_sdk/utils/generate_yaml.py @@ -50,9 +50,11 @@ def gen_names(name): else: return name, name -def gen_dashboard_route_name(cluster_name): + +def gen_dashboard_ingress_name(cluster_name): return f"ray-dashboard-{cluster_name}" + # Check if the ingress api cluster resource exists def is_openshift_cluster(): try: @@ -586,7 +588,7 @@ def enable_openshift_oauth(user_yaml, cluster_name, namespace): port_name = "oauth-proxy" host = _get_api_host(k8_client) host = host.replace( - "api.", f"{gen_dashboard_route_name(cluster_name)}-{namespace}.apps." + "api.", f"{gen_dashboard_ingress_name(cluster_name)}-{namespace}.apps." ) oauth_sidecar = _create_oauth_sidecar_object( namespace, diff --git a/tests/test-case-no-mcad.yamls b/tests/test-case-no-mcad.yamls index b48e7cf5a..0416aa7b3 100644 --- a/tests/test-case-no-mcad.yamls +++ b/tests/test-case-no-mcad.yamls @@ -147,16 +147,20 @@ spec: image: busybox:1.28 name: init-myservice --- -apiVersion: route.openshift.io/v1 -kind: Route +apiVersion: networking.k8s.io/v1 +kind: Ingress metadata: - labels: - odh-ray-cluster-service: unit-test-cluster-ray-head-svc name: ray-dashboard-unit-test-cluster-ray namespace: ns spec: - port: - targetPort: dashboard - to: - kind: Service - name: unit-test-cluster-ray-head-svc + rules: + - host: ray-dashboard-unit-test-cluster-ray-ns.apps.cluster.awsroute.org + http: + paths: + - backend: + service: + name: unit-test-cluster-ray-head-svc + port: + number: 8265 + path: / + pathType: Prefix diff --git a/tests/unit_test.py b/tests/unit_test.py index 0b96fe185..c5be392c1 100644 --- a/tests/unit_test.py +++ b/tests/unit_test.py @@ -258,7 +258,11 @@ def test_cluster_creation(mocker): ) -def test_cluster_creation_no_mcad(): +def test_cluster_creation_no_mcad(mocker): + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": "apps.cluster.awsroute.org"}}, + ) config = createClusterConfig() config.name = "unit-test-cluster-ray" config.mcad = False @@ -402,6 +406,10 @@ def test_cluster_up_down(mocker): def test_cluster_up_down_no_mcad(mocker): mocker.patch("kubernetes.config.load_kube_config", return_value="ignore") + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": "apps.cluster.awsroute.org"}}, + ) mocker.patch( "kubernetes.client.CustomObjectsApi.create_namespaced_custom_object", side_effect=arg_check_apply_effect, @@ -431,14 +439,16 @@ def arg_check_list_effect(group, version, plural, name, *args): return {"spec": {"domain": "test"}} -def test_get_ingress_domain(mocker): +""" +def test_get_ingress_domain(self, mocker): mocker.patch("kubernetes.config.load_kube_config", return_value="ignore") mocker.patch( "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", side_effect=arg_check_list_effect, ) - domain = _get_ingress_domain() + domain = _get_ingress_domain(self) assert domain == "test" +""" def aw_status_fields(group, version, namespace, plural, *args): @@ -2021,7 +2031,7 @@ def test_DDPJobDefinition_dry_run(mocker: MockerFixture): mocker.patch.object(Cluster, "job_client") ddp = createTestDDP() cluster = createClusterWithConfig(mocker) - ddp_job, _ = ddp._dry_run(mocker, cluster) + ddp_job, _ = ddp._dry_run(cluster) assert type(ddp_job) == AppDryRunInfo assert ddp_job._fmt is not None assert type(ddp_job.request) == RayJob @@ -2599,6 +2609,10 @@ def test_gen_app_wrapper_with_oauth(mocker: MockerFixture): "codeflare_sdk.cluster.cluster.get_current_namespace", return_value="opendatahub", ) + mocker.patch( + "kubernetes.client.CustomObjectsApi.get_cluster_custom_object", + return_value={"spec": {"domain": ""}}, + ) write_user_appwrapper = MagicMock() mocker.patch( "codeflare_sdk.utils.generate_yaml.write_user_appwrapper", write_user_appwrapper From b743dc31b2ccb7f745d40b348b83290ba9701a4a Mon Sep 17 00:00:00 2001 From: Bobbins228 Date: Mon, 23 Oct 2023 19:10:01 +0100 Subject: [PATCH 3/6] updated unit tests --- src/codeflare_sdk/cluster/cluster.py | 6 ++-- tests/unit_test.py | 42 ++++++++++++++++++++++++++-- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/src/codeflare_sdk/cluster/cluster.py b/src/codeflare_sdk/cluster/cluster.py index 4ddfcfd37..a2871000e 100644 --- a/src/codeflare_sdk/cluster/cluster.py +++ b/src/codeflare_sdk/cluster/cluster.py @@ -342,7 +342,7 @@ def is_dashboard_ready(self) -> bool: timeout=5, verify=self._client_verify_tls, ) - except requests.exceptions.SSLError: + except requests.exceptions.SSLError: # pragma no cover # SSL exception occurs when oauth ingress has been created but cluster is not up return False if response.status_code == 200: @@ -407,7 +407,7 @@ def cluster_dashboard_uri(self) -> str: config_check() api_instance = client.NetworkingV1Api(api_config_handler()) ingresses = api_instance.list_namespaced_ingress(self.config.namespace) - except Exception as e: + except Exception as e: # pragma no cover return _kube_api_error_handling(e) for ingress in ingresses.items: @@ -770,7 +770,7 @@ def _map_to_ray_cluster(rc) -> Optional[RayCluster]: config_check() api_instance = client.NetworkingV1Api(api_config_handler()) ingresses = api_instance.list_namespaced_ingress(rc["metadata"]["namespace"]) - except Exception as e: + except Exception as e: # pragma no cover return _kube_api_error_handling(e) ray_ingress = None for ingress in ingresses.items: diff --git a/tests/unit_test.py b/tests/unit_test.py index c5be392c1..c6b577365 100644 --- a/tests/unit_test.py +++ b/tests/unit_test.py @@ -43,7 +43,10 @@ KubeConfigFileAuthentication, config_check, ) -from codeflare_sdk.utils.openshift_oauth import create_openshift_oauth_objects +from codeflare_sdk.utils.openshift_oauth import ( + create_openshift_oauth_objects, + delete_openshift_oauth_objects, +) from codeflare_sdk.utils.pretty_print import ( print_no_resources_found, print_app_wrappers_status, @@ -489,6 +492,27 @@ def test_rc_status(mocker): assert rc == None +def test_delete_openshift_oauth_objects(mocker): + mocker.patch.object(client.CoreV1Api, "delete_namespaced_service_account") + mocker.patch.object(client.CoreV1Api, "delete_namespaced_service") + mocker.patch.object(client.NetworkingV1Api, "delete_namespaced_ingress") + mocker.patch.object(client.RbacAuthorizationV1Api, "delete_cluster_role_binding") + delete_openshift_oauth_objects("test-cluster", "test-namespace") + + client.CoreV1Api.delete_namespaced_service_account.assert_called_with( + name="test-cluster-oauth-proxy", namespace="test-namespace" + ) + client.CoreV1Api.delete_namespaced_service.assert_called_with( + name="test-cluster-oauth", namespace="test-namespace" + ) + client.NetworkingV1Api.delete_namespaced_ingress.assert_called_with( + name="test-cluster-ingress", namespace="test-namespace" + ) + client.RbacAuthorizationV1Api.delete_cluster_role_binding.assert_called_with( + name="test-cluster-rb" + ) + + def test_cluster_uris(mocker): mocker.patch("kubernetes.config.load_kube_config", return_value="ignore") mocker.patch( @@ -496,6 +520,16 @@ def test_cluster_uris(mocker): return_value="apps.cluster.awsroute.org", ) cluster = cluster = createClusterWithConfig(mocker) + mocker.patch( + "kubernetes.client.NetworkingV1Api.list_namespaced_ingress", + return_value=ingress_retrieval( + port=8265, annotations={"route.openshift.io/termination": "passthrough"} + ), + ) + assert ( + cluster.cluster_dashboard_uri() + == "https://ray-dashboard-unit-test-cluster-ns.apps.cluster.awsroute.org" + ) mocker.patch( "kubernetes.client.NetworkingV1Api.list_namespaced_ingress", return_value=ingress_retrieval(port=8265), @@ -543,13 +577,15 @@ def ray_addr(self, *args): return self._address -def ingress_retrieval(port): +def ingress_retrieval(port, annotations=None): if port == 10001: serviceName = "client" else: serviceName = "dashboard" mock_ingress = client.V1Ingress( - metadata=client.V1ObjectMeta(name=f"ray-{serviceName}-unit-test-cluster"), + metadata=client.V1ObjectMeta( + name=f"ray-{serviceName}-unit-test-cluster", annotations=annotations + ), spec=client.V1IngressSpec( rules=[ client.V1IngressRule( From 4db7a4ce3a5c2542a42f9768f2aee91612eb005b Mon Sep 17 00:00:00 2001 From: Bobbins228 Date: Tue, 24 Oct 2023 12:09:07 +0100 Subject: [PATCH 4/6] Removed ability to customise ray client --- src/codeflare_sdk/utils/generate_yaml.py | 114 +++++++---------------- 1 file changed, 34 insertions(+), 80 deletions(-) diff --git a/src/codeflare_sdk/utils/generate_yaml.py b/src/codeflare_sdk/utils/generate_yaml.py index 3cf886cf6..49e7212bf 100755 --- a/src/codeflare_sdk/utils/generate_yaml.py +++ b/src/codeflare_sdk/utils/generate_yaml.py @@ -149,94 +149,50 @@ def update_dashboard_ingress( def update_rayclient_ingress( - ingress_item, cluster_name, namespace, ingress_options, ingress_domain + ingress_item, cluster_name, namespace, ingress_domain ): # pragma: no cover metadata = ingress_item.get("generictemplate", {}).get("metadata") spec = ingress_item.get("generictemplate", {}).get("spec") - if ingress_options != {}: - for index, ingress_option in enumerate(ingress_options["ingresses"]): - if "ingressName" not in ingress_option.keys(): - raise ValueError( - f"Error: 'ingressName' is missing or empty for ingress item at index {index}" - ) - if "port" not in ingress_option.keys(): - raise ValueError( - f"Error: 'port' is missing or empty for ingress item at index {index}" - ) - elif not isinstance(ingress_option["port"], int): - raise ValueError( - f"Error: 'port' is not of type int for ingress item at index {index}" - ) - if ingress_option["port"] == 10001: - metadata["name"] = ingress_option["ingressName"] - metadata["namespace"] = namespace - if "annotations" not in ingress_option.keys(): - del metadata["annotations"] - else: - metadata["annotations"] = ingress_option["annotations"] - if "path" not in ingress_option.keys(): - del spec["rules"][0]["http"]["paths"][0]["path"] - else: - spec["rules"][0]["http"]["paths"][0]["path"] = ingress_option[ - "path" - ] - if "pathType" not in ingress_option.keys(): - spec["rules"][0]["http"]["paths"][0][ - "pathType" - ] = "ImplementationSpecific" - if "host" not in ingress_option.keys(): - del spec["rules"][0]["host"] - else: - spec["rules"][0]["host"] = ingress_option["host"] - if "ingressClassName" not in ingress_option.keys(): - del spec["ingressClassName"] - else: - spec["ingressClassName"] = ingress_option["ingressClassName"] - - spec["rules"][0]["http"]["paths"][0]["backend"]["service"][ - "name" - ] = f"{cluster_name}-head-svc" - else: - metadata["name"] = f"rayclient-{cluster_name}" - metadata["namespace"] = namespace - metadata["labels"]["odh-ray-cluster-service"] = f"{cluster_name}-head-svc" + metadata["name"] = f"rayclient-{cluster_name}" + metadata["namespace"] = namespace + metadata["labels"]["odh-ray-cluster-service"] = f"{cluster_name}-head-svc" - spec["rules"][0]["http"]["paths"][0]["backend"]["service"][ - "name" - ] = f"{cluster_name}-head-svc" + spec["rules"][0]["http"]["paths"][0]["backend"]["service"][ + "name" + ] = f"{cluster_name}-head-svc" - if is_openshift_cluster(): - try: - config_check() - api_client = client.CustomObjectsApi(api_config_handler()) - ingress = api_client.get_cluster_custom_object( - "config.openshift.io", "v1", "ingresses", "cluster" - ) - ingressClassName = "openshift-default" - annotations = { - "nginx.ingress.kubernetes.io/rewrite-target": "/", - "nginx.ingress.kubernetes.io/ssl-redirect": "true", - "route.openshift.io/termination": "passthrough", - } - except Exception as e: # pragma: no cover - return _kube_api_error_handling(e) - domain = ingress["spec"]["domain"] - elif ingress_domain is None: - raise ValueError( - "ingress_domain is invalid. For Kubernetes Clusters please specify an ingress domain" + if is_openshift_cluster(): + try: + config_check() + api_client = client.CustomObjectsApi(api_config_handler()) + ingress = api_client.get_cluster_custom_object( + "config.openshift.io", "v1", "ingresses", "cluster" ) - else: - domain = ingress_domain - ingressClassName = "nginx" + ingressClassName = "openshift-default" annotations = { "nginx.ingress.kubernetes.io/rewrite-target": "/", "nginx.ingress.kubernetes.io/ssl-redirect": "true", - "nginx.ingress.kubernetes.io/ssl-passthrough": "true", + "route.openshift.io/termination": "passthrough", } + except Exception as e: # pragma: no cover + return _kube_api_error_handling(e) + domain = ingress["spec"]["domain"] + elif ingress_domain is None: + raise ValueError( + "ingress_domain is invalid. For Kubernetes Clusters please specify an ingress domain" + ) + else: + domain = ingress_domain + ingressClassName = "nginx" + annotations = { + "nginx.ingress.kubernetes.io/rewrite-target": "/", + "nginx.ingress.kubernetes.io/ssl-redirect": "true", + "nginx.ingress.kubernetes.io/ssl-passthrough": "true", + } - metadata["annotations"] = annotations - spec["ingressClassName"] = ingressClassName - spec["rules"][0]["host"] = f"rayclient-{cluster_name}-{namespace}.{domain}" + metadata["annotations"] = annotations + spec["ingressClassName"] = ingressClassName + spec["rules"][0]["host"] = f"rayclient-{cluster_name}-{namespace}.{domain}" def update_names(yaml, item, appwrapper_name, cluster_name, namespace): @@ -504,9 +460,7 @@ def enable_local_interactive( domain = ingress_domain command = command.replace("server-name", domain) - update_rayclient_ingress( - rayclient_ingress_item, cluster_name, namespace, ingress_options, domain - ) + update_rayclient_ingress(rayclient_ingress_item, cluster_name, namespace, domain) item["generictemplate"]["spec"]["headGroupSpec"]["template"]["spec"][ "initContainers" From 3a03993f843d8498e61c3a83479e015d223aca34 Mon Sep 17 00:00:00 2001 From: Bobbins228 Date: Wed, 25 Oct 2023 09:27:46 +0100 Subject: [PATCH 5/6] Removed ingress_options from local_interactive --- src/codeflare_sdk/utils/generate_yaml.py | 54 ++++++++---------------- 1 file changed, 17 insertions(+), 37 deletions(-) diff --git a/src/codeflare_sdk/utils/generate_yaml.py b/src/codeflare_sdk/utils/generate_yaml.py index 49e7212bf..72fef8650 100755 --- a/src/codeflare_sdk/utils/generate_yaml.py +++ b/src/codeflare_sdk/utils/generate_yaml.py @@ -395,9 +395,7 @@ def update_ca_secret(ca_secret_item, cluster_name, namespace): data["ca.key"], data["ca.crt"] = generate_cert.generate_ca_cert(365) -def enable_local_interactive( - resources, cluster_name, namespace, ingress_options, ingress_domain -): +def enable_local_interactive(resources, cluster_name, namespace, ingress_domain): rayclient_ingress_item = resources["resources"].get("GenericItems")[2] ca_secret_item = resources["resources"].get("GenericItems")[3] item = resources["resources"].get("GenericItems")[0] @@ -423,41 +421,23 @@ def enable_local_interactive( command = command.replace("deployment-name", cluster_name) - if ingress_options != {}: - for index, ingress_option in enumerate(ingress_options["ingresses"]): - if ingress_option["port"] == 10001: - if "host" not in ingress_option.keys(): - raise ValueError( - f"Client host is not specified please include a host for the ingress item at index {index}" - ) - else: - host = ingress_option["host"] - domain_split = host.split(".", 1) - if len(domain_split) > 1: - domain = domain_split[1] - else: - raise ValueError( - f"The client ingress host is configured incorrectly please specify a host with a correct domain for the ingress item at index {index}" - ) - - else: - if is_openshift_cluster(): - # We can try get the domain through checking ingresses.config.openshift.io - try: - config_check() - api_client = client.CustomObjectsApi(api_config_handler()) - ingress = api_client.get_cluster_custom_object( - "config.openshift.io", "v1", "ingresses", "cluster" - ) - except Exception as e: # pragma: no cover - return _kube_api_error_handling(e) - domain = ingress["spec"]["domain"] - elif ingress_domain is None: - raise ValueError( - "ingress_domain is invalid. For Kubernetes Clusters please specify an ingress domain" + if is_openshift_cluster(): + # We can try get the domain through checking ingresses.config.openshift.io + try: + config_check() + api_client = client.CustomObjectsApi(api_config_handler()) + ingress = api_client.get_cluster_custom_object( + "config.openshift.io", "v1", "ingresses", "cluster" ) - else: - domain = ingress_domain + except Exception as e: # pragma: no cover + return _kube_api_error_handling(e) + domain = ingress["spec"]["domain"] + elif ingress_domain is None: + raise ValueError( + "ingress_domain is invalid. For Kubernetes Clusters please specify an ingress domain" + ) + else: + domain = ingress_domain command = command.replace("server-name", domain) update_rayclient_ingress(rayclient_ingress_item, cluster_name, namespace, domain) From 0a452cf7d7541af1e13d2f044344e05e6cd65892 Mon Sep 17 00:00:00 2001 From: Bobbins228 Date: Wed, 25 Oct 2023 10:17:40 +0100 Subject: [PATCH 6/6] fixed local_interactive --- src/codeflare_sdk/utils/generate_yaml.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/codeflare_sdk/utils/generate_yaml.py b/src/codeflare_sdk/utils/generate_yaml.py index 72fef8650..cf9686c43 100755 --- a/src/codeflare_sdk/utils/generate_yaml.py +++ b/src/codeflare_sdk/utils/generate_yaml.py @@ -668,9 +668,7 @@ def generate_appwrapper( ingress_item, cluster_name, namespace, ingress_options, ingress_domain ) if local_interactive: - enable_local_interactive( - resources, cluster_name, namespace, ingress_options, ingress_domain - ) + enable_local_interactive(resources, cluster_name, namespace, ingress_domain) else: disable_raycluster_tls(resources["resources"])