feat: add os_user and os_env_vars for use by deadline vfs #104

natmatn · 2023-11-10T00:26:05Z

What was the problem/requirement? (What/Why)

Queue credentials for the working session are stored and accessed in the deadline-cloud-worker-agent using the os_user's aws config file. This is used to create a boto3 session that deadline-cloud uses to sync inputs in the Copied method. This session can't be passed to the deadline_vfs subprocess so another path is needed to retrieve them.

What was the solution? (How)

Add os_user to the FileSystemPermissionSettings and an os_env_vars argument to AssetSync.sync_inputs(...) which can be used to configure the AWS config file path and AWS profile that is setup by the worker agent. If these details don't exist, the COPIED fallback is used in place of the VIRTUAL input sync method.

What is the impact of this change?

The Deadline worker agent will be able to configure the Deadline VFS to use the AWS credentials it vends for the queue and sync inputs for the job.

How was this change tested?

hatch run test
ran this on a worker and successfully completed both VIRTUAL and COPIED jobs

Was this change documented?

No

Is this a breaking change?

Yes, adding the new os_user required field requires a code change on the worker agent side. I've got another PR in the works to have the deadline-cloud-worker-agent provide this information from the session to thesync_inputs method

src/deadline/job_attachments/fus3.py

viknith · 2023-11-13T20:53:21Z

src/deadline/job_attachments/fus3.py

@@ -327,6 +332,9 @@ def get_launch_environ(cls) -> dict:
        ] = f"{Fus3ProcessManager.find_fus3_link_dir()}{os.pathsep}{os.environ['PATH']}"
        my_env["LD_LIBRARY_PATH"] = Fus3ProcessManager.get_library_path()  # type: ignore[assignment]

+        my_env["AWS_CONFIG_FILE"] = str(Path(f"~{self._os_user}/.aws/config").expanduser())


Shouldn't the path be f"~/{self._os_user}/.aws/config" with a slash after the tilde? Or does the _os_user variable come with a slash prepended?

Neither, the expanduser method returns a new path with both ~ and ~user constructs expanded, so it provides the first slash

Shouldn't the caller control this through the os_env_vars argument?

src/deadline/job_attachments/fus3.py

deadline vfs Signed-off-by: Nathan Matthews <[email protected]>

Signed-off-by: Nathan Matthews <[email protected]>

containing the AWS_PROFILE for use by the VFS Signed-off-by: Nathan Matthews <[email protected]>

mwiebe · 2023-11-14T01:52:49Z

src/deadline/job_attachments/asset_sync.py

