-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Support aws ecs readonly_root_filesystem #27496
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -252,6 +252,12 @@ def volumes(self) -> Optional[Sequence[Mapping[str, Any]]]: | |
return None | ||
return self.task_definition_dict.get("volumes") | ||
|
||
@property | ||
def readonly_root_filesystem(self) -> Optional[bool]: | ||
if not self.task_definition_dict: | ||
return None | ||
return self.task_definition_dict.get("readonly_root_filesystem") | ||
|
||
@property | ||
def repository_credentials(self) -> Optional[str]: | ||
if not self.task_definition_dict: | ||
|
@@ -712,6 +718,7 @@ def _run_task_kwargs( | |
runtime_platform=runtime_platform, | ||
volumes=container_context.volumes, | ||
mount_points=container_context.mount_points, | ||
readonly_root_filesystem=container_context.readonly_root_filesystem, | ||
repository_credentials=container_context.repository_credentials, | ||
linux_parameters=self.linux_parameters, | ||
) | ||
|
@@ -734,6 +741,7 @@ def _run_task_kwargs( | |
ephemeral_storage=container_context.run_resources.get("ephemeral_storage"), | ||
volumes=container_context.volumes, | ||
mount_points=container_context.mount_points, | ||
readonly_root_filesystem=container_context.readonly_root_filesystem, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. once this is just on the run launcher and not on EcsContainerContext, this can be |
||
additional_sidecars=container_context.run_sidecar_containers, | ||
repository_credentials=container_context.repository_credentials, | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -924,6 +924,51 @@ def test_launching_custom_task_definition(ecs, instance_cm, run, workspace, job, | |
assert run.run_id in str(override["command"]) | ||
|
||
|
||
@pytest.mark.parametrize("readonly_root_filesystem", [True, False]) | ||
def test_readonly_root_filesystem(ecs, instance_cm, run, workspace, job, remote_job, readonly_root_filesystem): | ||
with instance_cm( | ||
{ | ||
"task_definition": { | ||
"task_role_arn": "fake-task-role", | ||
"execution_role_arn": "fake-execution-role", | ||
"readonly_root_filesystem": readonly_root_filesystem, | ||
}, | ||
} | ||
) as instance: | ||
run = instance.create_run_for_job( | ||
job, | ||
remote_job_origin=remote_job.get_remote_origin(), | ||
job_code_origin=remote_job.get_python_origin(), | ||
) | ||
|
||
initial_task_definitions = ecs.list_task_definitions()["taskDefinitionArns"] | ||
initial_tasks = ecs.list_tasks()["taskArns"] | ||
|
||
# Launch the run | ||
instance.launch_run(run.run_id, workspace) | ||
|
||
# A new task definition is created | ||
task_definitions = ecs.list_task_definitions()["taskDefinitionArns"] | ||
assert len(task_definitions) == len(initial_task_definitions) + 1 | ||
|
||
# A new task is launched | ||
tasks = ecs.list_tasks()["taskArns"] | ||
assert len(tasks) == len(initial_tasks) + 1 | ||
|
||
task_arn = next(iter(set(tasks).difference(initial_tasks))) | ||
task = ecs.describe_tasks(tasks=[task_arn])["tasks"][0] | ||
task_definition_arn = task["taskDefinitionArn"] | ||
|
||
# Get the task definition and container definition | ||
task_definition = ecs.describe_task_definition(taskDefinition=task_definition_arn)[ | ||
"taskDefinition" | ||
] | ||
container_definition = task_definition["containerDefinitions"][0] | ||
|
||
# Assert that readonlyRootFilesystem is set to correctly | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment Spotted by Graphite Reviewer |
||
assert container_definition.get("readonlyRootFilesystem") is readonly_root_filesystem | ||
|
||
|
||
def test_eventual_consistency(ecs, instance, workspace, run, monkeypatch): | ||
initial_tasks = ecs.list_tasks()["taskArns"] | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configuration that just goes on the run launcher config and not in the per-location config goes here instead:
dagster/python_modules/libraries/dagster-aws/dagster_aws/ecs/launcher.py
Lines 392 to 399 in ff96fcb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented out all instances of readonly_root_filesystem in container_context.py, moved that def to launcher.py, and changed the pytest to use:
Is this approach correct in general (i.e. is that the right way to have this test run?)? I think I will have to uncomment some instances of readonly_root_filesystem in container_context.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that looks right to me!