pan-net / terraform-provider-powerdns

Terraform PowerDNS provider
https://www.terraform.io/docs/providers/powerdns/
Mozilla Public License 2.0
44 stars 48 forks source link

Add cache for the PowerDNS REST API requests #81

Closed menai34 closed 3 years ago

menai34 commented 3 years ago

Hello, Yesterday, I start using the terraform provider for PowerDNS and faced to DDoS PowerDNS REST API, when the DNS zone has many entries. Unfortunately, PowerDNS REST API doesn't have a query to get one DNS record, search request only. But the search request doesn't have comment data in a response body. So, I think that cache of the PowerDNS REST API request best way, at this moment.

terraform plan for one DNS zone with ~400 DNS entries without cache terraform plan 9.22s user 4.03s system 5% cpu 3:56.36 total

terraform plan for one DNS zone with ~400 DNS entries with cache terraform plan 4.41s user 1.00s system 56% cpu 9.525 total

What do you think about this?

Regards, Andrey.

menai34 commented 3 years ago

@mbag sorry for the mention but just ping :)

mbag commented 3 years ago

hi @menai34 ,

thanks for your pull request. Even though personally I think that caching will cause weird problems when enabled, since it's optional, I'll add it. Please see comments I made.

I agree that provider should provide a resource type to set multiple records within same zone, which would cause single GET zone data request, and not multiple GET requests to get same zone data.

Another way to lower the number of requests to your server is to use terraforms -parallelism option and limit the number of concurrent operations. It defaults to 10. This is just a tip on how to

menai34 commented 3 years ago

@mbag Thank you very much for your review.

Even though personally I think that caching will cause weird problems when enabled

About this, yes, we can get this behavior when using other automation at the same time but usually terraform uses for static DNS zones with remote state storage like as aws-s3 that lock the state file. Anyway, this features needs for people that don't like to wait approximately 5-10mins to apply changes via terraform plan :)

-parallelism

Yes, everyone can use this option but it's very long :(

DNS zone with ~400 DNS entries without cache terraform plan -parallelism 1 9.37s user 5.43s system 1% cpu 18:43.46 total

DNS zone with ~400 DNS entries with cache terraform plan -parallelism 1 3.74s user 1.44s system 51% cpu 10.088 total

Anyway, thank you very much :) Regard, Andrey.

menai34 commented 3 years ago

@mbag just a ping

menai34 commented 3 years ago

Hello @mbag Do you have other remarks?

ag-TJNII commented 3 years ago

I'm consistently getting

==> Checking that code complies with gofmt requirements...
go install
powerdns/client.go:15:2: cannot find package "." in:
        /work/vendor/github.com/coocood/freecache
make: *** [GNUmakefile:12: build] Error 1

when running make build with this branch. Are all the dependencies fully committed?

[EDIT] go mod vendor fixed it for me [/EDIT]

menai34 commented 3 years ago

@ag-TJNII Hi, do I need some fix? Also, are you the maintainer of this repository now?

ag-TJNII commented 3 years ago

@ag-TJNII Hi, do I need some fix? Also, are you the maintainer of this repository now?

@menai34 I'm not a maintainer, just another user. I am using this provider to maintain several thousand DNS records, so I needed to pull this improvement in as the performance without caching was pretty painful.

To get this working I needed to run 'go mod vendor' and commit the results. It looks like this has been done before in 59e9e6b0cb181e76d2fc4b58675b71da398cf644. I'm not sure if @mbag wants that commit in this PR or not.

One suggestion I do have is to document that the cache size needs to be 1024x the largest zone size. This is coming from the freecache module you used (https://github.com/coocood/freecache/blob/master/segment.go#L13). I really didn't dig beyond that error message in the dependent module, I just set my cache size to 1024MB and moved on. However, as getting the TF provider logs can be a bit painful, and because if the cache is too small it simply doesn't cache, that would be good to add to the docs. I don't know of freecache can be configured to need a lower overhead, as 1024 is hardcoded into the error I assumed not.

menai34 commented 3 years ago

@ag-TJNII As I remember this value by the design of cache. Yes, why not, I'll try to describe cache configuration more clearly later. So, one question, did the cache helped you?

ag-TJNII commented 3 years ago

@ag-TJNII As I remember this value by the design of cache. Yes, why not, I'll try to describe cache configuration more clearly later. So, one question, did the cache helped you?

Well I just rolled it out yesterday so still early in evaluating the impact, however in testing I noticed no side effects and it had a significant improvement on terraform plan times. I'm 👍 on this change, caching the zone makes a lot of sense to me in my environment. This looks like it's going to be a big help.