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

BFD-3126: Pipeline manages its own scale-in #2518

Open
wants to merge 43 commits into
base: master
Choose a base branch
from
Open

Conversation

aschey-forpeople
Copy link
Contributor

JIRA Ticket:
BFD-3126

What Does This PR Do?

  • Updates the CCW pipeline to modify the ASG capacity from 1 to 0 once it has determined there is no more work to do
    • Only happens once we've received a ManifestList.done file
    • On receiving this file, we ensure all manifest files are done processing and every manifest in the manifest list is accounted for
    • Termination signal is propagated through the pipeline's signal handlers to ensure all cleanup logic is executed
    • New permissions are given to the pipeline so it can modify its ASG appropriately
  • Removes scale-in logic from the scheduler lambda since it's no longer needed
  • Adds a new script for converting a synthetic data load to a non-synthetic data format
    • Useful for testing different scenarios using synthetic data in the test environment

Note that the manifest list file will not be moved to the "done" folder since we're removing the move logic soon anyway.

What Should Reviewers Watch For?

If you're reviewing this PR, please check for these things in particular:

What Security Implications Does This PR Have?

Please indicate if this PR does any of the following:

  • Adds any new software dependencies

  • Modifies any security controls

  • Adds new transmission or storage of data

  • Any other changes that could possibly affect security?

  • I have considered the above security implications as it relates to this PR. (If one or more of the above apply, it cannot be merged without the ISSO or team security engineer's (@sb-benohe) approval.)

Validation

Have you fully verified and tested these changes? Is the acceptance criteria met? Please provide reproducible testing instructions, code snippets, or screenshots as applicable.

Unit/integration tests, manually tested both synthetic and non-synthetic loads in an ephemeral environment.

@@ -0,0 +1,12 @@
[tool.poetry]
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 decided to go rogue and use poetry here. Overall I think it's much easier to manage than messing with pip-compile.

@@ -52,5 +52,5 @@ Please indicate if this PR does any of the following:

### Validation

Have you fully verified and tested these changes? Is the acceptance criteria met? Please provide reproducible testing instructions, code snippets, or screenshots as applicable.
**Have you fully verified and tested these changes? Is the acceptance criteria met? Please provide reproducible testing instructions, code snippets, or screenshots as applicable.**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to bold this bit so it's easier to differentiate the question from the response

* The date when the final manifest list was implemented. Pipeline runs before this date won't
* have a manifest list.
*/
private static final Instant FINAL_MANIFEST_CUTOFF = Instant.parse("2024-12-12T00:00:00Z");
Copy link
Contributor

Choose a reason for hiding this comment

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

This has a magic number feel, to me. Not sure how else to implement, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove it if/when we get rid of the old manifest loads, but it's necessary for now because manifest loads older than that don't have a manifest list file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's similar logic elsewhere to handle older records before some feature was implemented.

@dondevun dondevun self-requested a review January 14, 2025 14:31
dondevun
dondevun previously approved these changes Jan 14, 2025
MahiFentaye
MahiFentaye previously approved these changes Jan 14, 2025
@aschey-forpeople aschey-forpeople dismissed stale reviews from MahiFentaye and dondevun via 96392d3 January 16, 2025 00:01
dondevun
dondevun previously approved these changes Jan 16, 2025
echo put-parameter $full_ssm_name
# some values may need to be escaped to be used as an env var
# but should be unescaped in SSM
value=$(echo "${value}" | tr -d '\')
Copy link
Contributor Author

@aschey-forpeople aschey-forpeople Jan 17, 2025

Choose a reason for hiding this comment

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

Unrelated, had to make this change so that variables with quotes can be used in both SSM and environment variables due to unfortunate differences in escaping

@aschey-forpeople aschey-forpeople changed the base branch from master to BFD-3129 January 18, 2025 00:23
@aschey-forpeople aschey-forpeople changed the base branch from BFD-3129 to master January 18, 2025 00:24
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