RHOAIENG-33283: Error handling, remove obsolete volumes, housekeeping

Author: kryanbeane

src/codeflare_sdk/common/widgets/test_widgets.py

@@ -106,7 +106,7 @@ def test_view_clusters(mocker, capsys):
     # Prepare to run view_clusters when notebook environment is detected
     mocker.patch("codeflare_sdk.common.widgets.widgets.is_notebook", return_value=True)
     mock_get_current_namespace = mocker.patch(
-        "codeflare_sdk.common.utils.get_current_namespace",
+        "codeflare_sdk.common.widgets.widgets.get_current_namespace",
         return_value="default",
     )
     namespace = mock_get_current_namespace.return_value

src/codeflare_sdk/common/widgets/widgets.py

@@ -353,8 +353,6 @@ def view_clusters(namespace: str = None):
         )
         return  # Exit function if not in Jupyter Notebook
 
-    from ...common.utils import get_current_namespace
-
     if not namespace:
         namespace = get_current_namespace()
 

src/codeflare_sdk/ray/cluster/cluster.py

@@ -520,7 +520,7 @@ def cluster_dashboard_uri(self) -> str:
                     protocol = "https" if route["spec"].get("tls") else "http"
                     return f"{protocol}://{route['spec']['host']}"
             # No route found for this cluster
-            return "Dashboard not available yet, have you run cluster.up()?"
+            return "Dashboard not available yet, have you run cluster.apply()?"
         else:
             try:
                 api_instance = client.NetworkingV1Api(get_api_client())

src/codeflare_sdk/ray/rayjobs/config.py

@@ -22,8 +22,6 @@
 from dataclasses import dataclass, field, fields
 from typing import Dict, List, Optional, Union, get_args, get_origin, Any, Tuple
 from kubernetes.client import (
-    V1ConfigMapVolumeSource,
-    V1KeyToPath,
     V1LocalObjectReference,
     V1SecretVolumeSource,
     V1Toleration,
@@ -62,50 +60,6 @@
     "huawei.com/Ascend310": "NPU",
 }
 
-# Default volume mounts for CA certificates
-DEFAULT_VOLUME_MOUNTS = [
-    V1VolumeMount(
-        mount_path="/etc/pki/tls/certs/odh-trusted-ca-bundle.crt",
-        name="odh-trusted-ca-cert",
-        sub_path="odh-trusted-ca-bundle.crt",
-    ),
-    V1VolumeMount(
-        mount_path="/etc/ssl/certs/odh-trusted-ca-bundle.crt",
-        name="odh-trusted-ca-cert",
-        sub_path="odh-trusted-ca-bundle.crt",
-    ),
-    V1VolumeMount(
-        mount_path="/etc/pki/tls/certs/odh-ca-bundle.crt",
-        name="odh-ca-cert",
-        sub_path="odh-ca-bundle.crt",
-    ),
-    V1VolumeMount(
-        mount_path="/etc/ssl/certs/odh-ca-bundle.crt",
-        name="odh-ca-cert",
-        sub_path="odh-ca-bundle.crt",
-    ),
-]
-
-# Default volumes for CA certificates
-DEFAULT_VOLUMES = [
-    V1Volume(
-        name="odh-trusted-ca-cert",
-        config_map=V1ConfigMapVolumeSource(
-            name="odh-trusted-ca-bundle",
-            items=[V1KeyToPath(key="ca-bundle.crt", path="odh-trusted-ca-bundle.crt")],
-            optional=True,
-        ),
-    ),
-    V1Volume(
-        name="odh-ca-cert",
-        config_map=V1ConfigMapVolumeSource(
-            name="odh-trusted-ca-bundle",
-            items=[V1KeyToPath(key="odh-ca-bundle.crt", path="odh-ca-bundle.crt")],
-            optional=True,
-        ),
-    ),
-]
-
 
 @dataclass
 class ManagedClusterConfig:
@@ -426,7 +380,7 @@ def _build_pod_spec(self, container: V1Container, is_head: bool) -> V1PodSpec:
 
     def _generate_volume_mounts(self) -> list:
         """Generate volume mounts for the container."""
-        volume_mounts = DEFAULT_VOLUME_MOUNTS.copy()
+        volume_mounts = []
 
         # Add custom volume mounts if specified
         if hasattr(self, "volume_mounts") and self.volume_mounts:
@@ -436,7 +390,7 @@ def _generate_volume_mounts(self) -> list:
 
     def _generate_volumes(self) -> list:
         """Generate volumes for the pod."""
