GoogleCloudPlatform / k8s-config-connector

GCP Config Connector, a Kubernetes add-on for managing GCP resources
https://cloud.google.com/config-connector/docs/overview
Apache License 2.0
897 stars 231 forks source link

Memcached instance CRD immutable parameters #484

Open mikesharpton-bestbuy opened 3 years ago

mikesharpton-bestbuy commented 3 years ago

Hello,

I am not sure if this is the proper type of issue to submit, but here it goes anyway. I am working on deploying memcached instances via config connector based on this page, https://cloud.google.com/config-connector/docs/reference/resource-docs/memcache/memcacheinstance.

In looking over this, I can see that all the memcached parameters or configuration parameters are immutable.
I tried to update my manifest to see what would happen, and this is what happened.

Warning UpdateFailed 38s (x12 over 50s) memcacheinstance-controller Update call failed: error applying desired state: summary: Error updating Instance "projects/REDACTED/locations/us-central1/instances/REDACATED": googleapi: Error 400: The update mask is empty com.google.apps.framework.request.StatusException: generic::INVALID_ARGUMENT: The update mask is empty Details: [ { "@type": "type.googleapis.com/google.rpc.BadRequest", "fieldViolations": [ { "description": "Invalid value: ", "field": "update_mask"

This limitation means to change or add a new parameter I would have to redeploy the instance and invalidate/destroy all cache on all nodes all at one time. This is not ideal.

I am able to change parameters in the UI and then stage the updates to each of my nodes to avoid complete cache flush on all nodes. I cannot do this with config connector. There is a likely a good reason why this cannot be done and is not done. Is there a technical reason this cannot be done, or is this planned for a later update to the CRD? Your help and time is appreciated.

Thanks, Mike

jcanseco commented 3 years ago

Hi @mikesharpton-bestbuy.

I can see that all the memcached parameters or configuration parameters are immutable.

To clarify, I'm assuming you're referring to the spec.memcacheParameters.params field? If so, then I can confirm that I end up with the same error message by trying to update that field:

Warning  UpdateFailed  21s (x2 over 103s)   memcacheinstance-controller  Update call failed: error applying desired state: summary: Error updating Instance "projects/cnrm-jcanseco-2/locations/us-central1/instances/memcacheinstance-sample": googleapi: Error 400: The update mask is empty
com.google.apps.framework.request.StatusException: <eye3 title='INVALID_ARGUMENT'/> generic::INVALID_ARGUMENT: The update mask is empty
Details:
[
  {
    "@type": "type.googleapis.com/google.rpc.BadRequest",
    "fieldViolations": [
      {
        "description": "Invalid value: ",
        "field": "update_mask"
      }
    ]
  }
], detail:

Upon inspecting the REST API docs, it does seem like the parameters should be updateable (it just requires a special method).

I can file a request to allow for parameter updates. Can you elaborate on whether you consider this issue a blocker, friction point, or nice-to-have?

mikesharpton-bestbuy commented 3 years ago

Hello @jcanseco,

Thanks for your reply. This is a nice to have, but also causes a bit of friction as we would like to be able to pass in params via our pipeline and be able to update params without destroying the instance via Config Connector. I would assume there is no way to stage the rollouts of the parameter changes via Config Connector as you can in the UI and no plans to add this?

This may be OK, as we could instruct the users to use the UI to deploy the changes to the nodes and use config connector to change/add the parameters (once the fields are no longer immutable via this enhancement request).

For now, I am going forward with what we have today and we can revisit this when the enhancement is released. All parameters will be configured via the UI only and deployed from there as well. Only the instance will be deployed via Config Connector and our pipeline. Can this issue remain open until then?

EDIT: I did not read this https://cloud.google.com/memorystore/docs/memcached/reference/rest/v1/projects.locations.instances/applyParameters

Perhaps you could deploy/apply the parameters via Config Connector but I am not sure how you would do this properly/efficiently as you have to list all the nodeIds. It may be too cumbersome to look up and list out nodes to apply parameter changes to them in the yaml and I am not sure how this would work until I see how it is implemented in Config Connector.

jcanseco commented 3 years ago

I would assume there is no way to stage the rollouts of the parameter changes via Config Connector as you can in the UI

Unfortunately, that seems to be the case.

no plans to add this?

Our plan is to look into whether it's possible to add support for this use-case. MemcacheInstance is one of our Terraform-based resources, so I've deferred to the Terraform team to see if adding support would be feasible. We'll have to wait and see what they have to say. There might have been a reason why they didn't implement support for parameter updates to begin with.

Can this issue remain open until then

Yes of course.

I am not sure how you would do this properly/efficiently as you have to list all the nodeIds.

Thanks, this is a good call out so I looked into it as well. It seems that this should be possible since the instance resource supports a memcacheNodes field which lists its nodes (including their nodeId).

There might be other limitations preventing the Terraform team from implementing parameter updates that we're not aware of at this moment, but listing the nodeIds needed for the applyParameters call should be possible at least.