Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Make discovery poll interval configurable #397

Merged
merged 2 commits into from
Feb 26, 2021

Conversation

zetaab
Copy link
Contributor

@zetaab zetaab commented Feb 7, 2021

current solution is that it will query discovery Poll each minute. However, when we have for instance kops cluster it means that we have 6 etcd manager processes. This means that etcd-manager will query each 10 seconds the status from the cloudprovider API. This means total 8640 queries/day and in case of for instance OpenStack / GCP it makes also 8640 * 3 ~ 26k compute API calls /day. In my opinion this is too much and that is why I would like to configure this value.

It is not problem if we have one cluster in whole cloud, but if we have for instance 100 kops clusters those numbers are pretty high.

@@ -81,6 +81,7 @@ func main() {
flag.StringVar(&o.ClusterName, "cluster-name", o.ClusterName, "name of cluster")
flag.StringVar(&o.BackupStorePath, "backup-store", o.BackupStorePath, "backup store location")
flag.StringVar(&o.BackupInterval, "backup-interval", o.BackupInterval, "interval for periodic backups")
flag.StringVar(&o.DiscoveryPollInterval, "discovery-poll-interval", o.DiscoveryPollInterval, "interval for discovery poll")

Choose a reason for hiding this comment

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

How are we going to set that in the context of kOps? I mean, is it worth enabling the setting via ENV vars also, just like backup retentions?

Copy link
Contributor Author

@zetaab zetaab Feb 8, 2021

Choose a reason for hiding this comment

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

we need modify kops apis to make it configurable, my plan was to do that instead of just env variables

Choose a reason for hiding this comment

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

As those changes need time to go through the versioning cycle I'd recommend also to add the possibility for env vars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah there are two PRs open that will both add new flags. IMO that should be implemented then both of these. I would like to get these PRs merged first and after that make common solution for envs

@@ -81,6 +81,7 @@ func main() {
flag.StringVar(&o.ClusterName, "cluster-name", o.ClusterName, "name of cluster")
flag.StringVar(&o.BackupStorePath, "backup-store", o.BackupStorePath, "backup store location")
flag.StringVar(&o.BackupInterval, "backup-interval", o.BackupInterval, "interval for periodic backups")
flag.StringVar(&o.DiscoveryPollInterval, "discovery-poll-interval", o.DiscoveryPollInterval, "interval for discovery poll")
Copy link
Contributor

Choose a reason for hiding this comment

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

There is flag.DurationVar, which I think is the same but just avoids the need to parse

@@ -156,7 +156,8 @@ func (n *TestHarnessNode) Run() {
Id: string(uniqueID),
Endpoints: []string{grpcEndpoint},
}
peerServer, err := privateapi.NewServer(n.ctx, myInfo, serverTLSConfig, disco, grpcPort, dnsProvider, dnsSuffix, clientTLSConfig)
discoveryInterval := time.Second * 5
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh cool - does this make our tests go faster?

@justinsb
Copy link
Contributor

Thanks @zetaab ... we could also think about e.g. having a different interval once we've reached steady-state, or having a way that clients could signal the process that there's a change. But this is a great first step!

In terms of flag proliferation and mapping them, in kops-controller we're starting to define things using a yaml config block, which could be in a configmap. But etcd-manager is so low in the stack, we can't use a ConfigMap, so I'm not sure we have an easy answer here. One answer could be to define a config map, and then have something like the overrides so that we can at least have systematic names for the flags (e.g. --override spec.pollInterval=10m)

@justinsb
Copy link
Contributor

/approve
/lgtm

@justinsb justinsb merged commit d2bc348 into kopeio:master Feb 26, 2021
@zetaab zetaab deleted the feature/discoveryinterval branch February 26, 2021 14:13
@zetaab
Copy link
Contributor Author

zetaab commented Feb 26, 2021

@justinsb I totally agree, we should have faster interval if the cluster is not in steady-state. And after we have that, we should use longer duration.

justinsb added a commit to justinsb/kops that referenced this pull request Mar 1, 2021
Changes:

* Add user agent to etcd-manager requests [kubernetes#395](kopeio/etcd-manager#395)
* Add etcd-manager metrics, add openstack API metrics [kubernetes#396](kopeio/etcd-manager#396)
* Make discovery poll interval configurable [kubernetes#397](kopeio/etcd-manager#397)
* Add log levels to prevent too verbose logging [kubernetes#394](kopeio/etcd-manager#394)
justinsb added a commit to justinsb/kops that referenced this pull request Mar 1, 2021
Changes:

* Add user agent to etcd-manager requests [kubernetes#395](kopeio/etcd-manager#395)
* Add etcd-manager metrics, add openstack API metrics [kubernetes#396](kopeio/etcd-manager#396)
* Make discovery poll interval configurable [kubernetes#397](kopeio/etcd-manager#397)
* Add log levels to prevent too verbose logging [kubernetes#394](kopeio/etcd-manager#394)
hakman pushed a commit to hakman/kops that referenced this pull request Mar 1, 2021
Changes:

* Add user agent to etcd-manager requests [kubernetes#395](kopeio/etcd-manager#395)
* Add etcd-manager metrics, add openstack API metrics [kubernetes#396](kopeio/etcd-manager#396)
* Make discovery poll interval configurable [kubernetes#397](kopeio/etcd-manager#397)
* Add log levels to prevent too verbose logging [kubernetes#394](kopeio/etcd-manager#394)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants