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

New testing option for ec2 (reuse existing server) #1036

Merged
merged 1 commit into from
Jul 27, 2023
Merged

Conversation

JackUrb
Copy link
Contributor

@JackUrb JackUrb commented Jul 27, 2023

Overview

For some reason, redeploying servers to EC2 has become increasingly painful due to delays downloading the required resources. This option allows reuse of an existing server, with the following workflow:

Deploy

$ python run_task.py mephisto.architect._deploy_type=retain mephisto.architect.subdomain=retain-test +profile=sandbox

Reuse

$ python run_task.py mephisto.architect._deploy_type=use_existing mephisto.architect.subdomain=retain-test +profile=sandbox

Cleanup

$ python ../../mephisto/abstractions/architects/ec2/cleanup_ec2_server_by_name.py 
Please enter server name you want to clean up (existing servers: ['retain-test'])
>> retain-test

note: No new files are pushed to the server, so no actual changes to tasks will be reflected with this option currently. @meta-paul this may be something worth expanding on.

note 2: Only one Mephisto task can be actively serviced by a routing server, so this should not be used to connect to servers that are in-use.

@JackUrb JackUrb requested a review from meta-paul July 27, 2023 18:37
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 27, 2023
@meta-paul
Copy link
Contributor

For the sample commands (e.g. python ../../mephisto/abstractions/architects/ec2/cleanup_ec2_server_by_name.py) I'd make the paths such that they can be run from the root of the code repo.

@JackUrb
Copy link
Contributor Author

JackUrb commented Jul 27, 2023

For the sample commands (e.g. python ../../mephisto/abstractions/architects/ec2/cleanup_ec2_server_by_name.py) I'd make the paths such that they can be run from the root of the code repo.

In this case I ran the above from a sample folder. True proper documentation should note the frame of reference, and when the feature is documented that would be good.

@meta-paul
Copy link
Contributor

note: No new files are pushed to the server - this narrows the scope of reuse, during debugging usually something is changed (code files or YAML config).

Since the main bottleneck is assembly of NodeJS part (while Python part sails through quickly), in the ideal world we'd cache the NodeJS build, while allowing to alter Python and/or YAML files. Not sure if this piecemeal caching is possible here.

print(f"Ec2: Deleting server: {self.server_id}")
if self.router_rule_arn is not None:
ec2_helpers.delete_rule(
if self._deploy_type == "standard":
Copy link
Contributor

Choose a reason for hiding this comment

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

if deploy type is different, I suggest to output an INFO message here saying that for deploy type X the server continue to live, and needs to be shut down manually via cleanup command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning message appears on launch for the retain server currently. Feel free to add additional logging elsewhere as appropriate.

self.fallback_details["key_pair_name"],
server_dir,
)
print("EC2: Starting instance...")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could include here what deploy type this is

@JackUrb
Copy link
Contributor Author

JackUrb commented Jul 27, 2023

Since the main bottleneck is assembly of NodeJS part (while Python part sails through quickly), in the ideal world we'd cache the NodeJS build, while allowing to alter Python and/or YAML files. Not sure if this piecemeal caching is possible here.

Indeed - this I'd consider to be future work, requiring splitting the setup part of the server from the package/deploy part, and only running the setup part on initial launch, but instead running a content wipe for the latter.

@JackUrb JackUrb merged commit eca15cd into main Jul 27, 2023
13 checks passed
@JackUrb JackUrb deleted the use-existing-ec2 branch July 27, 2023 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants