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

EC2Architect - Various small improvements #609

Merged
merged 3 commits into from
Nov 18, 2021
Merged

EC2Architect - Various small improvements #609

merged 3 commits into from
Nov 18, 2021

Conversation

JackUrb
Copy link
Contributor

@JackUrb JackUrb commented Nov 17, 2021

Overview

In more testing, ran into two primary issues:

  1. With tmux sessions sometimes the ssh-agent forwarding would become stale and then setting up the server would hang indefinitely.
  2. Determining the priority to use based on the number of rules meant that if some servers shut down (reducing the rule number) and then more were launched, two servers could attempt to be launched with the same rule priority.
  3. The subdomain string could contain special characters not allowed by amazon

Implementation

  1. Clearing SSH_AUTH_SOCK in the env overwrites use of the ssh-agent for auth (which we don't need as we're referring to the .pem keypair anyways), which should resolve tmux issues (can't directly reproduce yet).
  2. Implements a search over possible priority numbers
  3. Runs a regex replace to remove special characters from the provided subdomain
  4. Also adds an Owner tag to all resources, to make them easier to track

cc @klshuster @ytsheng

@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 Nov 17, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #609 (8aec981) into main (e70e460) will increase coverage by 0.06%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #609      +/-   ##
==========================================
+ Coverage   63.20%   63.26%   +0.06%     
==========================================
  Files          86       86              
  Lines        8221     8230       +9     
==========================================
+ Hits         5196     5207      +11     
+ Misses       3025     3023       -2     
Impacted Files Coverage Δ
...histo/abstractions/architects/ec2/ec2_architect.py 35.71% <25.00%> (-0.06%) ⬇️
...ephisto/abstractions/architects/ec2/ec2_helpers.py 17.48% <25.00%> (+0.29%) ⬆️
mephisto/operations/supervisor.py 79.65% <0.00%> (+0.64%) ⬆️
...tractions/architects/channels/websocket_channel.py 80.23% <0.00%> (+1.16%) ⬆️
mephisto/abstractions/architects/mock_architect.py 91.50% <0.00%> (+2.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e70e460...8aec981. Read the comment docs.

@JackUrb
Copy link
Contributor Author

JackUrb commented Nov 18, 2021

Merging without review after testing, to get as many people on the bugless version as possible.

@JackUrb JackUrb merged commit 46b6118 into main Nov 18, 2021
@JackUrb JackUrb deleted the ec2-fixes-2 branch November 18, 2021 17:25
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