@@ -400,6 +402,9 @@ def sync_inputs(
        if (
            attachments.fileSystem == JobAttachmentsFileSystem.VIRTUAL.value
            and sys.platform != "win32"
+            and fs_permission_settings is not None


Question - does this mean if a user turns off the jobRunAsUser feature, the VFS will stop working?

Is it possible to turn off that feature? I thought it was a required field of the create-queue request

It is required, but users can opt-out by entering blank values for the jobRunAsUser → user and jobRunAsUser → group.

Ah, you're right. Those fs_permission_settings are still using job-user when those fields are empty, which explains why my testing didn't encounter an issue here.

Discussed offline and we will address this in a follow-up fix.

src/deadline/job_attachments/fus3.py

jusiskin · 2023-11-14T19:26:52Z

src/deadline/job_attachments/fus3.py

@@ -327,6 +332,9 @@ def get_launch_environ(cls) -> dict:
        ] = f"{Fus3ProcessManager.find_fus3_link_dir()}{os.pathsep}{os.environ['PATH']}"
        my_env["LD_LIBRARY_PATH"] = Fus3ProcessManager.get_library_path()  # type: ignore[assignment]

+        my_env["AWS_CONFIG_FILE"] = str(Path(f"~{self._os_user}/.aws/config").expanduser())


Shouldn't the caller control this through the os_env_vars argument?

jusiskin · 2023-11-14T19:28:39Z

src/deadline/job_attachments/models.py

        os_group (str): The target operating system group for ownership.
        dir_mode (int): The permission mode to be added to directories.
        file_mode (int): The permission mode to be added to files.
    """

+    os_user: str


Do we still need this change? I believe it will have no effect if we are going with the solution to have the caller pass environment variables.

The environment variables being passed only contain the AWS_PROFILE so I still need to get the Config file path on the other side

We discussed offline. While I maintain that a cleaner interface would be to have the worker agent pass AWS_CONFIG_FILE in the os_env_vars argument, it requires greater effort on the worker agent side and the code architecture there is not very conducive to this solution.

I'd like to revisit this later, but for now I'm okay merging this due to time constraints.

natmatn requested a review from a team as a code owner November 10, 2023 00:26

jusiskin requested changes Nov 10, 2023

View reviewed changes

src/deadline/job_attachments/fus3.py Outdated Show resolved Hide resolved

src/deadline/job_attachments/fus3.py Outdated Show resolved Hide resolved

natmatn mentioned this pull request Nov 10, 2023

fix: Adding os_user to FileSystemPermissionSettings for use in the deadline vfs aws-deadline/deadline-cloud-worker-agent#94

Merged

viknith reviewed Nov 13, 2023

View reviewed changes

viknith approved these changes Nov 13, 2023

View reviewed changes

mwiebe requested changes Nov 13, 2023

View reviewed changes

src/deadline/job_attachments/fus3.py Outdated Show resolved Hide resolved

natmatn added 4 commits November 14, 2023 01:50

fix: Adding os_user to FileSystemPermissionSettings for use by the

e63bd00

deadline vfs Signed-off-by: Nathan Matthews <[email protected]>

Removing posix

aad128c

Signed-off-by: Nathan Matthews <[email protected]>

Formatting issue

594b682

Signed-off-by: Nathan Matthews <[email protected]>

Sync inputs now takes an environment variable from the worker agent

b8772d0

containing the AWS_PROFILE for use by the VFS Signed-off-by: Nathan Matthews <[email protected]>

natmatn force-pushed the natmatn/deadline_vfs_user branch from ecff747 to b8772d0 Compare November 14, 2023 01:50

mwiebe reviewed Nov 14, 2023

View reviewed changes

mwiebe approved these changes Nov 14, 2023

View reviewed changes

Merge branch 'mainline' into natmatn/deadline_vfs_user

6758ae0

jusiskin requested changes Nov 14, 2023

View reviewed changes

jusiskin changed the title ~~fix: Adding os_user to FileSystemPermissionSettings for use by the deadline vfs~~ feat: add os_user and os_env_vars for use by deadline vfs Nov 14, 2023

jusiskin added the enhancement New feature or request label Nov 14, 2023

jusiskin approved these changes Nov 14, 2023

View reviewed changes

jusiskin merged commit 7b3b3e6 into mainline Nov 14, 2023
17 checks passed

jusiskin deleted the natmatn/deadline_vfs_user branch November 14, 2023 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add os_user and os_env_vars for use by deadline vfs #104

feat: add os_user and os_env_vars for use by deadline vfs #104

natmatn commented Nov 10, 2023 •

edited by jusiskin

Loading

viknith Nov 13, 2023 •

edited

Loading

natmatn Nov 13, 2023

jusiskin Nov 14, 2023

mwiebe Nov 14, 2023 •

edited

Loading

natmatn Nov 14, 2023

jusiskin Nov 14, 2023

natmatn Nov 14, 2023

jusiskin Nov 14, 2023

jusiskin Nov 14, 2023

jusiskin Nov 14, 2023

natmatn Nov 14, 2023

jusiskin Nov 14, 2023 •

edited

Loading

feat: add os_user and os_env_vars for use by deadline vfs #104

feat: add os_user and os_env_vars for use by deadline vfs #104

Conversation

natmatn commented Nov 10, 2023 • edited by jusiskin Loading

What was the problem/requirement? (What/Why)

What was the solution? (How)

What is the impact of this change?

How was this change tested?

Was this change documented?

Is this a breaking change?

viknith Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwiebe Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jusiskin Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

natmatn commented Nov 10, 2023 •

edited by jusiskin

Loading

viknith Nov 13, 2023 •

edited

Loading

mwiebe Nov 14, 2023 •

edited

Loading

jusiskin Nov 14, 2023 •

edited

Loading