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.

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

Minimum validation for multitenant formations #5199

Merged
merged 14 commits into from
Mar 25, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
129 changes: 107 additions & 22 deletions ibm/service/database/resource_ibm_database.go
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,16 @@
Computed: true,
Description: "Can memory scale down as well as up.",
},
"cpu_enforcement_ratio_ceiling_mb": {
Type: schema.TypeInt,
Optional: true,
Description: "The amount of memory required before the cpu/memory ratio is no longer enforced. (multitenant only).",
},
"cpu_enforcement_ratio_mb": {
Type: schema.TypeInt,
Optional: true,
Description: "The maximum memory allowed per CPU until the ratio ceiling is reached. (multitenant only).",
},
},
},
},
Expand Down Expand Up @@ -987,21 +997,23 @@
}

type GroupResource struct {
Units string
Allocation int
Minimum int
Maximum int
StepSize int
IsAdjustable bool
IsOptional bool
CanScaleDown bool
Units string
Allocation int
Minimum int
Maximum int
StepSize int
IsAdjustable bool
IsOptional bool
CanScaleDown bool
CPUEnforcementRatioCeilingMb int
CPUEnforcementRatioMb int
}

type HostFlavorGroupResource struct {
ID string
}

func getDefaultScalingGroups(_service string, _plan string, meta interface{}) (groups []clouddatabasesv5.Group, err error) {
func getDefaultScalingGroups(_service string, _plan string, _hostFlavor string, meta interface{}) (groups []clouddatabasesv5.Group, err error) {
cloudDatabasesClient, err := meta.(conns.ClientSession).CloudDatabasesV5()
if err != nil {
return groups, fmt.Errorf("[ERROR] Error getting database client settings: %s", err)
Expand Down Expand Up @@ -1033,6 +1045,9 @@
}

getDefaultScalingGroupsOptions := cloudDatabasesClient.NewGetDefaultScalingGroupsOptions(service)
if _hostFlavor != "" {
getDefaultScalingGroupsOptions.SetHostFlavor(_hostFlavor)

Check failure on line 1049 in ibm/service/database/resource_ibm_database.go

View workflow job for this annotation

GitHub Actions / Build

getDefaultScalingGroupsOptions.SetHostFlavor undefined (type *clouddatabasesv5.GetDefaultScalingGroupsOptions has no field or method SetHostFlavor)
}

getDefaultScalingGroupsResponse, _, err := cloudDatabasesClient.GetDefaultScalingGroups(getDefaultScalingGroupsOptions)
if err != nil {
Expand All @@ -1043,7 +1058,7 @@
}

func getInitialNodeCount(service string, plan string, meta interface{}) (int, error) {
groups, err := getDefaultScalingGroups(service, plan, meta)
groups, err := getDefaultScalingGroups(service, plan, "", meta)

if err != nil {
return 0, err
Expand Down Expand Up @@ -2702,14 +2717,16 @@
}

group.Memory = &GroupResource{
Units: *g.Memory.Units,
Allocation: int(*g.Memory.AllocationMb),
Minimum: int(*g.Memory.MinimumMb),
Maximum: int(*g.Memory.MaximumMb),
StepSize: int(*g.Memory.StepSizeMb),
IsAdjustable: *g.Memory.IsAdjustable,
IsOptional: *g.Memory.IsOptional,
CanScaleDown: *g.Memory.CanScaleDown,
Units: *g.Memory.Units,
Allocation: int(*g.Memory.AllocationMb),
Minimum: int(*g.Memory.MinimumMb),
Maximum: int(*g.Memory.MaximumMb),
StepSize: int(*g.Memory.StepSizeMb),
IsAdjustable: *g.Memory.IsAdjustable,
IsOptional: *g.Memory.IsOptional,
CanScaleDown: *g.Memory.CanScaleDown,
CPUEnforcementRatioCeilingMb: getCPUEnforcementRatioCeilingMb(g.Memory),
CPUEnforcementRatioMb: getCPUEnforcementRatioMb(g.Memory),
}

group.Disk = &GroupResource{
Expand Down Expand Up @@ -2745,6 +2762,22 @@
return groups
}

func getCPUEnforcementRatioCeilingMb(groupMemory *clouddatabasesv5.GroupMemory) int {
if groupMemory.CPUEnforcementRatioCeilingMb != nil {

Check failure on line 2766 in ibm/service/database/resource_ibm_database.go

View workflow job for this annotation

GitHub Actions / Build

groupMemory.CPUEnforcementRatioCeilingMb undefined (type *clouddatabasesv5.GroupMemory has no field or method CPUEnforcementRatioCeilingMb)
return int(*groupMemory.CPUEnforcementRatioCeilingMb)

Check failure on line 2767 in ibm/service/database/resource_ibm_database.go

View workflow job for this annotation

GitHub Actions / Build

groupMemory.CPUEnforcementRatioCeilingMb undefined (type *clouddatabasesv5.GroupMemory has no field or method CPUEnforcementRatioCeilingMb)
}

return 0
}

func getCPUEnforcementRatioMb(groupMemory *clouddatabasesv5.GroupMemory) int {
if groupMemory.CPUEnforcementRatioMb != nil {

Check failure on line 2774 in ibm/service/database/resource_ibm_database.go

View workflow job for this annotation

GitHub Actions / Build

groupMemory.CPUEnforcementRatioMb undefined (type *clouddatabasesv5.GroupMemory has no field or method CPUEnforcementRatioMb)
return int(*groupMemory.CPUEnforcementRatioMb)

Check failure on line 2775 in ibm/service/database/resource_ibm_database.go

View workflow job for this annotation

GitHub Actions / Build

groupMemory.CPUEnforcementRatioMb undefined (type *clouddatabasesv5.GroupMemory has no field or method CPUEnforcementRatioMb)
}

return 0
}

func expandGroups(_groups []interface{}) []*Group {
if len(_groups) == 0 {
return nil
Expand Down Expand Up @@ -2837,6 +2870,39 @@
return nil
}

func validateMultitenantMemoryCpu(groupId string, resourceName string, resourceDefaults *Group, group *Group) error {

Choose a reason for hiding this comment

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

Not sure what Go conventions are (just learning), but you might be able to put guard clauses in here to simplify the rest of your code. Like in the first line of this method:

if group.CPU != nil {
  return nil
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need resourceName parameter here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope! Thank you for catching that πŸ˜„

// TODO: Replace this with resourceDefaults.Memory.CPUEnforcementRatioCeiling when it is fixed
cpuEnforcementRatioCeiling := 16384
Copy link
Collaborator

Choose a reason for hiding this comment

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

When will this fix go in? Will that be before merging this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@obai-1 This fix won't go in until the next release cut and deployment, so we're looking at around May.

This PR will be merged sometime thing week. So that's why we gotta hard code the cpuEnforcementRatioCeiling for now.

cpuEnforcmentRationMb := resourceDefaults.Memory.CPUEnforcementRatioMb

// This means the CPUEnforcementRatioMb was not sent, which means we are dealing with a non-multitenant going to a multitenat
if cpuEnforcmentRationMb == 0 {
cpuEnforcmentRationMb = 8192
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

if cpuEnforcmentRationMb is coming from resourceDefaults is this logic required here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@obai-1 There is an edge case, when a user is has an existing databases and is going from non-multitenant to multitenant. In that case, we use the deployments/:id/groups API endpoint to get the database current configuration.

However, the deployments/:id/groups does not have the host_flavor query param. In other words, we do not get the cpuEnforcementRatioCeiling or the cpuEnforcementRatioMb.

So we have one of 2 options:

  1. Call the deployables/{type}/group. Process the result, extract the ratios from it, and pass those into the function as well. This is a large amount of code that will have to be removed regardless as the enforcement ratios and query parameter are going to go away soonish, per the following "when the old hosting model goes away, so do these query string parameters".
  2. Account for the edge case by hard-coding the cpuEnforcementRatio in the event the API didn't return it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

as the enforcement ratios and query parameter are going to go away soonish

Hmm.. unless I am missing something, they are not going away anytime soon..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also hardcoding these numbers here is not safe. Those numbers could change down the line, and could also be db specific. Remember once we release it out to the wild, it is hard to take it back :)


if resourceDefaults.Members == nil {
return nil
}

if group.Memory.Allocation < resourceDefaults.Memory.Minimum/resourceDefaults.Members.Allocation {
return fmt.Errorf("We were unable to complete your request: group.memory requires a minimum of %d megabytes. Try again with valid values or contact support if the issue persists.", resourceDefaults.Memory.Minimum/resourceDefaults.Members.Allocation)
}

if group.CPU == nil {
return nil
}

if group.CPU != nil && group.CPU.Allocation > 2 {
Copy link
Collaborator

@obai-1 obai-1 Mar 21, 2024

Choose a reason for hiding this comment

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

should be something like group.CPU.Allocation >= EnforcementRatioCeiling/EnforcementRatio

return nil
}

if group.CPU != nil && group.Memory.Allocation*resourceDefaults.Members.Allocation < cpuEnforcementRatioCeiling*resourceDefaults.Members.Allocation && group.Memory.Allocation*resourceDefaults.Members.Allocation > group.CPU.Allocation*cpuEnforcmentRationMb {
return fmt.Errorf("We were unable to complete your request: group.memory %d with group.cpu %d is not valid. Try again with valid values or contact support if the issue persists.", group.Memory.Allocation, group.CPU.Allocation)
}

return nil
}

func validateGroupsDiff(_ context.Context, diff *schema.ResourceDiff, meta interface{}) (err error) {
instanceID := diff.Id()
service := diff.Get("service").(string)
Expand All @@ -2846,11 +2912,23 @@
var currentGroups []Group
var groupList []clouddatabasesv5.Group
var groupIds []string
groups := expandGroups(group.(*schema.Set).List())
var memberGroup *Group
for _, g := range groups {
if g.ID == "member" {
memberGroup = g
break
}
}

if instanceID != "" {
groupList, err = getGroups(instanceID, meta)
} else {
groupList, err = getDefaultScalingGroups(service, plan, meta)
if memberGroup.HostFlavor != nil {
groupList, err = getDefaultScalingGroups(service, plan, memberGroup.HostFlavor.ID, meta)
} else {
groupList, err = getDefaultScalingGroups(service, plan, "", meta)
}
}

if err != nil {
Expand Down Expand Up @@ -2892,8 +2970,8 @@
// set current nodeCount
nodeCount := groupDefaults.Members.Allocation

if group.Members != nil {
err = validateGroupScaling(groupId, "members", group.Members.Allocation, groupDefaults.Members, 1)
if group.Memory != nil && group.HostFlavor != nil && group.HostFlavor.ID != "" && group.HostFlavor.ID == "multitenant" && groupDefaults.Memory.CPUEnforcementRatioCeilingMb != 0 && groupDefaults.Memory.CPUEnforcementRatioMb != 0 {
err = validateMultitenantMemoryCpu(groupId, "memory", groupDefaults, group)
if err != nil {
return err
}
Expand All @@ -2906,6 +2984,13 @@
}
}

if group.Members != nil {
err = validateGroupScaling(groupId, "members", group.Members.Allocation, groupDefaults.Members, 1)
if err != nil {
return err
}
}

if group.Memory != nil {
err = validateGroupScaling(groupId, "memory", group.Memory.Allocation, groupDefaults.Memory, nodeCount)
if err != nil {
Expand Down Expand Up @@ -2972,7 +3057,7 @@

// TODO: Use Capability API
// RBAC roles supported for Redis 6.0 and above
if service == "databases-for-redis" && !(version > 0 && version < 6) {
if (service == "databases-for-redis") && !(version > 0 && version < 6) {
err = change.New.ValidateRBACRole()
} else if service == "databases-for-mongodb" && change.New.Type == "ops_manager" {
err = change.New.ValidateOpsManagerRole()
Expand Down
32 changes: 16 additions & 16 deletions ibm/service/database/resource_ibm_database_elasticsearch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func TestAccIBMDatabaseInstance_Elasticsearch_Basic(t *testing.T) {
resource.TestCheckResourceAttr(name, "service", "databases-for-elasticsearch"),
resource.TestCheckResourceAttr(name, "plan", "standard"),
resource.TestCheckResourceAttr(name, "location", acc.Region()),
resource.TestCheckResourceAttr(name, "groups.0.memory.0.allocation_mb", "6144"),
resource.TestCheckResourceAttr(name, "groups.0.memory.0.allocation_mb", "12288"),
resource.TestCheckResourceAttr(name, "groups.0.disk.0.allocation_mb", "18432"),
resource.TestCheckResourceAttr(name, "users.#", "0"),
resource.TestCheckResourceAttr(name, "connectionstrings.#", "1"),
Expand Down Expand Up @@ -213,7 +213,7 @@ func TestAccIBMDatabaseInstance_Elasticsearch_Group(t *testing.T) {
resource.TestCheckResourceAttr(name, "plan", "standard"),
resource.TestCheckResourceAttr(name, "location", acc.Region()),
resource.TestCheckResourceAttr(name, "groups.0.count", "3"),
resource.TestCheckResourceAttr(name, "groups.0.memory.0.allocation_mb", "3072"),
resource.TestCheckResourceAttr(name, "groups.0.memory.0.allocation_mb", "12288"),
resource.TestCheckResourceAttr(name, "groups.0.disk.0.allocation_mb", "18432"),
resource.TestCheckResourceAttr(name, "groups.0.cpu.0.allocation_count", "9"),
resource.TestCheckResourceAttr(name, "allowlist.#", "2"),
Expand All @@ -231,7 +231,7 @@ func TestAccIBMDatabaseInstance_Elasticsearch_Group(t *testing.T) {
resource.TestCheckResourceAttr(name, "plan", "standard"),
resource.TestCheckResourceAttr(name, "location", acc.Region()),
resource.TestCheckResourceAttr(name, "groups.0.count", "3"),
resource.TestCheckResourceAttr(name, "groups.0.memory.0.allocation_mb", "3072"),
resource.TestCheckResourceAttr(name, "groups.0.memory.0.allocation_mb", "12288"),
resource.TestCheckResourceAttr(name, "groups.0.disk.0.allocation_mb", "18432"),
resource.TestCheckResourceAttr(name, "groups.0.cpu.0.allocation_count", "9"),
resource.TestCheckResourceAttr(name, "allowlist.#", "0"),
Expand All @@ -248,7 +248,7 @@ func TestAccIBMDatabaseInstance_Elasticsearch_Group(t *testing.T) {
resource.TestCheckResourceAttr(name, "plan", "standard"),
resource.TestCheckResourceAttr(name, "location", acc.Region()),
resource.TestCheckResourceAttr(name, "groups.0.count", "4"),
resource.TestCheckResourceAttr(name, "groups.0.memory.0.allocation_mb", "4096"),
resource.TestCheckResourceAttr(name, "groups.0.memory.0.allocation_mb", "15360"),
resource.TestCheckResourceAttr(name, "groups.0.disk.0.allocation_mb", "24576"),
resource.TestCheckResourceAttr(name, "groups.0.cpu.0.allocation_count", "12"),
resource.TestCheckResourceAttr(name, "allowlist.#", "0"),
Expand Down Expand Up @@ -329,7 +329,7 @@ func testAccCheckIBMDatabaseInstanceElasticsearchBasic(databaseResourceGroup str
group {
group_id = "member"
memory {
allocation_mb = 2048
allocation_mb = 4096
}
host_flavor {
id = "multitenant"
Expand Down Expand Up @@ -379,7 +379,7 @@ func testAccCheckIBMDatabaseInstanceElasticsearchFullyspecified(databaseResource
group {
group_id = "member"
memory {
allocation_mb = 2048
allocation_mb = 4096
}
host_flavor {
id = "multitenant"
Expand Down Expand Up @@ -414,7 +414,7 @@ func testAccCheckIBMDatabaseInstanceElasticsearchReduced(databaseResourceGroup s
group {
group_id = "member"
memory {
allocation_mb = 2048
allocation_mb = 4096
}
host_flavor {
id = "multitenant"
Expand Down Expand Up @@ -449,7 +449,7 @@ func testAccCheckIBMDatabaseInstanceElasticsearchGroupMigration(databaseResource
group_id = "member"

memory {
allocation_mb = 2048
allocation_mb = 4096
}
host_flavor {
id = "multitenant"
Expand Down Expand Up @@ -489,7 +489,7 @@ func testAccCheckIBMDatabaseInstanceElasticsearchNodeBasic(databaseResourceGroup
allocation_count = 3
}
memory {
allocation_mb = 1024
allocation_mb = 4096
}
disk {
allocation_mb = 5120
Expand Down Expand Up @@ -539,7 +539,7 @@ func testAccCheckIBMDatabaseInstanceElasticsearchNodeFullyspecified(databaseReso
allocation_count = 3
}
memory {
allocation_mb = 1024
allocation_mb = 5120
}
disk {
allocation_mb = 6144
Expand Down Expand Up @@ -597,7 +597,7 @@ func testAccCheckIBMDatabaseInstanceElasticsearchNodeReduced(databaseResourceGro
allocation_count = 3
}
memory {
allocation_mb = 1024
allocation_mb = 4096
}
disk {
allocation_mb = 6144
Expand Down Expand Up @@ -639,7 +639,7 @@ func testAccCheckIBMDatabaseInstanceElasticsearchNodeScaleOut(databaseResourceGr
allocation_count = 4
}
memory {
allocation_mb = 1024
allocation_mb = 4096
}
disk {
allocation_mb = 6144
Expand Down Expand Up @@ -682,7 +682,7 @@ func testAccCheckIBMDatabaseInstanceElasticsearchGroupBasic(databaseResourceGrou
allocation_count = 3
}
memory {
allocation_mb = 1024
allocation_mb = 4096
}
disk {
allocation_mb = 5120
Expand Down Expand Up @@ -734,7 +734,7 @@ func testAccCheckIBMDatabaseInstanceElasticsearchGroupFullyspecified(databaseRes
allocation_count = 3
}
memory {
allocation_mb = 1024
allocation_mb = 4096
}
disk {
allocation_mb = 6144
Expand Down Expand Up @@ -794,7 +794,7 @@ func testAccCheckIBMDatabaseInstanceElasticsearchGroupReduced(databaseResourceGr
allocation_count = 3
}
memory {
allocation_mb = 1024
allocation_mb = 4096
}
disk {
allocation_mb = 6144
Expand Down Expand Up @@ -837,7 +837,7 @@ func testAccCheckIBMDatabaseInstanceElasticsearchGroupScaleOut(databaseResourceG
allocation_count = 4
}
memory {
allocation_mb = 1024
allocation_mb = 4096
}
disk {
allocation_mb = 6144
Expand Down
8 changes: 4 additions & 4 deletions ibm/service/database/resource_ibm_database_etcd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestAccIBMDatabaseInstance_Etcd_Basic(t *testing.T) {
resource.TestCheckResourceAttr(name, "plan", "standard"),
resource.TestCheckResourceAttr(name, "location", acc.Region()),
resource.TestCheckResourceAttr(name, "adminuser", "root"),
resource.TestCheckResourceAttr(name, "groups.0.memory.0.allocation_mb", "9216"),
resource.TestCheckResourceAttr(name, "groups.0.memory.0.allocation_mb", "12288"),
resource.TestCheckResourceAttr(name, "groups.0.disk.0.allocation_mb", "184320"),
resource.TestCheckResourceAttr(name, "allowlist.#", "1"),
resource.TestCheckResourceAttr(name, "users.#", "1"),
Expand Down Expand Up @@ -69,7 +69,7 @@ func TestAccIBMDatabaseInstance_Etcd_Basic(t *testing.T) {
resource.TestCheckResourceAttr(name, "service", "databases-for-etcd"),
resource.TestCheckResourceAttr(name, "plan", "standard"),
resource.TestCheckResourceAttr(name, "location", acc.Region()),
resource.TestCheckResourceAttr(name, "groups.0.memory.0.allocation_mb", "9216"),
resource.TestCheckResourceAttr(name, "groups.0.memory.0.allocation_mb", "12288"),
resource.TestCheckResourceAttr(name, "groups.0.disk.0.allocation_mb", "193536"),
resource.TestCheckResourceAttr(name, "allowlist.#", "0"),
resource.TestCheckResourceAttr(name, "users.#", "0"),
Expand Down Expand Up @@ -135,7 +135,7 @@ func testAccCheckIBMDatabaseInstanceEtcdBasic(databaseResourceGroup string, name
group {
group_id = "member"
memory {
allocation_mb = 3072
allocation_mb = 4096
}
host_flavor {
id = "multitenant"
Expand Down Expand Up @@ -217,7 +217,7 @@ func testAccCheckIBMDatabaseInstanceEtcdReduced(databaseResourceGroup string, na
group {
group_id = "member"
memory {
allocation_mb = 3072
allocation_mb = 4096
}
host_flavor {
id = "multitenant"
Expand Down