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

Pipeline Editor: Extend data volume node property #2961

Merged
merged 23 commits into from
Oct 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
4997683
Change ui_details_map datastructure to prep for all object-valued pro…
kiersten-stokes Oct 5, 2022
7f43372
Pass value to DisableNodeCaching
kiersten-stokes Oct 6, 2022
d51f6ad
Make property attributes a well-defined object and renaming
kiersten-stokes Oct 6, 2022
1f403b8
Update propagation logic to handle runtime image separately
kiersten-stokes Oct 6, 2022
97a238a
Add an another attribute class for ListItem properties; update method…
kiersten-stokes Oct 7, 2022
a459c7a
Add enum and default_value as attribute properties
kiersten-stokes Oct 7, 2022
dc392fa
NIT: reorganize ElyraProperty property names
kiersten-stokes Oct 7, 2022
faf64aa
Make ElyraProperty and ElyraPropertyListItem ABCs
kiersten-stokes Oct 7, 2022
73924f7
Extend support for volume mounts
ptitzler Oct 8, 2022
5ada326
Fix test resource
ptitzler Oct 8, 2022
6677e83
Update validation error message and tests
ptitzler Oct 10, 2022
fe7caaf
Add overrides dependency and delete unnecessary docstrings to improve…
kiersten-stokes Oct 10, 2022
6e1d9c7
Adjust return type of overridden method
kiersten-stokes Oct 10, 2022
66fde70
Address NITs from recent review
kiersten-stokes Oct 10, 2022
2a25ede
Update special handling of runtime image during propagation
kiersten-stokes Oct 10, 2022
b1f5721
Update test to ensure previous commit changes work properly
kiersten-stokes Oct 10, 2022
a0f43b2
Merge branch 'obj-properties' of github.com:kiersten-stokes/elyra int…
ptitzler Oct 10, 2022
698d539
Revert addition of overrides package and decorators
kiersten-stokes Oct 10, 2022
8239fba
Update elyra/pipeline/airflow/processor_airflow.py
ptitzler Oct 11, 2022
250844f
Improve should_discard method implementation and documentation
ptitzler Oct 11, 2022
47e807a
Merge branch 'obj-properties' of github.com:kiersten-stokes/elyra int…
ptitzler Oct 11, 2022
ad5783b
Merge branch 'main' of github.com:ptitzler/elyra into extend-volume-p…
ptitzler Oct 11, 2022
24ac087
Improve (unrelated) node caching rendering
ptitzler Oct 12, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/source/user_guide/pipelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,8 @@ The following alphabetically sorted list identifies the node properties that are
- Format:
- _Mount path_: the path where the PVC shall be mounted in the container. Example: `/mnt/datavol/`
- _Persistent volume claim name_: a valid [Kubernetes resource name](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names) identifying a PVC that exists in the Kubernetes namespace where the pipeline nodes are executed. Example: `my-data-pvc`
- _Sub path_: relative path within the volume from which the container's volume should be mounted. Defaults to the volume's root. Example: `existing/path/in/volume`
- _Mount volume read-only_: whether to mount the volume in read-only mode
- Data volumes are not mounted when the pipeline is executed locally.

##### Disable node caching
Expand Down
10 changes: 8 additions & 2 deletions elyra/pipeline/airflow/processor_airflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,8 @@ def render_mounts(self, elyra_parameters: Dict[str, ElyraProperty]) -> str:
str_to_render = ""
for v in elyra_parameters.get(pipeline_constants.MOUNTED_VOLUMES, []):
str_to_render += f"""
VolumeMount(name="{v.pvc_name}", mount_path="{v.path}", sub_path=None, read_only=False),"""
VolumeMount(name="{v.pvc_name}", mount_path="{v.path}",
sub_path="{v.sub_path}", read_only={v.read_only}),"""
return dedent(str_to_render)

def render_secrets(self, elyra_parameters: Dict[str, ElyraProperty], cos_secret: Optional[str]) -> str:
Expand Down Expand Up @@ -679,7 +680,12 @@ def add_mounted_volume(self, instance: VolumeMount, execution_object: Any, **kwa
}
)
execution_object["volume_mounts"].append(
{"mountPath": instance.path, "name": instance.pvc_name, "read_only": False}
{
"mountPath": instance.path,
"name": instance.pvc_name,
"sub_path": instance.sub_path,
"read_only": instance.read_only,
}
)

def add_kubernetes_pod_annotation(self, instance: KubernetesAnnotation, execution_object: Any, **kwargs) -> None:
Expand Down
37 changes: 36 additions & 1 deletion elyra/pipeline/component_parameter.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ def get_schema(cls) -> Dict[str, Any]:
"""Build the JSON schema for an Elyra-owned component property"""
schema = super().get_schema()
schema["enum"] = ["True", "False"]
schema["uihints"] = {"ui:placeholder": "Use runtime default"}
return schema

def get_value_for_display(self) -> Dict[str, Any]:
Expand Down Expand Up @@ -499,11 +500,32 @@ class VolumeMount(ElyraPropertyListItem):
required=True,
use_in_key=False,
),
ListItemPropertyAttribute(
attribute_id="sub_path",
display_name="Sub Path",
placeholder="relative/path/within/volume",
input_type="string",
hidden=False,
required=False,
use_in_key=False,
),
ListItemPropertyAttribute(
attribute_id="read_only",
display_name="Mount volume read-only",
placeholder=None,
default_value=False,
input_type="boolean",
hidden=False,
required=False,
use_in_key=False,
),
]

def __init__(self, path, pvc_name, **kwargs):
def __init__(self, path: str, pvc_name: str, sub_path: str, read_only: bool, **kwargs):
self.path = path
self.pvc_name = pvc_name
self.sub_path = sub_path
self.read_only = read_only

def get_all_validation_errors(self) -> List[str]:
"""Identify configuration issues for this instance"""
Expand All @@ -514,14 +536,27 @@ def get_all_validation_errors(self) -> List[str]:
validation_errors.append("Required persistent volume claim name was not specified.")
elif not is_valid_kubernetes_resource_name(self.pvc_name):
validation_errors.append(f"PVC name '{self.pvc_name}' is not a valid Kubernetes resource name.")
if self.sub_path and self.sub_path.startswith("/"):
validation_errors.append(f"Sub-path '{self.sub_path}' must be a relative path.")

return validation_errors

def add_to_execution_object(self, runtime_processor: RuntimePipelineProcessor, execution_object: Any, **kwargs):
"""Add VolumeMount instance to the execution object for the given runtime processor"""
self.path = f"/{self.path.strip('/')}" # normalize path
if self.read_only is None:
self.read_only = False
runtime_processor.add_mounted_volume(instance=self, execution_object=execution_object, **kwargs)

def should_discard(self) -> bool:
"""
Returns a boolean indicating whether this VolumeMount instance should be silently discarded on
the basis of its mount path, PVC name, and sub-path attribute values. If these attributes
don't contain values this instance will not be validated or processed.
"""
# ignore the read_only attribute because it always contains a value
return not (self.path or self.pvc_name or self.sub_path)


class KubernetesAnnotation(ElyraPropertyListItem):
"""An ElyraProperty representing a single Kubernetes annotation"""
Expand Down
9 changes: 8 additions & 1 deletion elyra/pipeline/kfp/processor_kfp.py
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,14 @@ def add_mounted_volume(self, instance: VolumeMount, execution_object: Any, **kwa
)
if volume not in execution_object.volumes:
execution_object.add_volume(volume)
execution_object.container.add_volume_mount(V1VolumeMount(mount_path=instance.path, name=instance.pvc_name))
execution_object.container.add_volume_mount(
V1VolumeMount(
mount_path=instance.path,
name=instance.pvc_name,
sub_path=instance.sub_path,
read_only=instance.read_only,
)
)

def add_kubernetes_pod_annotation(self, instance: KubernetesAnnotation, execution_object: Any, **kwargs) -> None:
"""Add KubernetesAnnotation instance to the execution object for the given runtime processor"""
Expand Down
13 changes: 13 additions & 0 deletions elyra/tests/pipeline/resources/additional_generic_properties.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@
"type": "string",
"title": "Persistent Volume Claim Name",
"default": ""
},
"read_only": {
"default": false,
"title": "Mount volume read-only",
"type": "boolean"
},
"sub_path": {
"default": "",
"title": "Sub Path",
"type": "string"
}
},
"required": ["path", "pvc_name"]
Expand All @@ -58,6 +68,9 @@
},
"pvc_name": {
"ui:placeholder": "pvc-name"
},
"sub_path": {
"ui:placeholder": "relative/path/within/volume"
}
}
},
Expand Down
8 changes: 5 additions & 3 deletions elyra/tests/pipeline/test_pipeline_definition.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,11 +188,13 @@ def test_property_id_collision_with_system_property(monkeypatch, catalog_instanc
# pipeline node and in the pipeline default properties
derive1_vols = custom_node_derive1.get_component_parameter(MOUNTED_VOLUMES)
assert derive1_vols.to_dict() == {
"/mnt/vol2": {"path": "/mnt/vol2", "pvc_name": "pvc-claim-2"},
"/mnt/vol1": {"path": "/mnt/vol1", "pvc_name": "pvc-claim-1"},
"/mnt/vol2": {"path": "/mnt/vol2", "pvc_name": "pvc-claim-2", "read_only": None, "sub_path": None},
"/mnt/vol1": {"path": "/mnt/vol1", "pvc_name": "pvc-claim-1", "read_only": None, "sub_path": None},
}
derive2_vols = custom_node_derive2.get_component_parameter(MOUNTED_VOLUMES)
assert derive2_vols.to_dict() == {"/mnt/vol2": {"path": "/mnt/vol2", "pvc_name": "pvc-claim-2"}}
assert derive2_vols.to_dict() == {
"/mnt/vol2": {"path": "/mnt/vol2", "pvc_name": "pvc-claim-2", "read_only": None, "sub_path": None}
}

# TestOperator defines its own "mounted_volumes" property
# and should skip the Elyra system property of the same name
Expand Down
31 changes: 21 additions & 10 deletions elyra/tests/pipeline/test_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -446,8 +446,13 @@ def test_valid_node_property_volumes(validation_manager):

volumes = ElyraPropertyList(
[
VolumeMount(path="/mount/test", pvc_name="rwx-test-claim"), # valid
VolumeMount(path="/mount/test_two", pvc_name="second-claim"), # valid
VolumeMount(path="/mount/test", pvc_name="rwx-test-claim", sub_path=None, read_only=False),
VolumeMount(path="/mount/test", pvc_name="rwx-test-claim", sub_path="", read_only=False),
VolumeMount(path="/mount/test", pvc_name="rwx-test-claim", sub_path="relative/path", read_only=False),
VolumeMount(path="/mount/test_two", pvc_name="second-claim", sub_path="", read_only=True),
VolumeMount(path="/mount/test_two", pvc_name="second-claim", sub_path="path", read_only=True),
VolumeMount(path="/mount/test_two", pvc_name="second-claim", sub_path="path/", read_only=True),
VolumeMount(path="/mount/test_two", pvc_name="second-claim", sub_path="path/in/volume", read_only=None),
]
)
node_dict["app_data"]["component_parameters"][MOUNTED_VOLUMES] = volumes
Expand All @@ -466,13 +471,18 @@ def test_invalid_node_property_volumes(validation_manager):

volumes = ElyraPropertyList(
[
VolumeMount(path="", pvc_name=""), # missing mount path and pvc name
VolumeMount(path=None, pvc_name=None), # missing mount path and pvc name
VolumeMount(path="", pvc_name="pvc"), # missing mount path
VolumeMount(path=None, pvc_name="pvc"), # missing mount path
VolumeMount(path="/path", pvc_name=""), # missing pvc name
VolumeMount(path="/path/", pvc_name=None), # missing pvc name
VolumeMount(path="/mount/test_four", pvc_name="second#claim"), # invalid pvc name
VolumeMount(path="", pvc_name="", sub_path="", read_only=True), # missing mount path and pvc name
VolumeMount(path=None, pvc_name=None, sub_path=None, read_only=True), # missing mount path and pvc name
VolumeMount(path="", pvc_name="pvc", sub_path="", read_only=True), # missing mount path
VolumeMount(path=None, pvc_name="pvc", sub_path=None, read_only=True), # missing mount path
VolumeMount(path="/path", pvc_name="", sub_path="", read_only=True), # missing pvc name
VolumeMount(path="/path/", pvc_name=None, sub_path=None, read_only=False), # missing pvc name
VolumeMount(
path="/mount/test_four", pvc_name="second#claim", sub_path=None, read_only=False
), # invalid pvc name
VolumeMount(
path="/path", pvc_name="pvc", sub_path="/absolute/path", read_only=False
), # sub_path must be relative
]
)
node_dict["app_data"]["component_parameters"][MOUNTED_VOLUMES] = volumes
Expand All @@ -482,7 +492,7 @@ def test_invalid_node_property_volumes(validation_manager):
node_id=node.id, node_label=node.label, node=node, param_name=MOUNTED_VOLUMES, response=response
)
issues = response.to_json().get("issues")
assert len(issues) == 9, issues
assert len(issues) == 10, issues
assert issues[0]["severity"] == 1
assert issues[0]["type"] == "invalidVolumeMount"
assert issues[0]["data"]["propertyName"] == MOUNTED_VOLUMES
Expand All @@ -496,6 +506,7 @@ def test_invalid_node_property_volumes(validation_manager):
assert "Required persistent volume claim name was not specified." in issues[6]["message"]
assert "Required persistent volume claim name was not specified." in issues[7]["message"]
assert "PVC name 'second#claim' is not a valid Kubernetes resource name." in issues[8]["message"]
assert "Sub-path '/absolute/path' must be a relative path." in issues[9]["message"]


def test_valid_node_property_kubernetes_toleration(validation_manager):
Expand Down