feat: Basic Windows support #35

gmchale79 · 2023-09-18T22:01:38Z

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

The DLC worker agent did not start on Windows, and numerous unit tests did not pass.

What was the solution? (How)

Numerous small changes related to removing NotImplemented exceptions, using more appropriate file paths etc.

User impersonation is not intended to work in this branch at this time, and several of the changes relate to ensuring tests pass when impersonation is not used.

As of 11/6/23 this is targeted at a feature branch, not mainline.

What is the impact of this change?

The worker agent starts, then receives and executes jobs on Windows.

How was this change tested?

Submitting jobs to gamma and ensuring they are picked up and executed by a Windows worker.

Was this change documented?

No currently.

Is this a breaking change?

It is not intended to be.

jusiskin

Thank you for taking on the first pass of adding Windows support to the worker agent. This is a great first step towards an important feature. It seems like this is a partial implementation, which is fine.

The biggest blocker to shipping this code is setting expectations for those using it. Specifically, if we don't yet support isolation of OS users then that limitation and its associated risks need to be made clear

EDIT: Sorry, I missed that this was targeting the feature_windows branch. I presume that we will have OS user impersonation when we merge this feature branch into mainline. On that basis, we can disregard my concern above.

src/deadline_worker_agent/aws_credentials/queue_boto3_session.py

src/deadline_worker_agent/aws_credentials/aws_configs.py

src/deadline_worker_agent/sessions/job_entities/job_details.py

src/deadline_worker_agent/startup/config_file.py

src/deadline_worker_agent/startup/entrypoint.py

test/unit/conftest.py

test/unit/scheduler/test_session_cleanup.py

test/unit/startup/test_config.py

test/unit/test_worker.py

src/deadline_worker_agent/aws_credentials/aws_configs.py

src/deadline_worker_agent/aws_credentials/queue_boto3_session.py

src/deadline_worker_agent/scheduler/scheduler.py

src/deadline_worker_agent/sessions/job_entities/job_details.py

src/deadline_worker_agent/startup/config_file.py

src/deadline_worker_agent/startup/settings.py

src/deadline_worker_agent/worker.py

test/unit/aws_credentials/test_aws_configs.py

pyproject.toml

src/deadline_worker_agent/sessions/job_entities/job_details.py

src/deadline_worker_agent/installer/worker.toml.windows.example

test/unit/sessions/test_session.py

src/deadline_worker_agent/installer/worker.toml.windows.example

src/deadline_worker_agent/installer/install.ps1

src/deadline_worker_agent/aws_credentials/queue_boto3_session.py

src/deadline_worker_agent/set_windows_permissions.py

src/deadline_worker_agent/worker.py

test/unit/aws_credentials/test_aws_configs.py

jusiskin · 2023-10-03T21:39:18Z

test/unit/startup/test_config.py

