Refactor migration assessment codepath: flatten reported issues #2153

sanyamsinghal · 2025-01-06T08:15:54Z

Describe the changes in this pull request

Temporarily added new code to assessMigrationCommandV2.go
These functions will replace the existing codepath after UI updates are implemented.

Fixes: DB-14718, DB-14709, DB-14710, DB-14711, DB-14712, DB-14713, DB-14714, DB-14715

Sample of the how/what all info is stored in the AssessmentIssue struct

AssessmentIssue[0] = (cmd.AssessmentIssue) {
 Category: (string) (len=7) "feature",
 CategoryDescription: (string) (len=80) "Features of the source database that are not supported on the target YugabyteDB.",
 TypeName: (string) (len=24) "UNSUPPORTED_INDEX_METHOD",
 TypeDescription: (string) "",
 Impact: (string) "",
 ObjectType: (string) (len=5) "INDEX",
 ObjectName: (string) (len=46) "idx_box_data ON public.mixed_data_types_table1",
 SqlStatement: (string) (len=82) "CREATE INDEX idx_box_data ON public.mixed_data_types_table1 USING gist (box_data);",
 DocsLink: (string) (len=126) "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#gist-brin-and-spgist-index-types-are-not-supported",
 MinimumVersionFixedIn: (map[string]*ybversion.YBVersion) <nil>
}

AssessmentIssue[25] = (cmd.AssessmentIssue) {
 Category: (string) (len=15) "query_construct",
 CategoryDescription: (string) "",
 TypeName: (string) (len=14) "Advisory Locks",
 TypeDescription: (string) "",
 Impact: (string) "",
 ObjectType: (string) "",
 ObjectName: (string) "",
 SqlStatement: (string) (len=32) "SELECT pg_advisory_unlock($1,$2)",
 DocsLink: (string) (len=113) "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#advisory-locks-is-not-yet-implemented",
 MinimumVersionFixedIn: (map[string]*ybversion.YBVersion) <nil>
}


AssessmentIssue[51] = (cmd.AssessmentIssue) {
 Category: (string) (len=8) "datatype",
 CategoryDescription: (string) (len=82) "Data types of the source database that are not supported on the target YugabyteDB.",
 TypeName: (string) (len=14) "int8multirange",
 TypeDescription: (string) "",
 Impact: (string) "",
 ObjectType: (string) (len=6) "column",
 ObjectName: (string) (len=43) "public.bigint_multirange_table.value_ranges",
 SqlStatement: (string) "",
 DocsLink: (string) "",
 MinimumVersionFixedIn: (map[string]*ybversion.YBVersion) <nil>
}


AssessmentIssue[54] = (cmd.AssessmentIssue) {
 Category: (string) (len=17) "migration_caveats",
 CategoryDescription: (string) (len=81) "Migration Caveats highlights the current limitations with the migration workflow.",
 TypeName: (string) (len=17) "POLICY_WITH_ROLES",
 TypeDescription: (string) (len=211) "There are some policies that are created for certain users/roles. During the export schema phase, USERs and GRANTs are not exported. Therefore, they will have to be manually created before running import schema.",
 Impact: (string) "",
 ObjectType: (string) (len=6) "POLICY",
 ObjectName: (string) (len=42) "policy_test_report ON public.test_xml_type",
 SqlStatement: (string) (len=85) "CREATE POLICY policy_test_report ON public.test_xml_type TO test_policy USING (true);",
 DocsLink: (string) (len=132) "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#policies-on-users-in-source-require-manual-user-creation",
 MinimumVersionFixedIn: (map[string]*ybversion.YBVersion) <nil>
}
AssessmentIssue[60] = (cmd.AssessmentIssue) {
 Category: (string) (len=17) "migration_caveats",
 CategoryDescription: (string) "",
 TypeName: (string) (len=68) "Unsupported Data Types for Live Migration with Fall-forward/Fallback",
 TypeDescription: (string) (len=203) "There are some data types in the schema that are not supported by live migration with fall-forward/fall-back. These columns will be excluded when exporting and importing data in live migration workflows.",
 Impact: (string) "",
 ObjectType: (string) (len=6) "column",
 ObjectName: (string) (len=31) "public.documents.title_tsvector",
 SqlStatement: (string) "",
 DocsLink: (string) (len=130) "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#unsupported-datatypes-by-voyager-during-live-migration",
 MinimumVersionFixedIn: (map[string]*ybversion.YBVersion) <nil>
}

TODO: Ensure all the issues (categories, types) are getting generation under AssessmentIssue

Describe if there are any user-facing changes

How was this pull request tested?

existing tests

Does your PR have changes that can cause upgrade issues?

