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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Feb 4, 2025
+45 −0
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6322ed5
Modified callhome to include sizing in assess migration payload
atharva-kurhade-yb Jan 31, 2025
8797e85
Made the suggested naming changes and removed FailureReasoning
atharva-kurhade-yb Feb 3, 2025
141710a
Modified the diagnostics_test.go and added the newly created struct.
atharva-kurhade-yb Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions yb-voyager/cmd/assessMigrationCommand.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,23 @@ func packAndSendAssessMigrationPayload(status string, errMsg string) {
obfuscatedIssues = append(obfuscatedIssues, obfuscatedIssue)
}

sizingAssessmentReport := callhome.SizingCallhome{
NumColocatedTables: len(assessmentReport.Sizing.SizingRecommendation.ColocatedTables),
ColocatedReasoning: assessmentReport.Sizing.SizingRecommendation.ColocatedReasoning,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

NumShardedTables: len(assessmentReport.Sizing.SizingRecommendation.ShardedTables),
NumNodes: assessmentReport.Sizing.SizingRecommendation.NumNodes,
VCPUsPerInstance: assessmentReport.Sizing.SizingRecommendation.VCPUsPerInstance,
MemoryPerInstance: assessmentReport.Sizing.SizingRecommendation.MemoryPerInstance,
OptimalSelectConnectionsPerNode: assessmentReport.Sizing.SizingRecommendation.OptimalSelectConnectionsPerNode,
OptimalInsertConnectionsPerNode: assessmentReport.Sizing.SizingRecommendation.OptimalInsertConnectionsPerNode,
EstimatedTimeInMinForImport: assessmentReport.Sizing.SizingRecommendation.EstimatedTimeInMinForImport,
ParallelVoyagerJobs: assessmentReport.Sizing.SizingRecommendation.ParallelVoyagerJobs,
}

assessPayload := callhome.AssessMigrationPhasePayload{
PayloadVersion: callhome.ASSESS_MIGRATION_CALLHOME_PAYLOAD_VERSION,
TargetDBVersion: assessmentReport.TargetDBVersion,
Sizing: &sizingAssessmentReport,
MigrationComplexity: assessmentReport.MigrationComplexity,
MigrationComplexityExplanation: assessmentReport.MigrationComplexityExplanation,
SchemaSummary: callhome.MarshalledJsonString(schemaSummaryCopy),
Expand Down
14 changes: 14 additions & 0 deletions yb-voyager/src/callhome/diagnostics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Contributor

@priyanshi-yb priyanshi-yb Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok thank you for the review.

MigrationComplexity string `json:"migration_complexity"`
MigrationComplexityExplanation string `json:"migration_complexity_explanation"`
SchemaSummary string `json:"schema_summary"`
Expand All @@ -121,6 +122,19 @@ type AssessmentIssueCallhome struct {
ObjectType string `json:"object_type"`
}

type SizingCallhome struct {
NumColocatedTables int `json:"num_colocated_tables"`
ColocatedReasoning string `json:"colocated_reasoning"`
NumShardedTables int `json:"num_sharded_tables"`
NumNodes float64 `json:"num_nodes"`
VCPUsPerInstance int `json:"vcpus_per_instance"`
MemoryPerInstance int `json:"memory_per_instance"`
OptimalSelectConnectionsPerNode int64 `json:"optimial_select_connections_per_node"`
OptimalInsertConnectionsPerNode int64 `json:"optimial_insert_connections_per_node"`
EstimatedTimeInMinForImport float64 `json:"estimated_time_in_min_for_import"`
ParallelVoyagerJobs float64 `json:"parallel_voyager_jobs"`
}

type ObjectSizingStats struct {
SchemaName string `json:"schema_name,omitempty"`
ObjectName string `json:"object_name"`
Expand Down
17 changes: 17 additions & 0 deletions yb-voyager/src/callhome/diagnostics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ func TestCallhomeStructs(t *testing.T) {
expectedType: struct {
PayloadVersion string `json:"payload_version"`
TargetDBVersion *ybversion.YBVersion `json:"target_db_version"`
Sizing *SizingCallhome `json:"sizing"`
MigrationComplexity string `json:"migration_complexity"`
MigrationComplexityExplanation string `json:"migration_complexity_explanation"`
SchemaSummary string `json:"schema_summary"`
Expand All @@ -100,6 +101,22 @@ func TestCallhomeStructs(t *testing.T) {
ObjectType string `json:"object_type"`
}{},
},
{
name: "Validate SizingCallhome Struct Definition",
actualType: reflect.TypeOf(SizingCallhome{}),
expectedType: struct {
NumColocatedTables int `json:"num_colocated_tables"`
ColocatedReasoning string `json:"colocated_reasoning"`
NumShardedTables int `json:"num_sharded_tables"`
NumNodes float64 `json:"num_nodes"`
VCPUsPerInstance int `json:"vcpus_per_instance"`
MemoryPerInstance int `json:"memory_per_instance"`
OptimalSelectConnectionsPerNode int64 `json:"optimial_select_connections_per_node"`
OptimalInsertConnectionsPerNode int64 `json:"optimial_insert_connections_per_node"`
EstimatedTimeInMinForImport float64 `json:"estimated_time_in_min_for_import"`
ParallelVoyagerJobs float64 `json:"parallel_voyager_jobs"`
}{},
},
{
name: "Validate AssessMigrationBulkPhasePayload Struct Definition",
actualType: reflect.TypeOf(AssessMigrationBulkPhasePayload{}),
Expand Down