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

Make RFS runnable as Container #550

Merged
merged 3 commits into from
Apr 5, 2024

Conversation

lewijacn
Copy link
Collaborator

@lewijacn lewijacn commented Apr 4, 2024

Description

This changes introduces a Dockerfile for RFS that is linked to the gradle build process such that a user can make changes to RFS code or dependencies and simply execute ./gradlew buildDockerImages from the RFS directory and create a Docker image with the latest changes. A user could alternatively execute ./gradlew composeUp and create a docker environment with source/target/RFS containers using latest changes. Note: The gradle link is in place for docker compose but more work is needed here to limit manual steps a user needs to take to test RFS in this environment.

Along with this a few command line arguments were incorporated into RFS

Issues Resolved

https://opensearch.atlassian.net/browse/MIGRATIONS-1601

Testing

Local docker testing

Check List

  • New functionality includes testing
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Member

@chelma chelma left a comment

Choose a reason for hiding this comment

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

Can you please add a section to the README explaining how to use the new Docker functionality, hit the source/target clusters, and launch RFS?

RFS/docker/Dockerfile Outdated Show resolved Hide resolved
RFS/docker/Dockerfile Outdated Show resolved Hide resolved
Comment on lines 64 to 68
@Parameter(names = {"--index-template-whitelist"}, description = "List of index template names to migrate (e.g. 'posts_index_template1, posts_index_template2')", required = false)
public List<String> indexTemplateWhitelist;

@Parameter(names = {"--component-template-whitelist"}, description = "List of component template names to migrate (e.g. 'posts_template1, posts_template2')", required = false)
public List<String> componentTemplateWhitelist;
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, this is actually more potentially confusing than I thought originally. Per our convo, before ES 7.8, there was only templates, so if you use the script to migrate a 6.8 cluster you wouldn't see an appropriate option.

Would you mind changing --index-template-whitelist to --templates-whitelist and updating the description to indicate it refers to legacy templates for ES 6.8 and index templates for 7.10? Not a perfect fix long term, but probably enough for the moment.

TBH, this argument list is getting pretty long an gnarly. Not sure what to do about it right now, but we should think about ways to make it easier to navigate the many different use-cases it's trying to address.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, we'll also want a check so that --component-templates aren't supplied to a ES 6.8 migration.

I'm wondering if we want different wrapper scripts for different source versions, or something...

Copy link
Member

Choose a reason for hiding this comment

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

Created a follow-up JIRA for this: https://opensearch.atlassian.net/browse/MIGRATIONS-1646

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated as per comment for now. Need to think about this more but having a pattern for handling different arguments for different versions could be nice

@lewijacn
Copy link
Collaborator Author

lewijacn commented Apr 4, 2024

Can you please add a section to the README explaining how to use the new Docker functionality, hit the source/target clusters, and launch RFS?

Have added, will be revisiting the docker compose part of this in my other task so it will be more solid after I do some proper testing there

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.54%. Comparing base (5865ca3) to head (8167eec).
Report is 5 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #550      +/-   ##
============================================
- Coverage     76.64%   76.54%   -0.10%     
+ Complexity     1414     1410       -4     
============================================
  Files           155      155              
  Lines          6033     6033              
  Branches        543      543              
============================================
- Hits           4624     4618       -6     
- Misses         1044     1049       +5     
- Partials        365      366       +1     
Flag Coverage Δ
unittests 76.54% <ø> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chelma
Copy link
Member

chelma commented Apr 5, 2024

will be revisiting the docker compose part of this in my other task so it will be more solid after I do some proper testing there

Got it, makes sense. I'm hoping we'll be able to combine the special sauce I made for testing S3 snapshots into the docker compose setup, otherwise it probably won't be too useful 😛

Copy link
Member

@chelma chelma left a comment

Choose a reason for hiding this comment

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

LGTM, but we'll need to combine our Docker stuff soon.

Comment on lines +3 to +17
### Table of Contents (Generated)
- [reindex-from-snapshot](#reindex-from-snapshot)
- [How to run](#how-to-run)
- [Using a local snapshot directory](#using-a-local-snapshot-directory)
- [Using an existing S3 snapshot](#using-an-existing-s3-snapshot)
- [Using a source cluster](#using-a-source-cluster)
- [Using Docker](#using-docker)
- [Handling auth](#handling-auth)
- [How to set up an ES 6.8 Source Cluster w/ an attached debugger](#how-to-set-up-an-es-68-source-cluster-w-an-attached-debugger)
- [How to set up an ES 7.10 Source Cluster running in Docker](#how-to-set-up-an-es-710-source-cluster-running-in-docker)
- [Providing AWS permissions for S3 snapshot creation](#providing-aws-permissions-for-s3-snapshot-creation)
- [Setting up the Cluster w/ some sample docs](#setting-up-the-cluster-w-some-sample-docs)
- [How to set up an OS 2.11 Target Cluster](#how-to-set-up-an-os-211-target-cluster)


Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

@lewijacn lewijacn merged commit 48c43ea into opensearch-project:main Apr 5, 2024
6 of 7 checks passed
@lewijacn lewijacn deleted the rfs-docker branch May 23, 2024 20:50
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.

2 participants