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

[Feature] Add support for urls to substitute latest for the most recent build number #1492

Closed
peternied opened this issue Jan 18, 2022 · 17 comments · Fixed by #1569
Closed
Assignees
Labels

Comments

@peternied
Copy link
Member

peternied commented Jan 18, 2022

From opensearch-project/OpenSearch-Dashboards#1038

Potential Solutions:

  • Add functionality to the url re-writer
    The cdk deployment that uses a lambda function to rewrite the url on incoming requests to the s3 buckets, this could be updated to handle the /latest/ string in place of a build number by scanning the s3 bucket.

    const urlRewriter = new Function(stack, 'CfUrlRewriter', {
    code: Code.fromInline(`
    exports.handler = (event, context, callback) => {
    const request = event.Records[0].cf.request;
    request.uri = request.uri.replace(/^\\/ci\\/...\\//, '\\/')
    callback(null, request);
    };`),

    Trade-offs: Con, the bucket might not contain that artifact and result in a 404. Pro, could be applied on older builds.

  • When uploading artifacts, also add a latest bucket
    The upload code is on a shard and the artifacts could be uploaded into multiple locations.

    withAWS(role: "${ARTIFACT_PROMOTION_ROLE_NAME}", roleAccount: "${AWS_ACCOUNT_ARTIFACT}", duration: 900, roleSessionName: 'jenkins-session') {
    s3Upload(file: "builds/${productFilename}/${minArtifactPath}", bucket: "${ARTIFACT_PRODUCTION_BUCKET_NAME}", path: "release-candidates/core/${productFilename}/${buildManifest.build.version}/")
    s3Upload(file: "dist/${productFilename}/${packageName}", bucket: "${ARTIFACT_PRODUCTION_BUCKET_NAME}", path: "release-candidates/bundle/${productFilename}/${buildManifest.build.version}/")
    }

    Trade-offs: Con, Only works on products that build with that shared library function. Doesn't work with 1.x kind of functionality

Acceptance Criteria:

  • Given a url such as https://ci.opensearch.org/ci/dbc/bundle-build-dashboards/1.2.0/LATEST/linux/x64/builds/plugins/queryWorkbenchDashboards-1.2.0.zip gets the same artifact as calling https://ci.opensearch.org/ci/dbc/bundle-build-dashboards/1.2.0/512/linux/x64/builds/plugins/queryWorkbenchDashboards-1.2.0.zip (Assuming 512 is the most recent version)
  • Given a url such as https://ci.opensearch.org/ci/dbc/bundle-build-dashboards/1.2.0/LATEST/linux/x64/builds/plugins/queryWorkbenchDashboards-1.2.0.zip after the artifact has been updated the url returns the artifact in a way that bypasses caching.
@dblock
Copy link
Member

dblock commented Jan 18, 2022

I think the best case scenario for "latest" would be a 302 redirect, is that an option 3? That is also something that would allow us to move downloadable files around for older versions, as we tend to do this quite a bit while we learn the "ideal" url structure.

@peternied
Copy link
Member Author

I think the best case scenario for "latest" would be a 302 redirect

Good callout, I've added it to the acceptance criteria that we need a way to get through a cache.

@dblock
Copy link
Member

dblock commented Jan 18, 2022

Couple more:

  1. Coming from updated promote artifacts lib s3 upload #1493 I'd like "latest" to automatically give me a released version of 1.2.3 if 1.2.3 has been released or the latest available build of 1.2.3 if it has not.
  2. Consider supporting latest semver, latest 1.x could be the latest released 1.2.3, etc.

@tianleh
Copy link
Member

tianleh commented Jan 20, 2022

@peternied We have a dependency on this for running Dashboards integ test against a static OpenSearch url #1322 Do you know when this issue can be done?

@peternied
Copy link
Member Author

@tianleh we would welcome your contribution, The first option, Add functionality to the url re-writer seem like the sensible option to support a wide variety of scenarios and backward compatibility

@tianleh
Copy link
Member

tianleh commented Jan 20, 2022

take a look

@tianleh tianleh self-assigned this Jan 20, 2022
@tianleh
Copy link
Member

tianleh commented Jan 20, 2022

@seraphjiang for visibility

@seraphjiang
Copy link
Member

seraphjiang commented Jan 20, 2022

@tianleh we would welcome your contribution, The first option, Add functionality to the url re-writer seem like the sensible option to support a wide variety of scenarios and backward compatibility

Hi @peternied, could this be prioritized this item?

@tianleh
Copy link
Member

tianleh commented Jan 20, 2022

Experimenting with Option 1 a bit. It requires a list operation of the S3 bucket to find the most modified object. This can be done by a command like this
aws s3api list-objects-v2 --bucket "test-access-1-20" --query 'sort_by(Contents, &LastModified)[-1].Key' --output=text

However, this query will have performance impact to sort the objects under the bucket and require potential pagination (>1000 items). Thus we may need to build some storage layer which stores the mapping from latest for every version to a build number somewhere (e.g DynamoDB).

Given this, for simplicity, I would suggest the option 2 ("When uploading artifacts, also add a latest bucket") for development in this iteration.

@tianleh
Copy link
Member

tianleh commented Jan 20, 2022

@dblock

@seraphjiang
Copy link
Member

@tianleh we would welcome your contribution, The first option, Add functionality to the url re-writer seem like the sensible option to support a wide variety of scenarios and backward compatibility

Hi @peternied, could this be prioritized this item?

had quick chat with @peternied , this is just suggestion, not in critical path, there is no expected ETA for it.

@peternied
Copy link
Member Author

done by a command like this
aws s3api list-objects-v2 --bucket "test-access-1-20" --query 'sort_by(Contents, &LastModified)[-1].Key' --output=text

@tianleh If this output is sorted in newest to oldest, shouldn't the first result be the most recent build? That seems pretty scalable without any addition additional service calls or tracking systems.

The number of builds for a given release is a function of the number of changes in it, so I doubt this will be sufficiently larger than 1,000 change if we are releasing every quarter or faster.

@tianleh
Copy link
Member

tianleh commented Jan 20, 2022

Thanks Peter for mentioning the ls and adding / to the folder name to get the one-step deep sub folders.
aws s3 ls s3://artifact-bucket-stack-buildbucket-9omh0hnpg12q/distribution-build-opensearch/1.2.3/

I will check more on how the logic can be integrated into the Lambda function https://github.com/opensearch-project/opensearch-build/blob/main/deployment/lib/artifacts-public-access.ts#L23

@tianleh
Copy link
Member

tianleh commented Jan 27, 2022

After researching more, there are two ways to perform such in JavaScript.

  1. use the aws-sdk and s3.listObjects. See this https://docs.aws.amazon.com/code-samples/latest/catalog/javascript-s3-s3_listobjects.js.html
  2. use the aws-cli-js. See this https://www.npmjs.com/package/aws-cli-js

Will experiment more to see which one is simpler.

@tianleh
Copy link
Member

tianleh commented Jan 27, 2022

Also current Lambda code exists as an inline String https://github.com/opensearch-project/opensearch-build/blob/main/deployment/lib/artifacts-public-access.ts#L24 Will also check if there is an elegant way to make it exist as regular code for better readability since now the logic is more complicated.

@peternied
Copy link
Member Author

@tianleh Is there a draft PR that I could follow? While I would love an elegant solution, I think it make sense to discuss tradeoffs in a PR

@tianleh
Copy link
Member

tianleh commented Jan 27, 2022

@tianleh Is there a draft PR that I could follow? While I would love an elegant solution, I think it make sense to discuss tradeoffs in a PR

Sure. #1569

kavilla added a commit to kavilla/opensearch-build that referenced this issue Feb 18, 2022
Execute the integ-test workflow for OpenSearch Dashboards including
tests.

As opposed to OpenSearch, we need an image with the appropriate libraries
so we pass the image but it does not have the wget so the steps were broken
to access the build manifest.

Hardcoding the OpenSearch build id for now until this get resolves:
opensearch-project#1492

ARM tests will fail still until this is resolved:
opensearch-project#1381

Issue partially resolved:
opensearch-project#704

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
kavilla added a commit to kavilla/opensearch-build that referenced this issue Feb 18, 2022
Execute the integ-test workflow for OpenSearch Dashboards including
tests.

As opposed to OpenSearch, we need an image with the appropriate libraries
so we pass the image but it does not have the wget so the steps were broken
to access the build manifest.

Hardcoding the OpenSearch build id for now until this get resolves:
opensearch-project#1492

ARM tests will fail still until this is resolved:
opensearch-project#1381

Issue partially resolved:
opensearch-project#704

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
kavilla added a commit to kavilla/opensearch-build that referenced this issue Feb 22, 2022
Execute the integ-test workflow for OpenSearch Dashboards including
tests.

As opposed to OpenSearch, we need an image with the appropriate libraries
so we pass the image but it does not have the wget so the steps were broken
to access the build manifest.

Hardcoding the OpenSearch build id for now until this get resolves:
opensearch-project#1492

ARM tests will fail still until this is resolved:
opensearch-project#1381

Issue partially resolved:
opensearch-project#704

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
kavilla added a commit to kavilla/opensearch-build that referenced this issue Feb 22, 2022
Execute the integ-test workflow for OpenSearch Dashboards including
tests.

As opposed to OpenSearch, we need an image with the appropriate libraries
so we pass the image but it does not have the wget so the steps were broken
to access the build manifest.

Hardcoding the OpenSearch build id for now until this get resolves:
opensearch-project#1492

ARM tests will fail still until this is resolved:
opensearch-project#1381

Issue partially resolved:
opensearch-project#704

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
kavilla added a commit to kavilla/opensearch-build that referenced this issue Feb 23, 2022
Execute the integ-test workflow for OpenSearch Dashboards including
tests.

As opposed to OpenSearch, we need an image with the appropriate libraries
so we pass the image but it does not have the wget so the steps were broken
to access the build manifest.

Hardcoding the OpenSearch build id for now until this get resolves:
opensearch-project#1492

ARM tests will fail still until this is resolved:
opensearch-project#1381

Issue partially resolved:
opensearch-project#704

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
kavilla added a commit to kavilla/opensearch-build that referenced this issue Feb 24, 2022
Execute the integ-test workflow for OpenSearch Dashboards including
tests.

As opposed to OpenSearch, we need an image with the appropriate libraries
so we pass the image but it does not have the wget so the steps were broken
to access the build manifest.

Hardcoding the OpenSearch build id for now until this get resolves:
opensearch-project#1492

ARM tests will fail still until this is resolved:
opensearch-project#1381

Issue partially resolved:
opensearch-project#704

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
gaiksaya pushed a commit that referenced this issue Mar 2, 2022
* [Dashboards] integ-test in Jenkins

Execute the integ-test workflow for OpenSearch Dashboards including
tests.

As opposed to OpenSearch, we need an image with the appropriate libraries
so we pass the image but it does not have the wget so the steps were broken
to access the build manifest.

Hardcoding the OpenSearch build id for now until this get resolves:
#1492

ARM tests will fail still until this is resolved:
#1381

Issue partially resolved:
#704

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants