search

aws-controllers-k8s / community

AWS Controllers for Kubernetes (ACK) is a project enabling you to manage AWS services from Kubernetes
https://aws-controllers-k8s.github.io/community/
Apache License 2.0
2.42k stars 256 forks source link

Properly handle Parameters for RDS `DBParameterGroup` and `DBClusterParameterGroup` #869

Closed jaypipes closed 1 year ago

jaypipes commented 3 years ago

There are a number of problems with the way that RDS Parameter Groups handle parameters, and those problems surfaced in investigating #868.

1) There is a separate DescribeDBParameters API call to get Parameters, even though the way you update Parameters is via the ModifyDBParameterGroup API. And yes, there is already a DescribeDBParameterGroups API call that returns the name of the DB Parameter Group and its parameter group family, but not its parameters.

Currently, we have a Spec.Parameters field that we register with the DBParameterGroup CRD from this section of the RDS controller's generator.yaml file:

https://github.com/aws-controllers-k8s/rds-controller/blob/1dedf846dc06951f23fe27aa366f95ae995c3c4d/generator.yaml#L149-L153

What this means is that we will need to add custom code to the sdkFind method of the DBParameterGroup resource manager that will call the DescribeDBParameters API call after the normal call to DescribeDBParameterGroups API call is made and the CR's Spec fields have been set from the Output shape. The additional call to DescribeDBParameters and the corresponding setting of the CR's Spec.Parameters field will need to go in the sdk_find_post_set_output hook point.

2) Individual parameters in the parameter group can only by "applied immediately" if the parameter apply type is "dynamic". Parameters of apply type "static" have to have the "pending-reboot" ApplyMethod set and RDS will barf out an error if you attempt to change a static parameter and pass the "immediately" value for ApplyMethod.

This problem and the sub-optimal user experience it manifests in for Terraform is highlighted effectively in a blog post here:

https://tech.instacart.com/terraforming-rds-part-3-9d81a7e2047f

We should be able to at the very least make the user experience more tenable in the ACK RDS controller by handling the whole "I set apply method to 'immediate' for a static appy-type parameter" mistake.

3) Parameters don't have to return a ParameterValue. Even though all parameters have some default value. And there isn't any API call to get the default value of a parameter. There is only an API call to get the names of parameters in the engine's default parameter group.

chlunde commented 3 years ago

@jaypipes a related issue is that if you remove a parameter, you have to call https://docs.aws.amazon.com/cli/latest/reference/rds/reset-db-parameter-group.html

I don't think code gen exposes this?

Out of curiosity, I wonder why there has to be a separate call for this. It's a bit painful that everything is a string pointer, but you can never send a nil. Should't a nil value here reset the value?

robertgates55 commented 2 years ago

Is this usable right now? Even if a little hacky?

I'm doing a spike with ACK and if there's any way I can at least set some parameters (even if, for example, I then have no way to update the,?) that'd be super useful for proving things out.

jaypipes commented 2 years ago

@robertgates55 yes, you can definitely set parameters. The problem is when you try to update certain parameters...

robertgates55 commented 2 years ago

Hey, so I've definitely got an instance of this not working.

Helm version - rds-chart: v0.0.19

My object:

