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(install): Adding vfs install path to install parameters #58

Merged
merged 5 commits into from
Nov 7, 2023

Conversation

natmatn
Copy link
Contributor

@natmatn natmatn commented Oct 16, 2023

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

When launching the worker agent moved to systemd being spun up in the user data of the EC2 launch template, the FUS3_PATH environment variable in the profile.d file was no longer being exported. This resulted in deadline-cloud being unable to locate the VFS executable causing it to use the fallback loading method.

What was the solution? (How)

Add a --vfs-install-path argument to the install method which is then added to the env section of the systemd process definition. This will be set by the AMI builder so it can match the install location of the Fus3 package.

What is the impact of this change?

Deadline VFS install location will now be properly exported when the worker agent is started. Logging will be added to the deadline worker to make it more obvious next time the install location isn't available via the expected environment variable.
aws-deadline/deadline-cloud#75

How was this change tested?

  • hatch run test
  • Created a CMF using the deadline worker AMI, used the install script to install the deadline-worker service on said ami, launch the systemd service through the user data in Ec2 launch template user data section, and then confirmed through the cloudwatch logs that the VFS install location was found using the environment variable set in the service environment variables.

Was this change documented?

No

Is this a breaking change?

No

Signed-off-by: Nathan Matthews <natmatn@amazon.com>
@natmatn natmatn requested a review from a team as a code owner October 16, 2023 17:42
Signed-off-by: Nathan Matthews <natmatn@amazon.com>
Copy link
Contributor

@ddneilson ddneilson left a comment

Choose a reason for hiding this comment

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

I don't like the idea of having an app-specific configuration in the agent's installation; especially when that app is specific to one type of worker. Specifically, I don't like the precedent that it sets -- do we add app-specific configurations for every possible application that a job might need to interact with?

If we, instead, solve the more general problem (needing to have environment variable definitions added to the systemd script) then the installation can use the general mechanism.

What do you think about changing this from a vfs-specific feature to one where we allow the user to define a set of environment variables as part of the installation? The check for existence of the vfs binary would then be done outside of this script, and need to be done by the scripting that is calling the agent installation script prior to installing the agent.

Signed-off-by: Nathan Matthews <natmatn@amazon.com>
Copy link

@viknith viknith left a comment

Choose a reason for hiding this comment

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

Lgtm!

@natmatn natmatn merged commit 60959c0 into mainline Nov 7, 2023
9 checks passed
gmchale79 pushed a commit that referenced this pull request Feb 12, 2024
* feat(install): Adding vfs install path to install parameters

Signed-off-by: Nathan Matthews <natmatn@amazon.com>
Signed-off-by: Graeme McHale <gmchale@amazon.com>
gmchale79 pushed a commit that referenced this pull request Mar 11, 2024
* feat(install): Adding vfs install path to install parameters

Signed-off-by: Nathan Matthews <natmatn@amazon.com>
Signed-off-by: Graeme McHale <gmchale@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants