[Console] OSI support for API and Console Library #680

lewijacn · 2024-05-23T20:18:51Z

Description

This change refactors the previous OSI migration library to be a component of the new console library. Such that the following commands are now available from the console CLI:

Usage: console [OPTIONS] COMMAND [ARGS]...

Options:
  --config-file TEXT  Path to config file
  --json
  -v, --verbose       Verbosity level. Default is warn, -v is info, -vv is
                      debug.
  --help              Show this message and exit.

Commands:
  backfill  All actions related to historical/backfill data migrations
  clusters
  metrics
  replayer

Enter Backfill Options:

Usage: console backfill [OPTIONS] COMMAND [ARGS]...

  All actions related to historical/backfill data migrations

Options:
  --help  Show this message and exit.

Commands:
  create-migration  Create migration action
  start-migration   Start migration action
  stop-migration    Stop migration action

Along with this the three stubbed django APIs have now been implemented to also use this common library on the Migration console, giving the following endpoints:

curl -XPOST http://localhost:8000/orchestrator/osi-create-migration
curl -XPOST http://localhost:8000/orchestrator/osi-start-migration
curl -XPOST http://localhost:8000/orchestrator/osi-stop-migration

Issues Resolved

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

Is this a backport? If so, please add backport PR # and/or commits #

Testing

Local docker testing, unit 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.

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

codecov · 2024-05-23T20:41:02Z

Codecov Report

Attention: Patch coverage is 74.43946% with 57 lines in your changes are missing coverage. Please review.

Project coverage is 73.60%. Comparing base (5b185fa) to head (3e83480).
Report is 3 commits behind head on e2e-tests-q2.

Files	Patch %	Lines
...rationConsole/lib/console_link/console_link/cli.py	0.00%	21 Missing ⚠️
.../lib/console_link/console_link/models/osi_utils.py	90.00%	15 Missing ⚠️
.../lib/console_link/console_link/models/migration.py	70.27%	11 Missing ⚠️
...b/console_link/console_link/logic/instantiation.py	0.00%	9 Missing ⚠️
...le/lib/console_link/console_link/models/cluster.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##             e2e-tests-q2     #680      +/-   ##
==================================================
- Coverage           75.33%   73.60%   -1.73%     
  Complexity           1416     1416              
==================================================
  Files                 150      160      +10     
  Lines                6147     6707     +560     
  Branches              561      561              
==================================================
+ Hits                 4631     4937     +306     
- Misses               1143     1397     +254     
  Partials              373      373

Flag	Coverage Δ
unittests	`75.33% <ø> (ø)`

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.

…nsole-link Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

…nsole-link # Conflicts: # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/cli.py # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/logic/__init__.py # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/logic/instantiation.py # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/cluster.py # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/tests/requirements.txt # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/osiMigration.py

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

mikaylathompson

Super cool. Most of my comments are about organization. Open to pushback on them, or let me know if you think there's a preferable path where we keep it as is for now and refactor in the near-ish future.

mikaylathompson · 2024-05-24T20:41:02Z

...Solution/src/main/docker/migrationConsole/console_api/console_api/apps/orchestrator/views.py

    logger.info(pretty_request(request, request_data))
+
+    osi_serializer = OpenSearchIngestionCreateRequestSerializer(data=request_data)


this is super nice!

mikaylathompson · 2024-05-24T20:42:12Z

