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.

Sign up for GitHub

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

Reduce job history dir length #561

Open
wants to merge 2 commits into
base: mainline
Choose a base branch
from
Open

Reduce job history dir length #561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e1507b
fix: truncate job history dir path length
joel-wong-aws Jan 17, 2025
62d42bf
fix: add handling for 0 or negative job name calculations in job hist…
joel-wong-aws Jan 23, 2025
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
29 changes: 22 additions & 7 deletions src/deadline/client/job_bundle/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import datetime
import glob
import os
import sys

from ..config import get_setting
from ._yaml import deadline_yaml_dump
Expand All @@ -34,10 +35,6 @@ def create_job_history_bundle_dir(submitter_name: str, job_name: str) -> str:
char for char in submitter_name if char.isalnum() or char in " -_"
)

# Clean the job_name's characters and truncate for the filename
job_name_cleaned = "".join(char for char in job_name if char.isalnum() or char in " -_")
job_name_cleaned = job_name_cleaned[:128]

timestamp = datetime.datetime.now()
month_tag = timestamp.strftime("%Y-%m")
date_tag = timestamp.strftime("%Y-%m-%d")
Expand All @@ -53,8 +50,26 @@ def create_job_history_bundle_dir(submitter_name: str, job_name: str) -> str:
latest_dir = existing_dirs[-1]
number = int(os.path.basename(latest_dir)[len(date_tag) + 1 :].split("-", 1)[0]) + 1

result = os.path.join(
month_dir, f"{date_tag}-{number:02}-{submitter_name_cleaned}-{job_name_cleaned}"
)
job_dir_prefix = f"{date_tag}-{number:02}-{submitter_name_cleaned}-"

max_job_name_prefix = 128 # max job name from OpenJD spec

# max path length - manifest file name
# 256 - len("\manifests\d2b2c3102af5a862db950a2e30255429_input")
# = 207
Comment on lines +58 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: MAX_PATH is actually 260: https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry

D:\some 256-character path string<NUL>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the official MAX_PATH is 260, there may be some DCC constraints that limit this to 256 characters, so I decided to go with that. This also gives us a few characters of wiggle room.

if sys.platform in ["win32", "cygwin"]:
max_job_name_prefix = min(
207 - len(os.path.abspath(os.path.join(month_dir, job_dir_prefix))), max_job_name_prefix
)
Comment on lines +61 to +63
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we considered what happens if this ends up being 0 or a negative number?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added error handling for this case!

if max_job_name_prefix < 1:
raise RuntimeError(
"Job history directory is too long. Please update your 'settings.job_history_dir' to a shorter path."
)
Comment on lines +64 to +67
Copy link
Contributor

@epmog epmog Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, unfortunately due to the customer experience impact we can't do this change.

A valid windows submission that works with long paths will now fail to submit because we won't create the job history dir (to be used as input to the create job bundle callback for where to create the submission files).

In my mind, there's 2 part of this at play:

  1. job bundle history directory right now is a required portion of submission or export
    • in my opinion, it is an internal implementation detail that we must create a history entry to submit, rather than populate it after submission with best effort.
  2. using the library in a runtime that doesn't support long paths will have submission issues based off the job bundle directory location + job name length

I think short-term we can make the second issue less likely and do the truncation but only if it would fail otherwise. We can't have previously working setups failing due to this. If we can successfully create the history directory and a file whose path matches the longest path therein, then no truncation should happen.

Longer-term I think we should absolutely remove the truncation and:

  1. use a local temp folder to allow submission and then move it to the history folder after submission, and/or
  2. leverage \\?\ for this specific case of the internal implementation detail of having a history directory and how we populate it.

Let me know your thoughts


# Clean the job_name's characters and truncate for the filename
job_name_cleaned = "".join(char for char in job_name if char.isalnum() or char in " -_")
job_name_cleaned = job_name_cleaned[:max_job_name_prefix]

result = os.path.join(month_dir, f"{job_dir_prefix}{job_name_cleaned}")
joel-wong-aws marked this conversation as resolved.
Show resolved Hide resolved
os.makedirs(result)
return result
3 changes: 2 additions & 1 deletion src/deadline/client/ui/dialogs/submit_job_to_deadline_dialog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,8 @@ def on_submit(self):
deadline = api.get_boto3_client("deadline")

