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.

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

feat(windows-installer): add client telemetry opt out option #210

Merged
merged 2 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion src/deadline_worker_agent/installer/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def install() -> None:
farm_id=args.farm_id,
fleet_id=args.fleet_id,
region=args.region,
worker_agent_program=str(scripts_path),
worker_agent_program=scripts_path,
no_install_service=not args.install_service,
start=args.service_start,
confirm=args.confirmed,
Expand All @@ -46,6 +46,8 @@ def install() -> None:
installer_args.update(group_name=args.group)
if args.password:
installer_args.update(password=args.password)
if args.telemetry_opt_out:
installer_args.update(telemetry_opt_out=args.telemetry_opt_out)

start_windows_installer(**installer_args)
else:
Expand Down
2 changes: 1 addition & 1 deletion src/deadline_worker_agent/installer/install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ echo "Worker agent user: ${wa_user}"
echo "Worker job group: ${job_group}"
echo "Scripts path: ${scripts_path}"
echo "Worker agent program path: ${worker_agent_program}"
echo "Worker agent program path: ${client_library_program}"
echo "Deadline client program path: ${client_library_program}"
echo "Allow worker agent shutdown: ${allow_shutdown}"
echo "Start systemd service: ${start_service}"
echo "Telemetry opt-out: ${telemetry_opt_out}"
Expand Down
59 changes: 56 additions & 3 deletions src/deadline_worker_agent/installer/win_installer.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
FileSystemPermissionEnum,
)

import deadline.client.config.config_file
import pywintypes
import win32api
import win32net
Expand All @@ -31,6 +32,9 @@
DEFAULT_JOB_GROUP = "deadline-job-users"
DEFAULT_PASSWORD_LENGTH = 12

# Environment variable that overrides the config path used by the Deadline client
DEADLINE_CLIENT_CONFIG_PATH_OVERRIDE_ENV_VAR = "DEADLINE_CONFIG_FILE_PATH"


class InstallerFailedException(Exception):
"""Exception raised when the installer fails"""
Expand Down Expand Up @@ -392,11 +396,50 @@ def provision_directories(agent_username: str) -> WorkerAgentDirectories:
)


def update_deadline_client_config(
user: str,
settings: dict[str, str],
) -> None:
"""
Updates the Deadline Client config for the specified user.

Args:
user (str): The user to update the Deadline Client config for.
settings (dict[str, str]]): The key-value pairs of settings to update.

Raises:
InstallerFailedException: _description_
"""
# Build the Deadline client config path for the user
deadline_client_config_path = deadline.client.config.config_file.CONFIG_FILE_PATH
if not deadline_client_config_path.startswith("~"):
raise InstallerFailedException(
f"Cannot opt out of telemetry: Expected Deadline client config file path to start with a tilde (~), but got: {deadline_client_config_path}\n"
f"This is because the Deadline client program (version {deadline.client.version}) is not compatible with this version of the Worker agent installer\n"
f"To opt out of telemetry, please use a compatible version of the Deadline client program or run the following command as the worker user:\n\n"
"deadline config set telemetry.opt_out true\n"
)
user_deadline_client_config_path = f"~{user}" + deadline_client_config_path.removeprefix("~")

# Opt out of client telemetry for the agent user
old_environ = os.environ.copy()
try:
os.environ[DEADLINE_CLIENT_CONFIG_PATH_OVERRIDE_ENV_VAR] = user_deadline_client_config_path
for setting_key, setting_value in settings.items():
deadline.client.config.config_file.set_setting(setting_key, setting_value)
except Exception as e:
logging.error(f"Failed to update Deadline Client configuration for user '{user}': {e}")
raise
finally:
os.environ.clear()
os.environ.update(old_environ)


def start_windows_installer(
farm_id: str,
fleet_id: str,
region: str,
worker_agent_program: str,
worker_agent_program: Path,
allow_shutdown: bool,
parser: ArgumentParser,
password: typing.Optional[str] = None,
Expand All @@ -405,6 +448,7 @@ def start_windows_installer(
no_install_service: bool = False,
start: bool = False,
confirm: bool = False,
telemetry_opt_out: bool = False,
):
logging.basicConfig(level=logging.INFO, format="%(levelname)s: %(message)s")

Expand Down Expand Up @@ -441,9 +485,10 @@ def print_helping_info_and_exit():
f"Region: {region}\n"
f"Worker agent user: {user_name}\n"
f"Worker job group: {group_name}\n"
f"Worker agent program path: {worker_agent_program}\n"
f"Worker agent program path: {str(worker_agent_program)}\n"
f"Allow worker agent shutdown: {allow_shutdown}\n"
f"Start service: {start}"
f"Start service: {start}\n"
f"Telemetry opt-out: {telemetry_opt_out}"
)