apiVersion: rds.services.k8s.aws/v1alpha1
kind: DBClusterParameterGroup
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"rds.services.k8s.aws/v1alpha1","kind":"DBClusterParameterGroup","metadata":{"annotations":{},"name":"robgates-dbcpg-issuetestexample","namespace":"dev"},"spec":{"description":"robgates-dbcpg-is
suetestexample","family":"aurora-mysql5.7","name":"robgates-dbcpg-issuetestexample","parameters":[{"applyMethod":"immediate","parameterName":"innodb_strict_mode","parameterValue":"0"}]}}
  creationTimestamp: "2022-03-12T19:12:42Z"
  finalizers:
  - finalizers.rds.services.k8s.aws/DBClusterParameterGroup
  generation: 1
  name: robgates-dbcpg-issuetestexample
  namespace: dev
  resourceVersion: "569125855"
  uid: 0a01a739-1d54-419c-8f59-c3a80387e8ec
spec:
  description: robgates-dbcpg-issuetestexample
  family: aurora-mysql5.7
  name: robgates-dbcpg-issuetestexample
  parameters:
  - applyMethod: immediate
    parameterName: innodb_strict_mode
    parameterValue: "0"
status:
  ackResourceMetadata:
    arn: arn:aws:rds:eu-west-1:722198220036:cluster-pg:robgates-dbcpg-issuetestexample
    ownerAccountID: "722198220036"
  conditions:
  - lastTransitionTime: "2022-03-12T19:12:42Z"
    message: Resource synced successfully
    reason: ""
    status: "True"
    type: ACK.ResourceSynced

But, my resource via the console shows that none of the params have been set:

image

Any advice on what might be causing my troubles?

jaypipes commented 2 years ago

@robertgates55 I will need to look further into this. I will create a new e2e test in the rds-controller with your example above and reproduce.

robertgates55 commented 2 years ago

Let me know if I can help in any way. Should I raise a specific ticket maybe?

jaypipes commented 2 years ago

Let me know if I can help in any way. Should I raise a specific ticket maybe?

No need for separate ticket. I'll use this one :)

robertgates55 commented 2 years ago

@jaypipes Any update on this? Can we help in any way to reproduce etc?

jaypipes commented 2 years ago

@robertgates55 the ACK core team and the RDS team had a meeting about this issue yesterday evening. It's a top priority for us to address ASAP. Having wrapped up some planning docs I've been busy with, I'll be able to start work on this tomorrow and by end of day tomorrow should have a rough timeframe to give you.

We are going to be taking the following approach:

robertgates55 commented 2 years ago

Awesome - Thanks - keep us posted on progress.

---

We could add a Condition to the DBInstance stating that there are DBParameters that are pending apply. Or we could add a Condition to the DBParameterGroup saying there are DBInstances associated to the DBParamterGroup that will require a reboot for certain parameters to be applied. I'd love to hear your feedback on this topic!

My instinct is to reflect it on the parametergroup rather than the instance, but there's obviously a one-to-many paramgroup-to-instance relationship so that might get awkward if the paramgroup is reused extensively. Does AWS use the 'maintenance window' to automatically restart/apply param changes on instances? (am wondering whether this is something that will always require manual intervention or whether it'll eventually become consistent)

---

We will add a new field Status.ParameterStatuses which will represent the "status" information about all parameters that are in the Spec.Parameters field's keys. In other words, we will not be showing ALL the Parameters that could possibly be set for the given DBEngineFamily in Status.ParameterStatuses. We thought this would be entirely too much information (consider hundreds of database parameters....) Please let us know your thoughts on this decision!

100% agree. This should be about the parameters the user is wanting to add/modify/change, not All parameters. It's analogous to pretty standard config file patterns so will definitely make sense. It makes me wonder if there's a better name for the spec.Parameters field to reflect the fact that it's actually additionalParameters (ie in addition to the family defaults), but that's probably more of an RDS API issue rather than an ACK problem to solve.

robertgates55 commented 2 years ago

Any update here please?

jaypipes commented 2 years ago

Any update here please?

Still working on it, @robertgates55. Hoping to complete the resource tagging (#1276) today and parlay that work into the parameter support (it's a related code path)

luis-alen commented 1 year ago

I'm facing the same issue trying to create a DBClusterParameterGroup here @robertgates55. I'm unable to override the default values. It seems to work for the DBParameterGroup though.

Change the Spec.Parameters field to be a map[string]string. This will contain a map, keyed by the parameter name (e.g. "innodb_buffer_pool_size") having values corresponding to the desired ParameterValue.

I've noticed that DBParameterGroup has such a value structure, but for DBClusterParameterGroup it is different. Does that mean you've implemented/fixed DBParameterGroup but not DBClusterParameterGroup?

https://aws-controllers-k8s.github.io/community/reference/rds/v1alpha1/dbparametergroup/ https://aws-controllers-k8s.github.io/community/reference/rds/v1alpha1/dbclusterparametergroup/

I'm using ACK RDS Controller v0.1.2.

@jaypipes any updates?

a-hilaly commented 1 year ago

I'm unable to override the default values. It seems to work for the DBParameterGroup though.

I think this is correct. I didn't see the same fix applied to DBCLusterParameterGroups

a-hilaly commented 1 year ago

We need to resume working on this ASAP

brucegucode commented 1 year ago

I will redirect this issue to Andrew

jljaco commented 1 year ago

I will redirect this issue to Andrew

@brucegucode I am going to be picking up this issue, FYI.

jljaco commented 1 year ago

This should now be resolved. Both DBParameterGroup and DBClusterParameterGroup now support the fields Spec.ParameterOverrides and Status.ParameterOverrideStatuses that can be used for managing user-defined overrides of default parameters. See the relevant sections of generator.yaml for more details.

Implementation for DBParameterGroup Implementation for DBClusterParameterGroup