Skip to content

Commit

Permalink
tron: update schema for bulkdata + correctly pass extra volumes
Browse files Browse the repository at this point in the history 
We collectively missed that get_volumes() gets called on the
TronActionConfig and not the TronJobConfig. Furthermore, we missed that
we weren't actually calling get_volumes() for normal Tron jobs -
instead, we were only calling get_extra_volumes() and relying on the
Tron `default_volumes` config to add the usual default volumes (which
differs from how services work: we always call get_volumes() as we don't
distinguish between user-provided volumes and default ones when
generating the config there).

I've tested this on tron-infrastage (as well as ensured tests pass for
this commit), so this should be safe.
  • Loading branch information
@nemacysts
nemacysts committed Sep 5, 2024
1 parent c47f72b commit 15e7e69
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 32 deletions.
6 changes: 3 additions & 3 deletions paasta_tools/cli/schemas/tron_schema.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,9 @@
}
}
},
"uses_bulkdata": {
"type": "boolean"
},
"extra_volumes": {
"type": "array",
"items": {
Expand Down Expand Up @@ -660,9 +663,6 @@
},
"deploy_group": {
"type": "string"
},
"uses_bulkdata": {
"type": "boolean"
}
}
}
Expand Down
8 changes: 7 additions & 1 deletion paasta_tools/tron_tools.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1021,7 +1021,13 @@ def format_tron_action_dict(action_config: TronActionConfig):
# service account token volumes for service authentication
result["projected_sa_volumes"] = action_config.get_projected_sa_volumes()

extra_volumes = action_config.get_extra_volumes()
# XXX: now that we're actually passing through extra_volumes correctly (e.g., using get_volumes()),
# we can get rid of the default_volumes from the Tron master config
system_paasta_config = load_system_paasta_config()
extra_volumes = action_config.get_volumes(
system_paasta_config.get_volumes(),
uses_bulkdata_default=system_paasta_config.get_uses_bulkdata_default(),
)
if executor == "spark":
is_mrjob = action_config.config_dict.get("mrjob", False)
system_paasta_config = load_system_paasta_config()
Expand Down
108 changes: 80 additions & 28 deletions tests/test_tron_tools.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -943,7 +943,12 @@ def test_format_tron_action_dict_paasta(self):
}
],
"extra_volumes": [
{"containerPath": "/nail/tmp", "hostPath": "/nail/tmp", "mode": "RW"}
{
"containerPath": "/nail/bulkdata",
"hostPath": "/nail/bulkdata",
"mode": "RO",
},
{"containerPath": "/nail/tmp", "hostPath": "/nail/tmp", "mode": "RW"},
],
"trigger_downstreams": True,
"triggered_by": ["foo.bar.{shortdate}"],
Expand Down Expand Up @@ -1002,7 +1007,12 @@ def test_format_tron_action_dict_paasta(self):
}
],
"extra_volumes": [
{"container_path": "/nail/tmp", "host_path": "/nail/tmp", "mode": "RW"}
{
"container_path": "/nail/bulkdata",
"host_path": "/nail/bulkdata",
"mode": "RO",
},
{"container_path": "/nail/tmp", "host_path": "/nail/tmp", "mode": "RW"},
],
"projected_sa_volumes": [
{"audience": "foo.bar.com", "container_path": "/var/foo/bar"},
Expand Down Expand Up @@ -1086,6 +1096,11 @@ def test_format_tron_action_dict_spark(
}
],
"extra_volumes": [
{
"containerPath": "/nail/bulkdata",
"hostPath": "/nail/bulkdata",
"mode": "RO",
},
{"containerPath": "/nail/tmp", "hostPath": "/nail/tmp", "mode": "RW"},
],
"trigger_downstreams": True,
Expand All @@ -1099,18 +1114,21 @@ def test_format_tron_action_dict_spark(
"force_bounce": None,
}
mock_get_k8s_docker_volumes_conf.return_value = {
"spark.kubernetes.executor.volumes.hostPath.0.mount.path": "/nail/tmp",
"spark.kubernetes.executor.volumes.hostPath.0.options.path": "/nail/tmp",
"spark.kubernetes.executor.volumes.hostPath.0.mount.readOnly": "false",
"spark.kubernetes.executor.volumes.hostPath.1.mount.path": "/etc/pki/spark",
"spark.kubernetes.executor.volumes.hostPath.1.options.path": "/etc/pki/spark",
"spark.kubernetes.executor.volumes.hostPath.1.mount.readOnly": "true",
"spark.kubernetes.executor.volumes.hostPath.2.mount.path": "/etc/passwd",
"spark.kubernetes.executor.volumes.hostPath.2.options.path": "/etc/passwd",
"spark.kubernetes.executor.volumes.hostPath.0.mount.path": "/nail/bulkdata",
"spark.kubernetes.executor.volumes.hostPath.0.options.path": "/nail/bulkdata",
"spark.kubernetes.executor.volumes.hostPath.0.mount.readOnly": "true",
"spark.kubernetes.executor.volumes.hostPath.1.mount.path": "/nail/tmp",
"spark.kubernetes.executor.volumes.hostPath.1.options.path": "/nail/tmp",
"spark.kubernetes.executor.volumes.hostPath.1.mount.readOnly": "false",
"spark.kubernetes.executor.volumes.hostPath.2.mount.path": "/etc/pki/spark",
"spark.kubernetes.executor.volumes.hostPath.2.options.path": "/etc/pki/spark",
"spark.kubernetes.executor.volumes.hostPath.2.mount.readOnly": "true",
"spark.kubernetes.executor.volumes.hostPath.3.mount.path": "/etc/group",
"spark.kubernetes.executor.volumes.hostPath.3.options.path": "/etc/group",
"spark.kubernetes.executor.volumes.hostPath.3.mount.path": "/etc/passwd",
"spark.kubernetes.executor.volumes.hostPath.3.options.path": "/etc/passwd",
"spark.kubernetes.executor.volumes.hostPath.3.mount.readOnly": "true",
"spark.kubernetes.executor.volumes.hostPath.4.mount.path": "/etc/group",
"spark.kubernetes.executor.volumes.hostPath.4.options.path": "/etc/group",
"spark.kubernetes.executor.volumes.hostPath.4.mount.readOnly": "true",
}
mock_load_spark_srv_conf.return_value = (
{},
Expand Down Expand Up @@ -1254,18 +1272,21 @@ def test_format_tron_action_dict_spark(
"--conf spark.kubernetes.executor.label.paasta.yelp.com/pool=special_pool "
"--conf spark.kubernetes.executor.label.yelp.com/owner=core_ml "
"--conf spark.kubernetes.executor.podTemplateFile=/nail/srv/configs/spark_dns_pod_template.yaml "
"--conf spark.kubernetes.executor.volumes.hostPath.0.mount.path=/nail/tmp "
"--conf spark.kubernetes.executor.volumes.hostPath.0.options.path=/nail/tmp "
"--conf spark.kubernetes.executor.volumes.hostPath.0.mount.readOnly=false "
"--conf spark.kubernetes.executor.volumes.hostPath.1.mount.path=/etc/pki/spark "
"--conf spark.kubernetes.executor.volumes.hostPath.1.options.path=/etc/pki/spark "
"--conf spark.kubernetes.executor.volumes.hostPath.1.mount.readOnly=true "
"--conf spark.kubernetes.executor.volumes.hostPath.2.mount.path=/etc/passwd "
"--conf spark.kubernetes.executor.volumes.hostPath.2.options.path=/etc/passwd "
"--conf spark.kubernetes.executor.volumes.hostPath.0.mount.path=/nail/bulkdata "
"--conf spark.kubernetes.executor.volumes.hostPath.0.options.path=/nail/bulkdata "
"--conf spark.kubernetes.executor.volumes.hostPath.0.mount.readOnly=true "
"--conf spark.kubernetes.executor.volumes.hostPath.1.mount.path=/nail/tmp "
"--conf spark.kubernetes.executor.volumes.hostPath.1.options.path=/nail/tmp "
"--conf spark.kubernetes.executor.volumes.hostPath.1.mount.readOnly=false "
"--conf spark.kubernetes.executor.volumes.hostPath.2.mount.path=/etc/pki/spark "
"--conf spark.kubernetes.executor.volumes.hostPath.2.options.path=/etc/pki/spark "
"--conf spark.kubernetes.executor.volumes.hostPath.2.mount.readOnly=true "
"--conf spark.kubernetes.executor.volumes.hostPath.3.mount.path=/etc/group "
"--conf spark.kubernetes.executor.volumes.hostPath.3.options.path=/etc/group "
"--conf spark.kubernetes.executor.volumes.hostPath.3.mount.path=/etc/passwd "
"--conf spark.kubernetes.executor.volumes.hostPath.3.options.path=/etc/passwd "
"--conf spark.kubernetes.executor.volumes.hostPath.3.mount.readOnly=true "
"--conf spark.kubernetes.executor.volumes.hostPath.4.mount.path=/etc/group "
"--conf spark.kubernetes.executor.volumes.hostPath.4.options.path=/etc/group "
"--conf spark.kubernetes.executor.volumes.hostPath.4.mount.readOnly=true "
"--conf spark.dynamicAllocation.enabled=true "
"--conf spark.dynamicAllocation.shuffleTracking.enabled=true "
"--conf spark.dynamicAllocation.executorAllocationRatio=0.8 "
Expand Down Expand Up @@ -1357,6 +1378,11 @@ def test_format_tron_action_dict_spark(
"prometheus.io/path": "/metrics/prometheus",
},
"extra_volumes": [
{
"container_path": "/nail/bulkdata",
"host_path": "/nail/bulkdata",
"mode": "RO",
},
{"container_path": "/nail/tmp", "host_path": "/nail/tmp", "mode": "RW"},
{
"container_path": "/etc/kubernetes/spark.conf",
Expand Down Expand Up @@ -1443,7 +1469,13 @@ def test_format_tron_action_dict_paasta_k8s_service_account(self):
"secret_env": {},
"field_selector_env": {"PAASTA_POD_IP": {"field_path": "status.podIP"}},
"secret_volumes": [],
"extra_volumes": [],
"extra_volumes": [
{
"container_path": "/nail/bulkdata",
"host_path": "/nail/bulkdata",
"mode": "RO",
}
],
"service_account_name": "a-magic-sa",
}
expected_docker = "{}/{}".format(
Expand Down Expand Up @@ -1498,7 +1530,12 @@ def test_format_tron_action_dict_paasta_k8s(
}
],
"extra_volumes": [
{"containerPath": "/nail/tmp", "hostPath": "/nail/tmp", "mode": "RW"}
{
"containerPath": "/nail/bulkdata",
"hostPath": "/nail/bulkdata",
"mode": "RO",
},
{"containerPath": "/nail/tmp", "hostPath": "/nail/tmp", "mode": "RW"},
],
"trigger_downstreams": True,
"triggered_by": ["foo.bar.{shortdate}"],
Expand Down Expand Up @@ -1591,7 +1628,12 @@ def test_format_tron_action_dict_paasta_k8s(
],
"field_selector_env": {"PAASTA_POD_IP": {"field_path": "status.podIP"}},
"extra_volumes": [
{"container_path": "/nail/tmp", "host_path": "/nail/tmp", "mode": "RW"}
{
"container_path": "/nail/bulkdata",
"host_path": "/nail/bulkdata",
"mode": "RO",
},
{"container_path": "/nail/tmp", "host_path": "/nail/tmp", "mode": "RW"},
],
"trigger_downstreams": True,
"triggered_by": ["foo.bar.{shortdate}"],
Expand Down Expand Up @@ -1628,7 +1670,12 @@ def test_format_tron_action_dict_paasta_no_branch_dict(self):
}
],
"extra_volumes": [
{"containerPath": "/nail/tmp", "hostPath": "/nail/tmp", "mode": "RW"}
{
"containerPath": "/nail/bulkdata",
"hostPath": "/nail/bulkdata",
"mode": "RO",
},
{"containerPath": "/nail/tmp", "hostPath": "/nail/tmp", "mode": "RW"},
],
}
action_config = tron_tools.TronActionConfig(
Expand Down Expand Up @@ -1672,7 +1719,12 @@ def test_format_tron_action_dict_paasta_no_branch_dict(self):
}
],
"extra_volumes": [
{"container_path": "/nail/tmp", "host_path": "/nail/tmp", "mode": "RW"}
{
"container_path": "/nail/bulkdata",
"host_path": "/nail/bulkdata",
"mode": "RO",
},
{"container_path": "/nail/tmp", "host_path": "/nail/tmp", "mode": "RW"},
],
"field_selector_env": {"PAASTA_POD_IP": {"field_path": "status.podIP"}},
"node_selectors": {"yelp.com/pool": "special_pool"},
Expand Down Expand Up @@ -1830,7 +1882,7 @@ def test_create_complete_config_e2e(self, tmpdir):
# that are not static, this will cause continuous reconfiguration, which
# will add significant load to the Tron API, which happened in DAR-1461.
# but if this is intended, just change the hash.
assert hasher.hexdigest() == "211f7a9d1fdee382664ee0e25fe76e17"
assert hasher.hexdigest() == "26dae1d70ae0b937706e3de597cc07e8"

def test_override_default_pool_override(self, tmpdir):
soa_dir = tmpdir.mkdir("test_create_complete_config_soa")
Expand Down

0 comments on commit 15e7e69

Please sign in to comment.