...Solution/src/main/docker/migrationConsole/console_api/console_api/apps/orchestrator/views.py

    response_data = {
-        'Timestamp': datetime.datetime.now(datetime.timezone.utc)
+        'Timestamp': datetime.datetime.now(datetime.timezone.utc),
+        'MigrationType': MigrationType.OSI_HISTORICAL_MIGRATION


Is this a thing that DMS has requested? I'm fine with it, but not sure exactly why we need it

They haven't and in its current context isn't super useful. I originally was going to have this function not be specific to a certain type of migration and this would have been more useful.

mikaylathompson · 2024-05-24T20:43:19Z

...rSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/migration.py

+    OSI_HISTORICAL_MIGRATION = "OSI_HISTORICAL_MIGRATION"
+
+
+class Migration():


can we make this Backfill or Historical? I think a Migration will be a very high level class that includes the full context of backfill, a replay, possibly validation, switchover, etc.

mikaylathompson · 2024-05-24T20:46:57Z

TrafficCapture/dockerSolution/src/main/docker/migrationConsole/console_api/requirements.txt

@@ -1,3 +1,5 @@
+-e ../lib/console_link


in the future, we probably don't want -e (editable install) but it's very helpful for testing in the short term. just worth adding a note that it shouldn't make it to production.

Agreed, have added a comment here as a reminder

mikaylathompson · 2024-05-24T20:48:16Z

TrafficCapture/dockerSolution/src/main/docker/migrationConsole/docker-compose-console-only.yml

+      - AWS_ACCESS_KEY_ID=${AWS_ACCESS_KEY_ID}
+      - AWS_SECRET_ACCESS_KEY=${AWS_SECRET_ACCESS_KEY}
+      - AWS_SESSION_TOKEN=${AWS_SESSION_TOKEN}
+      - AWS_DEFAULT_REGION=${AWS_DEFAULT_REGION}


this approach vs. mounting the ~/.aws directory -- is there a benefit? I don't think I know either approach well enough to have a preference, but standardization > not-standardization.

Curious how you typically load credentials? I think my ~/.aws is pretty much empty and I normally am only using environment variables

mikaylathompson · 2024-05-24T21:08:11Z

...kerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/cluster.py

+        try:
            self.auth_details = config["authorization"]["details"]
+        except KeyError:
+            self.auth_details = None


in general, I think self.auth_details = config["authorization"].get("details", None) is a little more concise and clearer. I'd do try/except in this case if we intended to do a more complicated behavior or throw an error if it were missing.

not a blocker though.

Definitely cleaner, updated

mikaylathompson · 2024-05-24T21:10:21Z

...rSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/migration.py

+        'type': 'list',
+        'required': True,
+        'schema': {
+            'type': 'string',
+        }


this syntax means "list of strings"? 👍

mikaylathompson · 2024-05-24T21:14:45Z

...rSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/migration.py

+        if not v.validate(config):
+            raise ValueError("Invalid config file for OpenSearchIngestion migration", v.errors)
+        self.osi_props = OpenSearchIngestionMigrationProps(config=config)
+        self.migration_logic = OSIMigrationLogic()


this is interesting. I'm not opposed to it, but I had been picturing logic imports models, not models imports logic. I'm game to play with it for now and see what we end up with. The risk of having both directions right now is that we're going to run into circular imports at some point, but we can play with it until we hit that.

One point of consideration, I guess -- are there any contexts outside of this class that will use the OSIMigrationLogic? If not, I'd tend to make them internal methods of this class.

My thought was that logic would be used to make sure that the users of the library could remain as thin as possible--so catIndices goes in the logic module because 1. it means that callers don't have to know anything about the exact endpoint or how to call it, but 2. it would be the same logic for all Clusters, regardless of type, so there's no reason to re-implement it multiple times in each class.

I don't know if that's the optimal approach, very open to discussion on it. But I'd tend to say that if the OSIMigrationLogic stuff is more "internal" it should go inside this class or into a osi_utils sort of spot.

Per discussion, have moved all implementation details out of logic and they now reside in osi_utils. Still expecting a middleware layer that will create this OpenSearchIngestionMigration from the raw input it receives

mikaylathompson · 2024-05-24T21:20:43Z

...rSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/migration.py

+                                                      target_cluster=self.target_cluster,
+                                                      print_config_only=print_config_only)
+
+    def create_from_json(self, config_json: Dict, pipeline_template_path: str) -> None:


