[DB-14479] In assess-migration payload, send sizing and estimation information #2282

atharva-kurhade-yb · 2025-01-31T07:59:44Z

Describe the changes in this pull request

Modified callhome and assess migration command to include sizing assessment report information in assess migration payload

Sample Payload JSON :-
{
"error": "",
"sizing": {
"num_nodes": 3,
"num_sharded_tables": 0,
"vcpus_per_instance": 4,
"colocated_reasoning": "Recommended instance type with 4 vCPU and 16 GiB memory could fit 1 objects (1 tables/materialized views and 0 explicit/implicit indexes) with 0.00 MB size and throughput requirement of 0 reads/sec and 0 writes/sec as colocated. Non leaf partition tables/indexes and unsupported tables/indexes were not considered.",
"memory_per_instance": 16,
"num_colocated_tables": 1,
"parallel_voyager_jobs": 1,
"estimated_time_in_min_for_import": 1,
"optimial_insert_connections_per_node": 12,
"optimial_select_connections_per_node": 8
},
"iops_interval": 0,
"schema_summary": "{"Description":"","DbName":"","SchemaNames":null,"DbVersion":"","DatabaseObjects":[{"ObjectType":"SCHEMA","TotalCount":1,"InvalidCount":0,"ObjectNames":""},{"ObjectType":"TABLE","TotalCount":1,"InvalidCount":0,"ObjectNames":""}]}",
"payload_version": "1.0",
"assessment_issues": "null",
"target_db_version": "2024.2.0.0",
"index_sizing_stats": "null",
"table_sizing_stats": "[{"object_name":"XXXXX","reads_per_second":0,"writes_per_second":0,"size_in_bytes":8192}]",
"source_connectivity": true,
"migration_complexity": "LOW",
"migration_complexity_explanation": "Found 0 Level 1 issue(s) and 0 Level 2 issue(s), resulting in LOW migration complexity"
}

Describe if there are any user-facing changes

How was this pull request tested?

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 (Should not cause any issues as only a new field has been added)
YugabyteD Tables	Yes/No
TargetDB Metadata Tables	Yes/No

makalaaneesh · 2025-01-31T08:53:27Z

yb-voyager/cmd/assessMigrationCommand.go

@@ -191,9 +191,28 @@ func packAndSendAssessMigrationPayload(status string, errMsg string) {
 		obfuscatedIssues = append(obfuscatedIssues, obfuscatedIssue)
 	}