@@ -821,9 +853,10 @@ def test_uses_worker_settings(
        assert config.capabilities is mock_worker_settings.capabilities
        assert config.impersonation.inactive == (not mock_worker_settings.impersonation)
        if expected_config_posix_job_user:
+            posix_user: PosixSessionUser = request.getfixturevalue(expected_config_posix_job_user)


I was somewhat confused by this change. Could we instead switch to obtaining the os_user fixture from normally (as an argument to the test method) and remove the usage of expected_config_posix_job_user entirely?

This is the only approach I found that worked for using a fixture in a parameterize call, but I'm pretty much a python novice so I'll keep experimenting for a little longer.

Because it's a parameterized test, I can't find a cleaner way to get at the fixture as of now. I have added a TODO to revisit whether this can be simplified when we add impersonation on Windows (which is in progress right now).

Did it not work to add the os_user fixture as an argument to the test function along-side the parameterized arguments?

test/unit/conftest.py

test/unit/scheduler/test_scheduler.py

test/unit/scheduler/test_session_cleanup.py

test/unit/test_set_windows_permissions.py

moorec-aws · 2023-10-12T17:46:10Z

src/deadline_worker_agent/aws_credentials/aws_configs.py

@@ -28,8 +29,12 @@ def _run_cmd_as(*, user: PosixSessionUser, cmd: list[str]) -> None:

 def _setup_parent_dir(*, dir_path: Path, owner: SessionUser | None = None) -> None:
    if owner is None:
-        create_perms: int = stat.S_IRWXU
-        dir_path.mkdir(mode=create_perms, exist_ok=True)
+        if os.name == "posix":


should there be a try catch here for the possible CalledProcessError?

Yes, thanks.

src/deadline_worker_agent/aws_credentials/queue_boto3_session.py

src/deadline_worker_agent/set_windows_permissions.py

src/deadline_worker_agent/startup/entrypoint.py

mwiebe · 2023-10-12T17:42:12Z

src/deadline_worker_agent/startup/config_file.py

@@ -20,6 +21,7 @@
 DEFAULT_CONFIG_PATH: dict[str, Path] = {
    "darwin": Path("/etc/amazon/deadline/worker.toml"),
    "linux": Path("/etc/amazon/deadline/worker.toml"),
+    "win32": Path(os.path.expandvars(r"%PROGRAMDATA%/Amazon/Deadline/Config/worker.toml")),


Did you look at how to avoid relying on an environment variable for this path? A brief look suggests SHGetFolderPath is what to use, but I don't see PROGRAMDATA in the enum list.

Because users can change the environment variables, but not the system API calls. Reducing reliance on env variables makes things more robust.

Good point! :)

But without meaning to be overly argumentative, isn't there a counter argument that a user may change an env var for good reason and would want our code to respect their change?

src/deadline_worker_agent/worker.py

The base branch was changed.

Signed-off-by: Graeme McHale <[email protected]>

mwiebe · 2023-11-06T19:31:42Z

src/deadline_worker_agent/set_windows_permissions.py

+
+    subprocess.run(
+        [
+            "icacls",


In the job attachments code, they were able to remove the subprocess calls to icacls by using pywin32. IMO we should do the same thing here.

Yes, I agree. Consistency would be nice, as would skipping the overhead of using subprocess.

For the record, the reason that I went with this approach in the first place is that I figured Windows IT Admin curious about our code what find it easier to interpret icacls calls and may be unfamiliar with pywin32.

mwiebe · 2023-11-06T19:32:16Z

src/deadline_worker_agent/startup/config_file.py

@@ -20,6 +21,7 @@
 DEFAULT_CONFIG_PATH: dict[str, Path] = {
    "darwin": Path("/etc/amazon/deadline/worker.toml"),
    "linux": Path("/etc/amazon/deadline/worker.toml"),
+    "win32": Path(os.path.expandvars(r"%PROGRAMDATA%/Amazon/Deadline/Config/worker.toml")),


Because users can change the environment variables, but not the system API calls. Reducing reliance on env variables makes things more robust.

jusiskin

This looks good to me.

A few small remaining nit-picks but they are not critical for merging

jusiskin · 2023-10-23T15:07:06Z

test/unit/aws_credentials/test_aws_configs.py

+    @pytest.fixture
+    def set_windows_permissions(self) -> MagicMock:
+        return MagicMock()


This fixture does not appear to be used. It looks like you may have intended to use this on lines 63-68, but ended up using a local mock instead.

src/deadline_worker_agent/aws_credentials/queue_boto3_session.py

jusiskin · 2023-10-23T15:28:15Z

test/unit/startup/test_config.py

@@ -821,9 +853,10 @@ def test_uses_worker_settings(
        assert config.capabilities is mock_worker_settings.capabilities
        assert config.impersonation.inactive == (not mock_worker_settings.impersonation)
        if expected_config_posix_job_user:
+            posix_user: PosixSessionUser = request.getfixturevalue(expected_config_posix_job_user)


Did it not work to add the os_user fixture as an argument to the test function along-side the parameterized arguments?

jusiskin · 2023-11-08T19:50:57Z

test/unit/test_set_windows_permissions.py

+from deadline_worker_agent.set_windows_permissions import set_user_restricted_path_permissions
+
+
+class TestSetWindowsPermissions(unittest.TestCase):


nit: Since we use pytest, we don't need to derive from unittest.TestCase, although pytest allows this for compatibility to make migrating to pytest easier.

Suggested change

class TestSetWindowsPermissions(unittest.TestCase):

class TestSetWindowsPermissions:

Signed-off-by: Josh Usiskin <[email protected]>

gmchale79 assigned ddneilson, gmchale79 and jusiskin Sep 18, 2023

jusiskin added the enhancement New feature or request label Sep 18, 2023

ddneilson requested review from ddneilson and jusiskin September 19, 2023 14:03

jusiskin requested changes Sep 19, 2023

View reviewed changes

ddneilson suggested changes Sep 19, 2023

View reviewed changes

gmchale79 force-pushed the gmchale79/windows_dev branch from b0b3bad to 815735f Compare September 20, 2023 23:26

gmchale79 commented Sep 20, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

gmchale79 commented Sep 20, 2023

View reviewed changes

src/deadline_worker_agent/sessions/job_entities/job_details.py Outdated Show resolved Hide resolved

ddneilson reviewed Sep 25, 2023

View reviewed changes

src/deadline_worker_agent/installer/worker.toml.windows.example Outdated Show resolved Hide resolved

gmchale79 commented Sep 25, 2023

View reviewed changes

test/unit/sessions/test_session.py Outdated Show resolved Hide resolved

src/deadline_worker_agent/installer/worker.toml.windows.example Outdated Show resolved Hide resolved

gmchale79 force-pushed the gmchale79/windows_dev branch 2 times, most recently from f4a4ae4 to e5862ae Compare September 27, 2023 22:37

gmchale79 changed the base branch from feature_windows to mainline September 27, 2023 22:40

gmchale79 marked this pull request as ready for review September 27, 2023 22:41

gmchale79 requested a review from a team as a code owner September 27, 2023 22:41

gmchale79 changed the title ~~wip: First pass Windows support~~ feat: Basic Windows support Sep 27, 2023

gmchale79 force-pushed the gmchale79/windows_dev branch 2 times, most recently from 2316e7a to e6a880f Compare September 28, 2023 23:34

gmchale79 requested review from ddneilson and jusiskin September 28, 2023 23:46

ddneilson reviewed Sep 29, 2023

View reviewed changes

src/deadline_worker_agent/installer/install.ps1 Outdated Show resolved Hide resolved

src/deadline_worker_agent/installer/install.ps1 Outdated Show resolved Hide resolved

ddneilson reviewed Sep 29, 2023

View reviewed changes

src/deadline_worker_agent/installer/install.ps1 Outdated Show resolved Hide resolved

gmchale79 force-pushed the gmchale79/windows_dev branch 2 times, most recently from 771280c to 7bf56bf Compare October 2, 2023 18:49

gmchale79 requested a review from ddneilson October 2, 2023 19:02

ddneilson approved these changes Oct 2, 2023

View reviewed changes

src/deadline_worker_agent/aws_credentials/queue_boto3_session.py Show resolved Hide resolved

jusiskin requested changes Oct 4, 2023

View reviewed changes

gmchale79 force-pushed the gmchale79/windows_dev branch 3 times, most recently from 7fc8386 to f67f6ae Compare October 11, 2023 21:19

gmchale79 requested a review from jusiskin October 11, 2023 21:35

ddneilson previously approved these changes Oct 12, 2023

View reviewed changes

moorec-aws reviewed Oct 12, 2023

View reviewed changes

mwiebe requested changes Oct 12, 2023

View reviewed changes

gmchale79 force-pushed the gmchale79/windows_dev branch from ade5a04 to cf7bc6f Compare October 19, 2023 00:01

gmchale79 mentioned this pull request Oct 19, 2023

test: Add github actions for Windows #62

Closed

gmchale79 requested review from ddneilson and mwiebe October 19, 2023 23:48

gmchale79 force-pushed the gmchale79/windows_dev branch from cf7bc6f to fc3d58f Compare October 21, 2023 22:40

gmchale79 changed the base branch from mainline to feature_windows November 2, 2023 17:58

feat: basic Windows support

f4b0db6

Signed-off-by: Graeme McHale <[email protected]>

gmchale79 force-pushed the gmchale79/windows_dev branch from fc3d58f to f4b0db6 Compare November 6, 2023 17:35

gmchale79 requested review from Honglichenn, anandgo1 and mwiebe and removed request for mwiebe November 6, 2023 17:40

mwiebe reviewed Nov 6, 2023

View reviewed changes

mwiebe approved these changes Nov 6, 2023

View reviewed changes

anandgo1 approved these changes Nov 6, 2023

View reviewed changes

jusiskin approved these changes Nov 8, 2023

View reviewed changes

gmchale79 merged commit a59e2b9 into feature_windows Nov 8, 2023
2 checks passed

ddneilson added the security Pull requests that could impact security label Jan 30, 2024

jusiskin added a commit to jusiskin/deadline-cloud-worker-agent that referenced this pull request Sep 4, 2024

fix!: CreateFleet API changes and robust cleanup (aws-deadline#35)

2ba8987

Signed-off-by: Josh Usiskin <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Basic Windows support #35

feat: Basic Windows support #35

gmchale79 commented Sep 18, 2023 •

edited

Loading

jusiskin left a comment •

edited

Loading

jusiskin Oct 3, 2023

gmchale79 Oct 5, 2023

gmchale79 Oct 11, 2023

jusiskin Oct 23, 2023

moorec-aws Oct 12, 2023

gmchale79 Oct 12, 2023

mwiebe Oct 12, 2023

gmchale79 Oct 12, 2023

mwiebe Nov 6, 2023

gmchale79 Nov 6, 2023

mwiebe Nov 6, 2023

gmchale79 Nov 6, 2023

mwiebe Nov 6, 2023

jusiskin left a comment

jusiskin Oct 23, 2023

jusiskin Oct 23, 2023

jusiskin Nov 8, 2023

		from deadline_worker_agent.set_windows_permissions import set_user_restricted_path_permissions


		class TestSetWindowsPermissions(unittest.TestCase):

	class TestSetWindowsPermissions(unittest.TestCase):
	class TestSetWindowsPermissions:

feat: Basic Windows support #35

feat: Basic Windows support #35

Conversation

gmchale79 commented Sep 18, 2023 • edited 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?

jusiskin left a comment • 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

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 left a 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

gmchale79 commented Sep 18, 2023 •

edited

Loading

jusiskin left a comment •

edited

Loading