Component	Breaking changes?
MetaDB	Yes/No
Name registry json	Yes/No
Data File Descriptor Json	Yes/No
Export Snapshot Status Json	Yes/No
Import Data State	Yes/No
Export Status Json	Yes/No
Data .sql files of tables	Yes/No
Export and import data queue	Yes/No
Schema Dump	Yes/No
AssessmentDB	Yes/No
Sizing DB	Yes/No
Migration Assessment Report Json	Yes/No
Callhome Json	Yes/No
YugabyteD Tables	Yes/No
TargetDB Metadata Tables	Yes/No

priyanshi-yb · 2025-01-06T10:09:09Z

yb-voyager/cmd/assessMigrationCommandV2.go

+	return assessmentIssues
+}
+
+// TODO: will replace fetchUnsupportedPGFeaturesFromSchemaReport()


These functions will replace the existing codepath after UI updates are implemented.

@sanyamsinghal if we will be replacing the old functions after UI changes then how will it still work to get the previous structure as I believe we still need that for YugabyteD and maybe callhome. cc: @makalaaneesh

hmm good catch, we might have to retain this then till the time ybd versions have moved to flattened AssessmentIssue approach.

To hide the current unflatten things in AssessmentReport struct, we can mark their json tag as "-" to avoid it getting in the assessmentReport.json

we can't remove them from the json either, unfortunately 🥲
because we send the json to yugabyted.

yeah rights, thats bad, we send the complete assessment report json, not the require fields.

sanyamsinghal · 2025-01-06T11:49:20Z

https://jenkins.dev.yugabyte.com/job/users/job/yb-voyager-testing/job/yb-voyager-testing-pipeline/4407/
https://jenkins.dev.yugabyte.com/job/users/job/yb-voyager-testing/job/yb-voyager-testing-pipeline/4418/

Final: https://jenkins.dev.yugabyte.com/job/users/job/yb-voyager-testing/job/yb-voyager-testing-pipeline/4425/

sanyamsinghal · 2025-01-07T10:37:52Z

yb-voyager/cmd/analyzeSchema.go

In this file, changes are mostly due to update in the reportCase function signature to have impact also as an argument.

makalaaneesh

(did not review the impact levels for each issue.)

makalaaneesh · 2025-01-07T13:19:37Z

yb-voyager/cmd/assessMigrationCommand.go