+	sizingRecommendation := callhome.SizingRecommendationCallhome{
+		NumColocatedTables:              len(assessmentReport.Sizing.SizingRecommendation.ColocatedTables),
+		ColocatedReasoning:              assessmentReport.Sizing.SizingRecommendation.ColocatedReasoning,


Pls verify that ColocatedReasoning and FailureReasoning do not include any sensitive information.
You can look at the code to get an idea of how these fields are set.

ColocatedReasoning does not include sensitive information as it mostly deals with number of colocated and sharded objects and their size and throughput requirements
Example : Recommended instance type with 4 vCPU and 16 GiB memory could fit 1 objects (1 tables/materialized views and 0 explicit/implicit indexes) with 0.00 MB size and throughput requirement of 0 reads/sec and 0 writes/sec as colocated. Non leaf partition tables/indexes and unsupported tables/indexes were not considered.

But Failure Reasoning might contain sensitive information as it catches all the errors that are thrown during sizing assessment.

Let's also bring this up in the standup that we won't be sending the failure reasoning with this change as it includes error msgs. Please also file a task for this, we have to do it at some point after redacting the error msgs.

makalaaneesh

In the PR description, pls add a sample json value of the entire payload that gets sent to callhome with your changes.

makalaaneesh · 2025-01-31T08:54:39Z

yb-voyager/src/callhome/diagnostics.go

-	IopsInterval                   int64                `json:"iops_interval"`
+	PayloadVersion                 string                          `json:"payload_version"`
+	TargetDBVersion                *ybversion.YBVersion            `json:"target_db_version"`
+	SizingAssessmentReport         *SizingAssessmentReportCallhome `json:"sizing_assessment_report"`


perhaps just sizing instead of sizing_assessment_report ?

Should I change the variable names as well or just the json key?

priyanshi-yb · 2025-02-03T07:39:20Z

yb-voyager/src/callhome/diagnostics.go

@@ -101,6 +101,7 @@ var ASSESS_MIGRATION_CALLHOME_PAYLOAD_VERSION = "1.0"
 type AssessMigrationPhasePayload struct {
 	PayloadVersion                 string               `json:"payload_version"`
 	TargetDBVersion                *ybversion.YBVersion `json:"target_db_version"`
+	Sizing                         *SizingCallhome      `json:"sizing"`


As you have added a new field to this structure, tests in diagnostics_test.go must fail, it is to test the changes in any structure in this area, mainly to understand whether the change is voyager upgrade safe between the migrations.
This is just an addition of a new field, so it should be okay, and you can just fix the test.

FYI, please check the table in the PR template whenever you raise a PR it has the check box for the same to update if there is any change to any struct of the voyager state.

In the PR template I would have to set CallhomeJSON to Yes right?

Yes you can, but mention that this is just an additional field so shouldn't affect anything.

Ok thank you for the review.

priyanshi-yb · 2025-02-03T07:42:25Z

yb-voyager/src/callhome/diagnostics.go

+	NumShardedTables                int     `json:"num_sharded_tables"`
+	NumNodes                        float64 `json:"num_nodes"`
+	VCPUsPerInstance                int     `json:"vcpus_per_instance"`
+	MemoryPerInstance               int     `json:"mem_per_instance"`


keep the JSON tag the same as memory_per_instance.

atharva-kurhade-yb · 2025-02-03T08:11:11Z

@priyanshi-yb
I have added changes to diagnostics_test.go and also changed the field name to memory_per_instance.
Please review the changes when you get the time

priyanshi-yb

LGTM

priyanshi-yb · 2025-02-03T12:18:15Z

yb-voyager/cmd/assessMigrationCommand.go

@@ -191,9 +191,28 @@ func packAndSendAssessMigrationPayload(status string, errMsg string) {
 		obfuscatedIssues = append(obfuscatedIssues, obfuscatedIssue)
 	}

+	sizingRecommendation := callhome.SizingRecommendationCallhome{
+		NumColocatedTables:              len(assessmentReport.Sizing.SizingRecommendation.ColocatedTables),
+		ColocatedReasoning:              assessmentReport.Sizing.SizingRecommendation.ColocatedReasoning,


Let's also bring this up in the standup that we won't be sending the failure reasoning with this change as it includes error msgs. Please also file a task for this, we have to do it at some point after redacting the error msgs.

atharva-kurhade-yb · 2025-02-04T05:56:48Z

Also ran a jenkins pipeline to run Oracle tests:-
https://jenkins.dev.yugabyte.com/job/users/job/yb-voyager-testing/job/yb-voyager-testing-pipeline/4651/

atharva-kurhade-yb requested review from makalaaneesh and sanyamsinghal January 31, 2025 08:01

makalaaneesh reviewed Jan 31, 2025

View reviewed changes

atharva-kurhade-yb requested review from makalaaneesh and priyanshi-yb January 31, 2025 10:18

priyanshi-yb reviewed Feb 3, 2025

View reviewed changes

atharva-kurhade-yb requested a review from priyanshi-yb February 3, 2025 08:14

atharva-kurhade-yb force-pushed the atharva/sizing_and_estimation_for_assess_migration_payload branch from 01636c1 to 78ab65b Compare February 3, 2025 09:21

priyanshi-yb approved these changes Feb 3, 2025

View reviewed changes

atharva-kurhade-yb added 3 commits February 4, 2025 11:21

Modified callhome to include sizing in assess migration payload

1c85b5d

Made the suggested naming changes and removed FailureReasoning

d197055

Modified the diagnostics_test.go and added the newly created struct.

f56ca67

atharva-kurhade-yb force-pushed the atharva/sizing_and_estimation_for_assess_migration_payload branch from 78ab65b to f56ca67 Compare February 4, 2025 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DB-14479] In assess-migration payload, send sizing and estimation information #2282

[DB-14479] In assess-migration payload, send sizing and estimation information #2282

atharva-kurhade-yb commented Jan 31, 2025 •

edited

Loading

makalaaneesh Jan 31, 2025

atharva-kurhade-yb Jan 31, 2025

priyanshi-yb Feb 3, 2025

makalaaneesh left a comment

makalaaneesh Jan 31, 2025

atharva-kurhade-yb Jan 31, 2025

priyanshi-yb Feb 3, 2025 •

edited

Loading

atharva-kurhade-yb Feb 3, 2025

priyanshi-yb Feb 3, 2025

atharva-kurhade-yb Feb 3, 2025

priyanshi-yb Feb 3, 2025

atharva-kurhade-yb commented Feb 3, 2025

priyanshi-yb left a comment

priyanshi-yb Feb 3, 2025

atharva-kurhade-yb commented Feb 4, 2025

[DB-14479] In assess-migration payload, send sizing and estimation information #2282

Are you sure you want to change the base?

[DB-14479] In assess-migration payload, send sizing and estimation information #2282

Conversation

atharva-kurhade-yb commented Jan 31, 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

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

priyanshi-yb Feb 3, 2025 • 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

atharva-kurhade-yb commented Feb 3, 2025

priyanshi-yb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atharva-kurhade-yb commented Feb 4, 2025

atharva-kurhade-yb commented Jan 31, 2025 •

edited

Loading

priyanshi-yb Feb 3, 2025 •

edited

Loading