self.job_history_bundle_dir = create_job_history_bundle_dir(
self.submitter_name, settings.name
self.submitter_name,
settings.name,
)

if self.show_host_requirements_tab:
Expand Down
102 changes: 102 additions & 0 deletions test/unit/deadline_client/job_bundle/test_job_history_folders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"""

import os
import sys
import tempfile
import pytest

Expand Down Expand Up @@ -55,6 +56,107 @@ def test_create_job_bundle_dir(fresh_deadline_config):
assert sorted(os.listdir(os.path.join(tmpdir, "2023-04"))) == EXPECTED_DIRS[3:]


def test_job_history_dir_truncation(fresh_deadline_config):
# Use a temporary directory for the job history
with tempfile.TemporaryDirectory() as tmpdir, freeze_time("2024-12-27T12:34:56"):
tmpdir_path_length = len(os.path.abspath(tmpdir))
long_tmpdir = os.path.join(
tmpdir,
"dir" + "1" * (140 - tmpdir_path_length),
)
config.set_setting("settings.job_history_dir", long_tmpdir)

job_name_127_chars = "job1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234"
output_path = job_bundle.create_job_history_bundle_dir(
"MySubmitter",
job_name_127_chars,
)

if sys.platform in ["win32", "cygwin"]:
assert len(os.path.abspath(output_path)) == 207
assert str(os.path.abspath(output_path)).startswith(
str(
os.path.abspath(
os.path.join(
long_tmpdir, "2024-12", "2024-12-27-01-MySubmitter-job1234567890"
)
)
)
)
assert (
len(
os.path.abspath(
os.path.join(
output_path, "manifests", "d2b2c3102af5a862db950a2e30255429_input"
)
)
)
== 256
)
else:
assert output_path == os.path.join(
long_tmpdir, "2024-12", f"2024-12-27-01-MySubmitter-{job_name_127_chars}"
)
assert len(output_path) > 256
assert os.path.isdir(output_path)


def test_job_history_dir_truncation_from_job_name_with_129_chars(fresh_deadline_config):
# Use a temporary directory for the job history
if sys.platform in ["win32", "cygwin"]:
# Windows has a long tmp dir path so it will truncate anyway - we only want to test job name truncation
tmp_parent_dir = "C:\\ProgramData"
else:
tmp_parent_dir = None
with tempfile.TemporaryDirectory(dir=tmp_parent_dir) as tmpdir, freeze_time(
"2024-08-26T18:42:05"
):
config.set_setting("settings.job_history_dir", tmpdir)

job_name_129_chars = "test12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345"
output_path = job_bundle.create_job_history_bundle_dir(
"SubmitterFour",
job_name_129_chars,
)
assert output_path == os.path.join(
tmpdir,
"2024-08",
"2024-08-26-01-SubmitterFour-test1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234",
)
assert os.path.isdir(output_path)


def test_job_history_dir_exceeds_256_characters(fresh_deadline_config):
# Use a temporary directory for the job history
with tempfile.TemporaryDirectory() as tmpdir, freeze_time("2023-11-12T13:14:15"):
tmpdir_path_length = len(os.path.abspath(tmpdir))
long_tmpdir = os.path.join(
tmpdir,
"dir" + "1" * (256 - tmpdir_path_length),
)
config.set_setting("settings.job_history_dir", long_tmpdir)

job_name_1_char = "a"

if sys.platform in ["win32", "cygwin"]:
with pytest.raises(RuntimeError, match="Job history directory is too long"):
job_bundle.create_job_history_bundle_dir(
"SubmitterFive",
job_name_1_char,
)
else:
output_path = job_bundle.create_job_history_bundle_dir(
"SubmitterFive",
job_name_1_char,
)
assert output_path == os.path.join(
long_tmpdir,
"2023-11",
"2023-11-12-01-SubmitterFive-a",
)
assert os.path.isdir(output_path)


@pytest.mark.parametrize(
"submitter_name, job_name, freeze_date, expected_output_path",
[
Expand Down
Loading