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

Conversation

omaraibrahim
Copy link
Collaborator

@omaraibrahim omaraibrahim commented Mar 13, 2024

Community Note

  • Please vote on this pull request by adding a πŸ‘ reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000

Testing Done

Provisioning with Redis

Provisioning multitenant Redis with insufficient memory

data "ibm_resource_group" "group" {
  name = "default"
}

resource "ibm_database" "test_acc" {
  resource_group_id = data.ibm_resource_group.group.id
  name              = "min-compute-redis-03-13-two"
  service           = "databases-for-redis"
  plan              = "standard"
  location          = "us-south"
  adminpassword     = "password123456789"
  group {
    group_id = "member"
    memory {
      allocation_mb = 1024
    }
    host_flavor {
      id = "multitenant"
    }
  }
  tags              = ["one:two"]
  users {
    name     = "user123"
    password = "password123456789"
  }
}
terraform plan
β•·
β”‚ Warning: Provider development overrides are in effect
β”‚ 
β”‚ The following provider development overrides are set in the CLI configuration:
β”‚  - ibm-cloud/ibm in /Users/omar/.go/bin
β”‚ 
β”‚ The behavior may therefore not match any released version of the provider and applying changes may cause the state to become incompatible with published releases.
β•΅
data.ibm_resource_group.group: Reading...
data.ibm_resource_group.group: Read complete after 1s [id=eb922ba0717f47589ca26d202c5cc915]

Planning failed. Terraform encountered an error while generating this plan.

β•·
β”‚ Error: 1 error occurred:
β”‚ 	* We were unable to complete your request: group.memory requires a minimum of 4096 megabytes. Try again with valid values or contact support if the issue persists.
β”‚ 
β”‚ 
β”‚ 
β”‚   with ibm_database.test_acc,
β”‚   on main.tf line 5, in resource "ibm_database" "test_acc":
β”‚    5: resource "ibm_database" "test_acc" {

Provisioning multitenant Redis with below-ratio memory

data "ibm_resource_group" "group" {
  name = "default"
}

resource "ibm_database" "test_acc" {
  resource_group_id = data.ibm_resource_group.group.id
  name              = "min-compute-redis-03-13-two"
  service           = "databases-for-redis"
  plan              = "standard"
  location          = "us-south"
  adminpassword     = "password123456789"
  group {
    group_id = "member"
    memory {
      allocation_mb = 5120
    }
    host_flavor {
      id = "multitenant"
    }
    cpu {
      allocation_count = 2
    }
  }
  tags              = ["one:two"]
  users {
    name     = "user123"
    password = "password123456789"
  }
}
terraform plan
β•·
β”‚ Warning: Provider development overrides are in effect
β”‚ 
β”‚ The following provider development overrides are set in the CLI configuration:
β”‚  - ibm-cloud/ibm in /Users/omar/.go/bin
β”‚ 
β”‚ The behavior may therefore not match any released version of the provider and applying changes may cause the state to become incompatible with published releases.
β•΅
data.ibm_resource_group.group: Reading...
data.ibm_resource_group.group: Read complete after 1s [id=eb922ba0717f47589ca26d202c5cc915]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # ibm_database.test_acc will be created
  + resource "ibm_database" "test_acc" {
      + adminpassword           = (sensitive value)
      + adminuser               = (known after apply)
      + configuration_schema    = (known after apply)
      + connectionstrings       = (known after apply)
      + groups                  = (known after apply)
      + guid                    = (known after apply)
      + id                      = (known after apply)
      + location                = "us-south"
      + name                    = "min-compute-redis-03-13-two"
      + plan                    = "standard"
      + resource_controller_url = (known after apply)
      + resource_crn            = (known after apply)
      + resource_group_id       = "eb922ba0717f47589ca26d202c5cc915"
      + resource_group_name     = (known after apply)
      + resource_name           = (known after apply)
      + resource_status         = (known after apply)
      + service                 = "databases-for-redis"
      + service_endpoints       = "public"
      + status                  = (known after apply)
      + tags                    = [
          + "one:two",
        ]
      + version                 = (known after apply)

      + group {
          + group_id = "member"

          + cpu {
              + allocation_count = 2
            }

          + host_flavor {
              + id = "multitenant"
            }

          + memory {
              + allocation_mb = 5120
            }
        }

      + users {
          # At least one attribute in this block is (or was) sensitive,
          # so its contents will not be displayed.
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Provisioning multitenant Redis with exact-ratio memory

data "ibm_resource_group" "group" {
  name = "default"
}

resource "ibm_database" "test_acc" {
  resource_group_id = data.ibm_resource_group.group.id
  name              = "min-compute-redis-03-13-two"
  service           = "databases-for-redis"
  plan              = "standard"
  location          = "us-south"
  adminpassword     = "password123456789"
  group {
    group_id = "member"
    memory {
      allocation_mb = 8192
    }
    host_flavor {
      id = "multitenant"
    }
    cpu {
      allocation_count = 2
    }
  }
  tags              = ["one:two"]
  users {
    name     = "user123"
    password = "password123456789"
  }
}
terraform plan
β•·
β”‚ Warning: Provider development overrides are in effect
β”‚ 
β”‚ The following provider development overrides are set in the CLI configuration:
β”‚  - ibm-cloud/ibm in /Users/omar/.go/bin
β”‚ 
β”‚ The behavior may therefore not match any released version of the provider and applying changes may cause the state to become incompatible with published releases.
β•΅
data.ibm_resource_group.group: Reading...
data.ibm_resource_group.group: Read complete after 1s [id=eb922ba0717f47589ca26d202c5cc915]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # ibm_database.test_acc will be created
  + resource "ibm_database" "test_acc" {
      + adminpassword           = (sensitive value)
      + adminuser               = (known after apply)
      + configuration_schema    = (known after apply)
      + connectionstrings       = (known after apply)
      + groups                  = (known after apply)
      + guid                    = (known after apply)
      + id                      = (known after apply)
      + location                = "us-south"
      + name                    = "min-compute-redis-03-13-two"
      + plan                    = "standard"
      + resource_controller_url = (known after apply)
      + resource_crn            = (known after apply)
      + resource_group_id       = "eb922ba0717f47589ca26d202c5cc915"
      + resource_group_name     = (known after apply)
      + resource_name           = (known after apply)
      + resource_status         = (known after apply)
      + service                 = "databases-for-redis"
      + service_endpoints       = "public"
      + status                  = (known after apply)
      + tags                    = [
          + "one:two",
        ]
      + version                 = (known after apply)

      + group {
          + group_id = "member"

          + cpu {
              + allocation_count = 2
            }

          + host_flavor {
              + id = "multitenant"
            }

          + memory {
              + allocation_mb = 8192
            }
        }

      + users {
          # At least one attribute in this block is (or was) sensitive,
          # so its contents will not be displayed.
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Provisioning multitenant Redis with Invalid Ratio

data "ibm_resource_group" "group" {
  name = "default"
}

resource "ibm_database" "test_acc" {
  resource_group_id = data.ibm_resource_group.group.id
  name              = "min-compute-redis-03-13-two"
  service           = "databases-for-redis"
  plan              = "standard"
  location          = "us-south"
  adminpassword     = "password123456789"
  group {
    group_id = "member"
    memory {
      allocation_mb = 8704
    }
    host_flavor {
      id = "multitenant"
    }
    cpu {
      allocation_count = 1
    }
  }
  tags              = ["one:two"]
  users {
    name     = "user123"
    password = "password123456789"
  }
}
terraform plan
β•·
β”‚ Warning: Provider development overrides are in effect
β”‚ 
β”‚ The following provider development overrides are set in the CLI configuration:
β”‚  - ibm-cloud/ibm in /Users/omar/.go/bin
β”‚ 
β”‚ The behavior may therefore not match any released version of the provider and applying changes may cause the state to become incompatible with published releases.
β•΅
data.ibm_resource_group.group: Reading...
data.ibm_resource_group.group: Read complete after 1s [id=eb922ba0717f47589ca26d202c5cc915]

Planning failed. Terraform encountered an error while generating this plan.

β•·
β”‚ Error: 1 error occurred:
β”‚ 	* The current cpu alloaction of 1 is not valid for your current configuration.
β”‚ 
β”‚ 
β”‚ 
β”‚   with ibm_database.test_acc,
β”‚   on main.tf line 5, in resource "ibm_database" "test_acc":
β”‚    5: resource "ibm_database" "test_acc" {
β”‚ 

Provisioning with RabbitMQ

Provisioning multitenant RabbitMQ with insufficient memory

data "ibm_resource_group" "group" {
  name = "default"
}

resource "ibm_database" "test_acc" {
  resource_group_id = data.ibm_resource_group.group.id
  name              = "min-compute-rabbitmq-03-13-two"
  service           = "messages-for-rabbitmq"
  plan              = "standard"
  location          = "us-south"
  adminpassword     = "password123456789"
  group {
    group_id = "member"
    memory {
      allocation_mb = 1024
    }
    host_flavor {
      id = "multitenant"
    }
  }
  tags              = ["one:two"]
  users {
    name     = "user123"
    password = "password123456789"
  }
}
terraform plan
β•·
β”‚ Warning: Provider development overrides are in effect
β”‚ 
β”‚ The following provider development overrides are set in the CLI configuration:
β”‚  - ibm-cloud/ibm in /Users/omar/.go/bin
β”‚ 
β”‚ The behavior may therefore not match any released version of the provider and applying changes may cause the state to become incompatible with published releases.
β•΅
data.ibm_resource_group.group: Reading...
data.ibm_resource_group.group: Read complete after 2s [id=eb922ba0717f47589ca26d202c5cc915]

Planning failed. Terraform encountered an error while generating this plan.

β•·
β”‚ Error: 1 error occurred:
β”‚ 	* member group memory must be >= 8192 and <= 114688 in increments of 128
β”‚ 
β”‚ 
β”‚ 
β”‚   with ibm_database.test_acc,
β”‚   on main.tf line 5, in resource "ibm_database" "test_acc":
β”‚    5: resource "ibm_database" "test_acc" {
β”‚ 

Scaling

Scaling non-multitenant to multitenant

Initial Non-Multitenant Formation
data "ibm_resource_group" "group" {
  name = "default"
}

resource "ibm_database" "test_acc" {
  resource_group_id = data.ibm_resource_group.group.id
  name              = "min-compute-redis-03-22-four"
  service           = "databases-for-redis"
  plan              = "standard"
  location          = "us-south"
  adminpassword     = "password123456789"
  group {
    group_id = "member"
    memory {
      allocation_mb = 1024
    }
  }
  tags              = ["one:two"]
  users {
    name     = "user123"
    password = "password123456789"
  }
}
Convert to Multitenant Formation with Invalid CPU Ratio
data "ibm_resource_group" "group" {
  name = "default"
}

resource "ibm_database" "test_acc" {
  resource_group_id = data.ibm_resource_group.group.id
  name              = "min-compute-redis-03-22-four"
  service           = "databases-for-redis"
  plan              = "standard"
  location          = "us-south"
  adminpassword     = "password123456789"
  group {
    group_id = "member"
    memory {
      allocation_mb = 1024
    }
    host_flavor {
      id = "multitenant"
    }
    cpu {
      allocation_count = 1
    }
  }
  tags              = ["one:two"]
  users {
    name     = "user123"
    password = "password123456789"
  }
}
Result
Planning failed. Terraform encountered an error while generating this plan.

β•·
β”‚ Error: 1 error occurred:
β”‚ 	* The current cpu alloaction of 1 is not valid for your current configuration.
β”‚ 
β”‚ 
β”‚ 
β”‚   with ibm_database.test_acc,
β”‚   on main.tf line 5, in resource "ibm_database" "test_acc":
β”‚    5: resource "ibm_database" "test_acc" {
β”‚ 
Convert to Multitenant Formation
data "ibm_resource_group" "group" {
  name = "default"
}

resource "ibm_database" "test_acc" {
  resource_group_id = data.ibm_resource_group.group.id
  name              = "min-compute-redis-03-22-four"
  service           = "databases-for-redis"
  plan              = "standard"
  location          = "us-south"
  adminpassword     = "password123456789"
  group {
    group_id = "member"
    memory {
      allocation_mb = 1024
    }
    host_flavor {
      id = "multitenant"
    }
  }
  tags              = ["one:two"]
  users {
    name     = "user123"
    password = "password123456789"
  }
}
Result
data "ibm_resource_group" "group" {
  name = "default"
}

resource "ibm_database" "test_acc" {
  resource_group_id = data.ibm_resource_group.group.id
  name              = "min-compute-redis-03-22-four"
  service           = "databases-for-redis"
  plan              = "standard"
  location          = "us-south"
  adminpassword     = "password123456789"
  group {
    group_id = "member"
    memory {
      allocation_mb = 1024
    }
    host_flavor {
      id = "multitenant"
    }
  }
  tags              = ["one:two"]
  users {
    name     = "user123"
    password = "password123456789"
  }
}
Scale Multitenant Formation Below Acceptable Memory
data "ibm_resource_group" "group" {
  name = "default"
}

resource "ibm_database" "test_acc" {
  resource_group_id = data.ibm_resource_group.group.id
  name              = "min-compute-redis-03-22-four"
  service           = "databases-for-redis"
  plan              = "standard"
  location          = "us-south"
  adminpassword     = "password123456789"
  group {
    group_id = "member"
    memory {
      allocation_mb = 2048
    }
    host_flavor {
      id = "multitenant"
    }
  }
  tags              = ["one:two"]
  users {
    name     = "user123"
    password = "password123456789"
  }
}
Result
Planning failed. Terraform encountered an error while generating this plan.

β•·
β”‚ Error: 1 error occurred:
β”‚ 	* member group memory must be >= 4096 and <= 114688 in increments of 128
β”‚ 
β”‚ 
β”‚ 
β”‚   with ibm_database.test_acc,
β”‚   on main.tf line 5, in resource "ibm_database" "test_acc":
β”‚    5: resource "ibm_database" "test_acc" {
Scale Multitenant Formation With Acceptable Memory
data "ibm_resource_group" "group" {
  name = "default"
}

resource "ibm_database" "test_acc" {
  resource_group_id = data.ibm_resource_group.group.id
  name              = "min-compute-redis-03-22-four"
  service           = "databases-for-redis"
  plan              = "standard"
  location          = "us-south"
  adminpassword     = "password123456789"
  group {
    group_id = "member"
    memory {
      allocation_mb = 4096
    }
    host_flavor {
      id = "multitenant"
    }
  }
  tags              = ["one:two"]
  users {
    name     = "user123"
    password = "password123456789"
  }
}
Result
Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

Acceptance Test

$ make testacc TEST=./ibm/service/database TESTARGS='-run=TestAccIBMDatabaseInstance_Redis_Basic'
--- PASS: TestAccIBMDatabaseInstance_Redis_Basic (824.23s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database	825.700s
...
--- PASS: TestAccIBMDatabaseInstanceMongodbBasic (1304.27s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database	1305.719s
--- PASS: TestAccIBMDatabaseInstance_Rabbitmq_Basic (1522.74s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database	1524.222s
--- PASS: TestAccIBMDatabaseInstance_Elasticsearch_Basic (953.96s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database	956.385s
--- PASS: TestAccIBMDatabaseInstance_ElasticsearchPlatinum_Basic (1658.71s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database	1659.985s
--- PASS: TestAccIBMDatabaseInstance_Etcd_Basic (2848.54s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database	2849.976s
--- PASS: TestAccIBMMysqlDatabaseInstanceBasic (838.18s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database	839.679s
--- PASS: TestAccIBMCassandraDatabaseInstanceBasic (2222.69s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database	2223.982s
--- PASS: TestAccIBMMongoDBEnterpriseDatabaseInstanceBasic (6331.87s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database	6333.160s

@omaraibrahim omaraibrahim marked this pull request as ready for review March 13, 2024 13:26
@@ -2837,6 +2875,30 @@ func validateGroupHostFlavor(groupId string, resourceName string, group *Group)
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
}

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)
}

if group.CPU != nil && group.Memory.Allocation < cpuEnforcementRatioCeiling*resourceDefaults.Members.Minimum && group.Memory.Allocation > group.CPU.Allocation*resourceDefaults.Memory.CPUEnforcementRatioMb {

Choose a reason for hiding this comment

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

cpuEnforcementRatioCeiling*resourceDefaults.Members.Minimum will work for provisioning, but a customer might have horizontally scaled, so we'd need you use cpuEnforcementRatioCeiling*resourceDefaults.Members.allocationCount, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bpeicher yup you're correct! Thank you for catching that!

@omaraibrahim omaraibrahim changed the title Minimum validation/1074 Minimum validation for multitenant formations Mar 15, 2024
Comment on lines 2874 to 2875
// 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.

@@ -2837,6 +2870,39 @@ func validateGroupHostFlavor(groupId string, resourceName string, group *Group)
return nil
}

func validateMultitenantMemoryCpu(groupId string, resourceName string, resourceDefaults *Group, group *Group) error {
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 πŸ˜„

Comment on lines 2878 to 2881
// 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 :)

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 < cpuEnforcementRatioCeilingTemp*resourceDefaults.Members.Allocation && group.Memory.Allocation*resourceDefaults.Members.Allocation > group.CPU.Allocation*cpuEnforcementRatioMb {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this validation useful, since you are not going to have any fractional values in group.CPU?

defaultList, err := getDefaultScalingGroups(service, plan, "multitenant", meta)

if err != nil {
return 0, 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better to propagate the error upward from here, rather than returning 0, 0.

Something like return err, 0, 0

// Get default or current group scaling values
for _, group := range tfGroups {
if group == nil {
break
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would continue be better than break here?

Comment on lines 2877 to 2883
if group.CPU == nil {
return nil
}

if group.CPU.Allocation == 0 {
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.

Nit: these can be combined with ||

@obai-1 obai-1 merged commit bba2529 into IBM-Cloud:master Mar 25, 2024
1 check passed
ismirlia pushed a commit to powervs-ibm/terraform-provider-ibm that referenced this pull request Apr 11, 2024
* added minimum-validation logic
* refactored minimum compute code
* updated tests to pass with enforced minimums
* used cloud-databases-go-sdk version 0.6.0
* added getCpuEnforcementRatios to get use the cpu enforcement ratios in all the different scenarios
* editted validateMultitenant Function to match CLI and Provisioning-ui
* made changes requested in latest PR review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants