Release/0.8.0 - #183
Conversation
…ts (#174) * issues/151: Added ExpectedBucketOwner parameter when making S3 requests * issues/151: Added ExpectedBucketOwner to get_object requests
#178) * issues/150: Added support for NRT filtering by granule id, minor fixes
* issues/173: changed default resource names to allow multiple deployments. BREAKING CHANGE, requires migrating using the new module inputs (see readme) * issues/181: Added retry logic for Harmony jobs that fail with 5xx errors
asjohnston-asf
left a comment
There was a problem hiding this comment.
The changes in get_harmony_job_status.py and state_machine_definition.tpl related to #181 look reasonable and are a clear improvement. I left two suggestions to make the retries a little more forgiving in additional error cases, but I don't see those as blocking a release.
| }, | ||
| { | ||
| "ErrorEquals":[ | ||
| "HarmonyTransientError" |
There was a problem hiding this comment.
I'd consider grouping this into the same clause with HarmonyJobIncompleteError. As written it only allows ~2 minutes for any underlying error to resolve itself. I'd be willing to give the issue the same amount of time to resolve as we're willing to wait for the job to finish.
There was a problem hiding this comment.
Yeah you're right, I didn't really think through the retry behavior in that much detail but it's idiosyncratic that it doesn't use the main retry parameters
| job_status = harmony_client.status(harmony_job_id) | ||
| try: | ||
| job_status = harmony_client.status(harmony_job_id) | ||
| except requests.exceptions.HTTPError as exc: |
There was a problem hiding this comment.
consider also catching requests.exceptions.ConnectionTimeout, and possibly requests.exceptions.ConnectionError, which will cover some of the cases where the request couldn't reach the server and doesn't get back any status code, 500 or otherwise.
There was a problem hiding this comment.
Pull request overview
This PR prepares the 0.8.0 release with breaking infrastructure renames for Consolidated Cumulus deployments, adds support for mixed NRT/Standard collections, improves robustness of Harmony status polling with retries on transient failures, and tightens S3 security by enforcing ExpectedBucketOwner.
Changes:
- Add NRT vs Standard handling via configurable
nrtFilenameRegex, and propagate NRT into CNM collection naming and CNM object keys (colon-safe filenames). - Add Harmony transient error classification and Step Functions retry behavior for 5xx/connection/timeouts.
- Update Terraform resource naming strategy for multi-deploy-per-account, plus add migration helpers (
force_delete_ecr,force_destroy_staging_bucket) and S3ExpectedBucketOwnerenforcement.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
bignbit/handle_big_result.py |
Adds NRT handling, refactors static day-of-year logic, switches checksum strategy to prefer S3 ETag, updates CNM naming. |
bignbit/get_harmony_job_status.py |
Wraps transient Harmony failures in HarmonyTransientError for retry. |
bignbit/get_dataset_configuration.py |
Adds ExpectedBucketOwner to S3 get_object. |
bignbit/send_to_gitc.py |
Adds ExpectedBucketOwner to S3 get_object when reading CNMs. |
bignbit/utils.py |
Adds cached STS lookup for AWS account ID and applies ExpectedBucketOwner to S3 uploads. |
terraform/main.tf |
Changes resource name prefixing strategy and introduces a separate SNS topic name local to preserve the existing topic name. |
terraform/lambda_functions.tf |
Updates ECR repo naming to include deployment prefix and adds force_delete option. |
terraform/s3.tf |
Adds force_destroy option for the staging bucket to support migration. |
terraform/sqs_sns.tf |
Preserves SNS topic name via sns_topic_name local while other resources adopt new naming. |
terraform/state_machine_definition.tpl |
Adds HarmonyTransientError to Step Functions retry configuration. |
terraform/variables.tf |
Introduces force_delete_ecr and force_destroy_staging_bucket variables. |
.github/workflows/cicd-pipeline.yml |
Adds GHCR login step prior to deployment. |
scripts/create_dataset_config.py |
Adds --nrtFilenameRegex CLI option for dataset config generation. |
scripts/CONFIGURATION.md |
Documents the new --nrtFilenameRegex option and refreshes CLI help formatting. |
README.md |
Documents new Terraform variables and dataset config field nrtFilenameRegex. |
tests/test_handle_big_result.py |
Expands tests for ETag-based checksums, multipart ETag fallback, NRT collection naming, and _resolve_static_data_day. |
tests/test_get_harmony_job_status.py |
Adds unit tests for transient error wrapping behavior. |
tests/test_send_to_gitc.py |
Updates tests to support STS mocking for account ID resolution. |
tests/test_image_set.py |
Updates construct_cnm calls for new is_nrt parameter. |
examples/cumulus-tf/tfvars/sit.tfvars |
Adds the new migration variables to the SIT example. |
CHANGELOG.md |
Adds 0.8.0 release notes matching the PR description. |
pyproject.toml |
Bumps package version to 0.8.0rc2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| get_response = s3_client.get_object(Bucket=bucket, Key=key, | ||
| ExpectedBucketOwner=utils.get_aws_account_id()) | ||
| checksum = hashlib.md5(get_response['Body'].read()).hexdigest() |
| [tool.poetry] | ||
| name = "bignbit" | ||
| version = "0.7.2" | ||
| version = "0.8.0rc2" |
Description
This release is intended to address some updates needed for Consolidated Cumulus (CCumulus) deployment, and to fix several pain points of scaling bignbit.
STOP AND PLEASE READ
!!!THIS IS A BREAKING CHANGE FOR EXISTING DEPLOYMENTS!!! If you are deploying this release to a venue with an existing deployment of bignbit, please note that most resource names have changed. You will need to temporarily configure your tfvars to force_delete the ecr repo and staging bucket, then set it back to false after migration:
Migration Procedure:
sit-bignbit,uat-bignbitorops-bignbit.bignbit-<prefix>-staging.Added
Changed
handle_big_resultlambda.Fixed
Security
Overview of verification done
Overview of integration done (TBD)
Integration test 1: Stress-testing in ASF's cumulus UAT environment to see if the retry behavior for Harmony status checks works under the load of processing 10 days of NISAR data.
Integration test 2: Consolidated Cumulus deployment and tests with ASDC TEMPO data, likely in Sprint 6.
PR checklist:
See Pull Request Review Checklist for pointers on reviewing this pull request