Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 7 additions & 4 deletions providers/gcp/services/computeengine/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,12 @@ func TestRootCUDUnsupportedShapesAndAmounts(t *testing.T) {
op := input.Content.OperationGroups[0].Operations[0]
fields := op.GetValue().GetStructValue().Fields
list := fields["resources"].GetListValue()
badAmount := false
skipRow := false
switch name {
case "root":
// A non-object root Value is a malformed plan: skipped like a
// bad amount, so the batch keeps its later valid rows.
skipRow = true
op.PathValue = &recommenderpb.Operation_Value{Value: structpb.NewNullValue()}
case "resources":
fields["resources"] = structpb.NewStringValue("bad")
Expand All @@ -98,7 +101,7 @@ func TestRootCUDUnsupportedShapesAndAmounts(t *testing.T) {
case "UNKNOWN", "ACCELERATOR", "LOCAL_SSD":
list.Values[0].GetStructValue().Fields["type"] = structpb.NewStringValue(name)
default:
badAmount = true
skipRow = true
list.Values[0].GetStructValue().Fields["amount"] = map[string]*structpb.Value{"bad amount": structpb.NewStringValue("6Gi"), "fractional": structpb.NewNumberValue(1.5), "zero": structpb.NewNumberValue(0), "overflow": structpb.NewStringValue("9223372036854775808"), "null amount": structpb.NewNullValue()}[name]
}
ctx := context.Background()
Expand All @@ -107,8 +110,8 @@ func TestRootCUDUnsupportedShapesAndAmounts(t *testing.T) {
service := &MockCommitmentsService{operation: &MockOperation{}}
client.SetCommitmentsService(service)
client.SetRecommenderClient(&MockRecommenderClient{iterator: &MockRecommenderIterator{recommendations: []*recommenderpb.Recommendation{input}}})
recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{})
if badAmount {
recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{ExperimentalGCPPlanTerm: true})
if skipRow {
require.NoError(t, err)
} else {
require.Error(t, err)
Expand Down
2 changes: 1 addition & 1 deletion providers/gcp/services/computeengine/commitment_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ func rootPlanTerm(root *recommenderpb.Operation) (term string, found bool, err e
}
object := root.GetValue().GetStructValue()
if object == nil {
return "", false, fmt.Errorf("root Commitment value must be an object")
return "", false, fmt.Errorf("%w: root Commitment value must be an object", errBadPlan)
}
value, present := object.Fields["plan"]
if !present {
Expand Down
20 changes: 20 additions & 0 deletions providers/gcp/services/computeengine/plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,26 @@ func TestRecommendationPlanConflictKeepsGoodRecommendation(t *testing.T) {
assert.Empty(t, service.insertReqs, "listing must not insert")
}

// A root Commitment whose Value is not an object is a malformed plan and must be
// skipped like any other bad plan, not abort the batch before later valid
// recommendations are processed (issue #291).
func TestRecommendationMalformedRootValueKeepsGoodRecommendation(t *testing.T) {
bad := rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "TWELVE_MONTH")
bad.Name = "bad-root-value"
bad.Content.OperationGroups[0].Operations[0].PathValue = &recommenderpb.Operation_Value{
Value: structpb.NewStringValue("not-an-object"),
}
good := rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "THIRTY_SIX_MONTH")
good.Name = "good"

client, service := planClient(t, bad, good)
recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{ExperimentalGCPPlanTerm: true})
require.NoError(t, err, "a malformed root value must be skipped, not abort the batch")
require.Len(t, recs, 1)
assert.Equal(t, "3yr", recs[0].Term)
assert.Empty(t, service.insertReqs, "listing must not insert")
}

// Mutation probe: the plan-to-term mapping must not be swappable.
func TestRootCommitmentTermMapping(t *testing.T) {
for plan, want := range map[string]string{"TWELVE_MONTH": "1yr", "THIRTY_SIX_MONTH": "3yr"} {
Expand Down
Loading