From 15e7e69b8f3d1a8e54bd3132ada99bb2474f9e7a Mon Sep 17 00:00:00 2001 From: Luis Perez Date: Thu, 5 Sep 2024 11:19:31 -0700 Subject: [PATCH] tron: update schema for bulkdata + correctly pass extra volumes 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. --- paasta_tools/cli/schemas/tron_schema.json | 6 +- paasta_tools/tron_tools.py | 8 +- tests/test_tron_tools.py | 108 ++++++++++++++++------ 3 files changed, 90 insertions(+), 32 deletions(-) diff --git a/paasta_tools/cli/schemas/tron_schema.json b/paasta_tools/cli/schemas/tron_schema.json index 1168eedb19..5c56833aea 100644 --- a/paasta_tools/cli/schemas/tron_schema.json +++ b/paasta_tools/cli/schemas/tron_schema.json @@ -231,6 +231,9 @@ } } }, + "uses_bulkdata": { + "type": "boolean" + }, "extra_volumes": { "type": "array", "items": { @@ -660,9 +663,6 @@ }, "deploy_group": { "type": "string" - }, - "uses_bulkdata": { - "type": "boolean" } } } diff --git a/paasta_tools/tron_tools.py b/paasta_tools/tron_tools.py index 4c47eedc12..13163a345a 100644 --- a/paasta_tools/tron_tools.py +++ b/paasta_tools/tron_tools.py @@ -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() diff --git a/tests/test_tron_tools.py b/tests/test_tron_tools.py index f5d761e630..a53c605024 100644 --- a/tests/test_tron_tools.py +++ b/tests/test_tron_tools.py @@ -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}"], @@ -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"}, @@ -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, @@ -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 = ( {}, @@ -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 " @@ -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", @@ -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( @@ -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}"], @@ -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}"], @@ -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( @@ -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"}, @@ -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")