minor, probably an internal only class (because it's not part of the base class interface), so can be _create_from_json

Have removed this function for now

mikaylathompson · 2024-05-24T21:22:19Z

TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/README.md

- `authorization`: optional, if it is provided, type is required.
-	- `type`: required, the only currently implemented option is "basic", but "sigv4" should be available soon
+- `authorization`: required, the auth method to use, if no auth the no_auth type must be specified.
+	- `type`: required, the only currently implemented options are "no_auth" and "basic", but "sigv4" should be available soon


mind updating this to "basic_auth"? (in the example yaml above as well)

Updated these spots now

…nsole-link

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

…nsole-link # Conflicts: # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/tests/requirements.txt

lewijacn

Addressing some minor items now, but some design decisions here need further updates that I will address in this PR later

lewijacn · 2024-05-25T04:27:52Z

...Solution/src/main/docker/migrationConsole/console_api/console_api/apps/orchestrator/views.py

    response_data = {
-        'Timestamp': datetime.datetime.now(datetime.timezone.utc)
+        'Timestamp': datetime.datetime.now(datetime.timezone.utc),
+        'MigrationType': MigrationType.OSI_HISTORICAL_MIGRATION


They haven't and in its current context isn't super useful. I originally was going to have this function not be specific to a certain type of migration and this would have been more useful.

lewijacn · 2024-05-25T04:29:18Z

TrafficCapture/dockerSolution/src/main/docker/migrationConsole/console_api/requirements.txt

@@ -1,3 +1,5 @@
+-e ../lib/console_link


Agreed, have added a comment here as a reminder

lewijacn · 2024-05-25T04:38:39Z

TrafficCapture/dockerSolution/src/main/docker/migrationConsole/docker-compose-console-only.yml

+      - AWS_ACCESS_KEY_ID=${AWS_ACCESS_KEY_ID}
+      - AWS_SECRET_ACCESS_KEY=${AWS_SECRET_ACCESS_KEY}
+      - AWS_SESSION_TOKEN=${AWS_SESSION_TOKEN}
+      - AWS_DEFAULT_REGION=${AWS_DEFAULT_REGION}


Curious how you typically load credentials? I think my ~/.aws is pretty much empty and I normally am only using environment variables

lewijacn · 2024-05-25T04:41:00Z

TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/README.md

- `authorization`: optional, if it is provided, type is required.
-	- `type`: required, the only currently implemented option is "basic", but "sigv4" should be available soon
+- `authorization`: required, the auth method to use, if no auth the no_auth type must be specified.
+	- `type`: required, the only currently implemented options are "no_auth" and "basic", but "sigv4" should be available soon


Updated these spots now

lewijacn · 2024-05-25T04:43:26Z

...Capture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/cli.py

+# ##################### OSI ###################
+
+@cli.command(name="osi-create-migration")
+@click.option('--pipeline-template-file', default='/root/osiPipelineTemplate.yaml', help='Path to config file')


By default we will use the pipeline template file in the starting directory of the migration console, but allowing this option is nice for more involved cases

lewijacn · 2024-05-25T04:45:31Z

...kerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/cluster.py

+    'authorization': {
+        'type': 'dict',
+        'required': True,
+        'schema': {
+            'type': {
+                'type': 'string',
+                'required': True,
+                'allowed': [e.name.lower() for e in AuthMethod]


Let me at least update so there is consistent quote usage

lewijacn · 2024-05-25T04:47:17Z

...kerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/cluster.py

+        try:
            self.auth_type = AuthMethod[config["authorization"]["type"].upper()]
+        except KeyError:
+            self.auth_type = None


You're right, updated

lewijacn · 2024-05-25T04:48:34Z

...kerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/cluster.py

+        try:
            self.auth_details = config["authorization"]["details"]
+        except KeyError:
+            self.auth_details = None


Definitely cleaner, updated

lewijacn · 2024-05-25T04:52:18Z

...rSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/migration.py

+        'type': 'list',
+        'required': True,
+        'schema': {
+            'type': 'string',
+        }


Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

.flake8

TrafficCapture/dockerSolution/src/main/docker/migrationConsole/README.md

...on/src/main/docker/migrationConsole/console_api/console_api/apps/orchestrator/serializers.py

...lution/src/main/docker/migrationConsole/lib/console_link/console_link/logic/osi_migration.py

...rSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/migration.py

TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/services.yaml

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

mikaylathompson

🚢

...rSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/migration.py

peternied

I think there might be some linger comment threads, not sure if you'd want to follow up on them or not. but I think it would be great to merge this change onto the feature branch.

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

lewijacn added 4 commits May 20, 2024 22:38

Add partial implementation for OSI console link

5861929

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Raw implementation for osi with console-link

19790ab

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Merge remote-tracking branch 'origin/main' into osi-to-console-link

ec59574

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Basic working functionality for osi on console and api

4fd1395

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

lewijacn added 2 commits May 23, 2024 19:59

Merge remote-tracking branch 'migrations/e2e-tests-q2' into osi-to-co…

2b5d2bd

…nsole-link Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Minor edits from docker testing

767ab48

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

lewijacn marked this pull request as ready for review May 24, 2024 01:19

lewijacn requested review from AndreKurait, chelma, gregschohn, mikaylathompson, okhasawn, peternied and sumobrian as code owners May 24, 2024 01:19

lewijacn added 5 commits May 24, 2024 11:22

Refactor test cases for osi_migration

1be58c2

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Update services.yaml

4b802cf

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Flake8 fix

6b3329b

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Updates for CI testing and minor fixes

d140495

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

mikaylathompson reviewed May 24, 2024

View reviewed changes

lewijacn added 4 commits May 24, 2024 17:26

Merge remote-tracking branch 'migrations/e2e-tests-q2' into osi-to-co…

63f0247

…nsole-link

Additional testing for migration.py

2627fbc

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Update for flake8

e07d45c

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

Merge remote-tracking branch 'migrations/e2e-tests-q2' into osi-to-co…

20f98bb

…nsole-link # Conflicts: # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/tests/requirements.txt

lewijacn commented May 25, 2024

View reviewed changes

Minor updates from PR feedback

bd5a434

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

peternied reviewed May 28, 2024

View reviewed changes

Structure changes and minor fixes from PR feedback

74b7bc5

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

mikaylathompson self-requested a review May 28, 2024 22:47

mikaylathompson approved these changes May 28, 2024

View reviewed changes

...rSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/migration.py Show resolved Hide resolved

peternied approved these changes May 29, 2024

View reviewed changes

Add basic sanity tests for osi_utils create functions

3e83480

Signed-off-by: Tanner Lewis <lewijacn@amazon.com>

lewijacn merged commit 9f167bd into opensearch-project:e2e-tests-q2 May 29, 2024
5 of 8 checks passed

mikaylathompson mentioned this pull request Jun 13, 2024

[Console] Update format of services.yaml #718

Merged

4 tasks

lewijacn deleted the osi-to-console-link branch October 10, 2024 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] OSI support for API and Console Library #680

[Console] OSI support for API and Console Library #680

lewijacn commented May 23, 2024 •

edited

Loading

codecov bot commented May 23, 2024 •

edited

Loading

mikaylathompson left a comment

mikaylathompson May 24, 2024

mikaylathompson May 24, 2024

lewijacn May 25, 2024

mikaylathompson May 24, 2024

mikaylathompson May 24, 2024

lewijacn May 25, 2024

mikaylathompson May 24, 2024

lewijacn May 25, 2024

mikaylathompson May 24, 2024

lewijacn May 25, 2024

mikaylathompson May 24, 2024

lewijacn May 25, 2024

mikaylathompson May 24, 2024

mikaylathompson May 24, 2024

lewijacn May 28, 2024

mikaylathompson May 24, 2024

lewijacn May 28, 2024

mikaylathompson May 24, 2024

lewijacn May 25, 2024

lewijacn left a comment •

edited

Loading

lewijacn May 25, 2024

lewijacn May 25, 2024

lewijacn May 25, 2024

lewijacn May 25, 2024

lewijacn May 25, 2024

lewijacn May 25, 2024

lewijacn May 25, 2024

lewijacn May 25, 2024

lewijacn May 25, 2024

mikaylathompson left a comment

peternied left a comment •

edited

Loading

		logger.info(pretty_request(request, request_data))

		osi_serializer = OpenSearchIngestionCreateRequestSerializer(data=request_data)

		OSI_HISTORICAL_MIGRATION = "OSI_HISTORICAL_MIGRATION"


		class Migration():

[Console] OSI support for API and Console Library #680

[Console] OSI support for API and Console Library #680

Conversation

lewijacn commented May 23, 2024 • edited Loading

Description

Issues Resolved

Testing

Check List

codecov bot commented May 23, 2024 • edited Loading

Codecov Report

mikaylathompson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewijacn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaylathompson left a comment

Choose a reason for hiding this comment

peternied left a comment • edited Loading

Choose a reason for hiding this comment

lewijacn commented May 23, 2024 •

edited

Loading

codecov bot commented May 23, 2024 •

edited

Loading

lewijacn left a comment •

edited

Loading

peternied left a comment •

edited

Loading