@@ -973,38 +985,59 @@ func areMinVersionsFixedInEqual(m1 map[string]*ybversion.YBVersion, m2 map[strin
 	return true
 }

-func getUnsupportedFeaturesFromSchemaAnalysisReport(featureName string, issueReason string, issueType string, schemaAnalysisReport utils.SchemaReport, displayDDLInHTML bool, description string) UnsupportedFeature {
+func getUnsupportedFeaturesFromSchemaAnalysisReport(category string, featureName string, issueReason string, issueType string, schemaAnalysisReport utils.SchemaReport, displayDDLInHTML bool, description string) UnsupportedFeature {


I thought category was already present in AnalyzeSchemaIssue as IssueType, right? Why, then do we need to explicitly pass this again here?

I was not sure if category present in analyzeschema issue is reliable.
Here i see that we are assigning it inside the convert function

func convertIssueInstanceToAnalyzeIssue(issueInstance queryissue.QueryIssue, fileName string, isPlPgSQLIssue bool) utils.AnalyzeSchemaIssue { issueType := UNSUPPORTED_FEATURES switch true { case isPlPgSQLIssue: issueType = UNSUPPORTED_PLPGSQL_OBJECTS case slices.ContainsFunc(MigrationCaveatsIssues, func(i string) bool { //Adding the MIGRATION_CAVEATS issueType of the utils.Issue for these issueInstances in MigrationCaveatsIssues return strings.Contains(issueInstance.Name, i) }): issueType = MIGRATION_CAVEATS case strings.HasPrefix(issueInstance.Name, UNSUPPORTED_DATATYPE): //Adding the UNSUPPORTED_DATATYPES issueType of the utils.Issue for these issues whose TypeName starts with "Unsupported datatype ..." issueType = UNSUPPORTED_DATATYPES }

Also we are naming it differently right now for assessment issue.
I thought its safer to provide when the caller already know.

@priyanshi-yb can confirm this, but I think we have to actually rely on the category set by the convertIssueInstanceToAnalyzeIssue . Caller of getUnsupportedFeaturesFromSchemaAnalysisReport is just looking for certain issue types, but issueType:category is not 1:1. For example, advisory locks can come up in a DML_QUERY or in PLPGSQL. Same issue type, different category.

Yes we can rely on convertIssueInstanceToAnalyzeIssue's IssueType which is the actual category of FEATURE, PLPGPSQL, DATATYPE

yb-voyager/cmd/assessMigrationCommand.go

yb-voyager/src/issue/issue.go

yb-voyager/src/query/queryissue/issues_dml.go

.github/workflows/pg-17-migtests.yml

- goal is to eventually get rid of using issue reason for lookups

- Temporarily added new code to assessMigrationCommandV2.go - These functions will replace the existing codepath after UI updates are implemented.

…and Description

… AssessmentIssue

…sues detected using regexp in analyze schema

- having V2 file and function will increase the scope to maintain the code

yb-voyager/cmd/assessMigrationCommand.go

priyanshi-yb · 2025-01-08T05:57:34Z

yb-voyager/src/constants/constants.go

@@ -19,10 +19,24 @@ const (
 	// Database Object types
 	TABLE    = "table"
 	FUNCTION = "function"
+	COLUMN   = "column"


db object types are mostly used all in caps in code TABLE, FUNCTION or COLUMN

it has been there like that before, lets change it later. Don't want to risk the scope of this PR.

yb-voyager/src/query/queryissue/issues_dml.go

makalaaneesh

LGTM!

sanyamsinghal self-assigned this Jan 6, 2025

priyanshi-yb reviewed Jan 6, 2025

View reviewed changes

sanyamsinghal requested review from makalaaneesh and priyanshi-yb January 6, 2025 10:40

sanyamsinghal marked this pull request as ready for review January 6, 2025 12:19

sanyamsinghal force-pushed the sanyam/refactor-assess2 branch from 408ff8f to 134b147 Compare January 7, 2025 10:33

sanyamsinghal commented Jan 7, 2025

View reviewed changes

makalaaneesh reviewed Jan 7, 2025

View reviewed changes

.github/workflows/pg-17-migtests.yml Show resolved Hide resolved

sanyamsinghal added 14 commits January 7, 2025 16:50

Use Issue Type for issues lookup in schema analysis report

579b050

- goal is to eventually get rid of using issue reason for lookups

reverting some cases

e2226a1

Refactor migration assessment codepath: flatten reported issues

5bfc5eb

- Temporarily added new code to assessMigrationCommandV2.go - These functions will replace the existing codepath after UI updates are implemented.

removed prints

f55fc4d

minor cleanup and comments

b6adfcf

moved pg/omnibus schema-migration test to the correct place

d4318f6

Added Type field to AssessmentIssue struct

b066879

Renamed TypeName and TypeDescription field of Issues structs to Name …

c4d920b

…and Description

Defining impact for issues_ddl and issues_dml and propagating that to…

ae28027

… AssessmentIssue

Added new arg - impact to reportCase() for assigning impact to the is…

869fe09

…sues detected using regexp in analyze schema

Added placeholder for impact of the new categories being considered now

54f1d59

Assigning impact to remaining cases/issues

5cd1f84

Moved code for building AssessmentIssue to the main codepath

cf943b2

- having V2 file and function will increase the scope to maintain the code

added impact for issues missed before

3ca100c

sanyamsinghal force-pushed the sanyam/refactor-assess2 branch from 3cee8cf to 3ca100c Compare January 7, 2025 16:53

changes as per review comments

afe39a0

priyanshi-yb reviewed Jan 8, 2025

View reviewed changes

review comments, renames and cleanup

b7968ab

sanyamsinghal requested review from makalaaneesh and priyanshi-yb January 8, 2025 08:10

makalaaneesh approved these changes Jan 8, 2025

View reviewed changes

sanyamsinghal merged commit d490424 into main Jan 8, 2025
67 checks passed

sanyamsinghal deleted the sanyam/refactor-assess2 branch January 8, 2025 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor migration assessment codepath: flatten reported issues #2153

Refactor migration assessment codepath: flatten reported issues #2153

sanyamsinghal commented Jan 6, 2025 •

edited

Loading

priyanshi-yb Jan 6, 2025

sanyamsinghal Jan 6, 2025

makalaaneesh Jan 6, 2025

sanyamsinghal Jan 6, 2025

sanyamsinghal commented Jan 6, 2025 •

edited

Loading

sanyamsinghal Jan 7, 2025

makalaaneesh left a comment

makalaaneesh Jan 7, 2025

sanyamsinghal Jan 7, 2025

makalaaneesh Jan 8, 2025

priyanshi-yb Jan 8, 2025

sanyamsinghal Jan 8, 2025

priyanshi-yb Jan 8, 2025

sanyamsinghal Jan 8, 2025

makalaaneesh left a comment

Refactor migration assessment codepath: flatten reported issues #2153

Refactor migration assessment codepath: flatten reported issues #2153

Conversation

sanyamsinghal commented Jan 6, 2025 • edited Loading

Describe the changes in this pull request

Describe if there are any user-facing changes

How was this pull request tested?

Does your PR have changes that can cause upgrade issues?

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

sanyamsinghal commented Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

makalaaneesh 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

makalaaneesh left a comment

Choose a reason for hiding this comment

sanyamsinghal commented Jan 6, 2025 •

edited

Loading

sanyamsinghal commented Jan 6, 2025 •

edited

Loading