-        volumes = DEFAULT_VOLUMES.copy()
+        volumes = []
 
         # Add custom volumes if specified
         if hasattr(self, "volumes") and self.volumes:

src/codeflare_sdk/ray/rayjobs/rayjob.py

@@ -169,11 +169,6 @@ def submit(self) -> str:
         # Extract files from entrypoint and runtime_env working_dir
         files = extract_all_local_files(self)
 
-        # Create Secret for files (will be mounted to submitter pod)
-        secret_name = None
-        if files:
-            secret_name = f"{self.name}-files"
-
         rayjob_cr = self._build_rayjob_cr()
 
         logger.info(f"Submitting RayJob {self.name} to Kuberay operator")

src/codeflare_sdk/ray/rayjobs/runtime_env.py

@@ -113,7 +113,7 @@ def extract_all_local_files(job: RayJob) -> Optional[Dict[str, str]]:
 
 def _zip_directory(directory_path: str) -> Optional[bytes]:
     """
-    Zip entire directory preserving structure, excluding Jupyter notebook files.
+    Zip entire directory preserving structure, excluding Jupyter notebook and markdown files.
 
     Args:
         directory_path: Path to directory to zip
@@ -338,29 +338,22 @@ def create_secret_from_spec(
 
     metadata = client.V1ObjectMeta(**secret_spec["metadata"])
 
-    # Add owner reference if we have the RayJob result
-    if (
-        rayjob_result
-        and isinstance(rayjob_result, dict)
-        and rayjob_result.get("metadata", {}).get("uid")
-    ):
-        logger.info(
-            f"Adding owner reference to Secret '{secret_name}' with RayJob UID: {rayjob_result['metadata']['uid']}"
-        )
-        metadata.owner_references = [
-            client.V1OwnerReference(
-                api_version="ray.io/v1",
-                kind="RayJob",
-                name=job.name,
-                uid=rayjob_result["metadata"]["uid"],
-                controller=True,
-                block_owner_deletion=True,
-            )
-        ]
-    else:
-        logger.warning(
-            f"No valid RayJob result with UID found, Secret '{secret_name}' will not have owner reference. Result: {rayjob_result}"
+    # Add owner reference to ensure proper cleanup
+    # We can trust that rayjob_result contains UID since submit_job() only returns
+    # complete K8s resources or None, and we already validated result exists
+    logger.info(
+        f"Adding owner reference to Secret '{secret_name}' with RayJob UID: {rayjob_result['metadata']['uid']}"
+    )
+    metadata.owner_references = [
+        client.V1OwnerReference(
+            api_version="ray.io/v1",
+            kind="RayJob",
+            name=job.name,
+            uid=rayjob_result["metadata"]["uid"],
+            controller=True,
+            block_owner_deletion=True,
         )
+    ]
 
     # Convert dict spec to V1Secret
     # Use stringData instead of data to avoid double base64 encoding
@@ -409,5 +402,4 @@ def create_file_secret(
     )
 
     # Create Secret with owner reference
-    # TODO Error handling
     create_secret_from_spec(job, secret_spec, rayjob_result)

src/codeflare_sdk/ray/rayjobs/test/test_runtime_env.py

@@ -176,7 +176,16 @@ def test_create_secret_from_spec(auto_mock_setup):
         "data": {"test.py": "print('test')"},
     }
 
-    result = create_secret_from_spec(rayjob, secret_spec)
+    # Provide valid RayJob result with UID as KubeRay client would
+    rayjob_result = {
+        "metadata": {
+            "name": "test-job",
+            "namespace": "test-namespace",
+            "uid": "test-uid-12345",
+        }
+    }
+
+    result = create_secret_from_spec(rayjob, secret_spec, rayjob_result)
 
     assert result == "test-files"
     mock_api_instance.create_namespaced_secret.assert_called_once()
@@ -205,7 +214,16 @@ def test_create_secret_already_exists(auto_mock_setup):
         "data": {"test.py": "print('test')"},
     }
 
-    result = create_secret_from_spec(rayjob, secret_spec)
+    # Provide valid RayJob result with UID as KubeRay client would
+    rayjob_result = {
+        "metadata": {
+            "name": "test-job",
+            "namespace": "test-namespace",
+            "uid": "test-uid-67890",
+        }
+    }
+
+    result = create_secret_from_spec(rayjob, secret_spec, rayjob_result)
 
     assert result == "test-files"
     mock_api_instance.create_namespaced_secret.assert_called_once()
@@ -271,92 +289,6 @@ def test_create_secret_with_owner_reference_basic(mocker, auto_mock_setup, caplo
     mock_api_instance.create_namespaced_secret.assert_called_once()
 
 
-def test_create_secret_without_owner_reference_no_uid(mocker, auto_mock_setup, caplog):
-    """
-    Test creating Secret without owner reference when RayJob has no UID.
-    """
-    mock_api_instance = auto_mock_setup["k8s_api"]
-
-    mock_v1_metadata = mocker.patch("kubernetes.client.V1ObjectMeta")
-    mock_metadata_instance = MagicMock()
-    mock_v1_metadata.return_value = mock_metadata_instance
-
-    rayjob = RayJob(
-        job_name="test-job",
-        cluster_name="existing-cluster",
-        entrypoint="python test.py",
-        namespace="test-namespace",
-    )
-
-    secret_spec = {
-        "apiVersion": "v1",
-        "kind": "Secret",
-        "type": "Opaque",
-        "metadata": {"name": "test-files", "namespace": "test-namespace"},
-        "data": {"test.py": "print('test')"},
-    }
-
-    # RayJob result without UID
-    rayjob_result = {
-        "metadata": {
-            "name": "test-job",
-            "namespace": "test-namespace",
-            # No UID field
-        }
-    }
-
-    with caplog.at_level("WARNING"):
-        result = create_secret_from_spec(rayjob, secret_spec, rayjob_result)
-
-    assert result == "test-files"
-
-    # Verify warning was logged and no owner reference was set
-    assert (
-        "No valid RayJob result with UID found, Secret 'test-files' will not have owner reference"
-        in caplog.text
-    )
-
-    # The important part is that the warning was logged, indicating no owner reference was set
-    mock_api_instance.create_namespaced_secret.assert_called_once()
-
-
-def test_create_secret_with_invalid_rayjob_result(auto_mock_setup, caplog):
-    """
-    Test creating Secret with None or invalid rayjob_result.
-    """
-    mock_api_instance = auto_mock_setup["k8s_api"]
-
-    rayjob = RayJob(
-        job_name="test-job",
-        cluster_name="existing-cluster",
-        entrypoint="python test.py",
-        namespace="test-namespace",
-    )
-
-    secret_spec = {
-        "apiVersion": "v1",
-        "kind": "Secret",
-        "type": "Opaque",
-        "metadata": {"name": "test-files", "namespace": "test-namespace"},
-        "data": {"test.py": "print('test')"},
-    }
-
-    # Test with None
-    with caplog.at_level("WARNING"):
-        result = create_secret_from_spec(rayjob, secret_spec, None)
-
-    assert result == "test-files"
-    assert "No valid RayJob result with UID found" in caplog.text
-
-    # Test with string instead of dict
-    caplog.clear()
-    with caplog.at_level("WARNING"):
-        result = create_secret_from_spec(rayjob, secret_spec, "not-a-dict")
-
-    assert result == "test-files"
-    assert "No valid RayJob result with UID found" in caplog.text
-
-
 def test_file_handling_kubernetes_best_practice_flow(mocker, tmp_path):
     """
     Test the Kubernetes best practice flow: pre-declare volume, submit, create Secret.
@@ -446,7 +378,13 @@ def test_rayjob_submit_with_files_new_cluster(auto_mock_setup, tmp_path):
     Test RayJob submission with file detection for new cluster.
     """
     mock_api_instance = auto_mock_setup["rayjob_api"]
-    mock_api_instance.submit_job.return_value = True
+    mock_api_instance.submit_job.return_value = {
+        "metadata": {
+            "name": "test-job",
+            "namespace": "test-namespace",
+            "uid": "test-uid-files-12345",
+        }
+    }
 
     mock_k8s_instance = auto_mock_setup["k8s_api"]
 
@@ -507,8 +445,17 @@ def test_create_secret_api_error_non_409(auto_mock_setup):
         "data": {"test.py": "print('test')"},
     }
 
+    # Provide valid RayJob result with UID as KubeRay client would
+    rayjob_result = {
+        "metadata": {
+            "name": "test-job",
+            "namespace": "test-namespace",
+            "uid": "test-uid-api-error",
+        }
+    }
+
     with pytest.raises(RuntimeError, match="Failed to create Secret"):
-        create_secret_from_spec(rayjob, secret_spec)
+        create_secret_from_spec(rayjob, secret_spec, rayjob_result)
 
 
 def test_add_file_volumes_existing_volume_skip():