# Confirm installation
Expand Down Expand Up @@ -486,3 +531,11 @@ def print_helping_info_and_exit():
if worker_user_rights:
# Grant the worker user the necessary rights
grant_account_rights(user_name, worker_user_rights)

if telemetry_opt_out:
logging.info("Opting out of client telemetry")
update_deadline_client_config(
user=user_name,
settings={"telemetry.opt_out": "true"},
)
logging.info("Opted out of client telemetry")
Comment on lines +535 to +541
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we need an else which should set telemetry.opt_out to `false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this makes sense, but this is not the same behavior as the Linux installer: https://github.com/casillas2/deadline-cloud-worker-agent/blob/58ce63d150d4df8229a7f418e8731a9f11411f23/src/deadline_worker_agent/installer/install.sh#L446-L450

Should I update the Linux installer to do the same? (If the user does not pass any --telemetry-opt-out, then we silently opt them in?)

Copy link
Contributor

@jusiskin jusiskin Mar 13, 2024

Choose a reason for hiding this comment

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

I suppose another alternative is that we:

  • add another argument --telemetry-opt-in which is mutually-exclusive with --telemetry-opt-out
  • when the installer detects a pre-existing config file, we'd not modify anything unless one of the arguments are passed

This makes re-installation behavior explicit and allows keeping the prior telemetry setting.

Maybe we should follow the Linux behavior for now and move this out-of-scope for the current PR.

21 changes: 21 additions & 0 deletions test/integ/installer/test_windows_installer.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import win32api
import win32net

import deadline.client.config.config_file
import deadline_worker_agent.installer.win_installer as installer_mod
from deadline_worker_agent.installer.win_installer import (
add_user_to_group,
Expand All @@ -20,6 +21,7 @@
ensure_local_queue_user_group_exists,
generate_password,
provision_directories,
update_deadline_client_config,
WorkerAgentDirectories,
)

Expand Down Expand Up @@ -226,3 +228,22 @@ def test_provision_directories(
assert actual_dirs.deadline_log_subdir.exists()
assert actual_dirs.deadline_persistence_subdir.exists()
assert actual_dirs.deadline_config_subdir.exists()


def test_update_deadline_client_config(tmp_path: pathlib.Path) -> None:
# GIVEN
deadline_client_config_path = tmp_path / "deadline_client_config"
deadline_client_config_path.touch(mode=0o644, exist_ok=False)

with patch(
"deadline.client.config.config_file.get_config_file_path",
return_value=deadline_client_config_path,
):
# WHEN
update_deadline_client_config(
user="", # Doesn't matter, config path is mocked out anyway
settings={"telemetry.opt_out": "true"},
)

# THEN
assert deadline.client.config.config_file.get_setting("telemetry.opt_out") == "true"
5 changes: 3 additions & 2 deletions test/unit/install/test_windows_installer.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def test_start_windows_installer(
farm_id=parsed_args.farm_id,
fleet_id=parsed_args.fleet_id,
region=parsed_args.region,
worker_agent_program=str(Path(sysconfig.get_path("scripts"))),
worker_agent_program=Path(sysconfig.get_path("scripts")),
no_install_service=not parsed_args.install_service,
start=parsed_args.service_start,
confirm=parsed_args.confirmed,
Expand All @@ -52,6 +52,7 @@ def test_start_windows_installer(
group_name=parsed_args.group,
password=parsed_args.password,
allow_shutdown=parsed_args.allow_shutdown,
telemetry_opt_out=parsed_args.telemetry_opt_out,
)


Expand All @@ -69,7 +70,7 @@ def test_start_windows_installer_fails_when_run_as_non_admin_user(
farm_id=parsed_args.farm_id,
fleet_id=parsed_args.fleet_id,
region=parsed_args.region,
worker_agent_program=str(Path(sysconfig.get_path("scripts"))),
worker_agent_program=Path(sysconfig.get_path("scripts")),
no_install_service=not parsed_args.install_service,
start=parsed_args.service_start,
confirm=parsed_args.confirmed,
Expand Down