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

admin: Implement a simple timer to pull config #4246

Merged
merged 8 commits into from Jul 28, 2021

Conversation

colinaaa
Copy link
Contributor

Implemented mostly referenced to the #4106

  • add a PullInterval filed in config
  • setup a timer in a goroutine
  • select with context and timer

A trick point is that

https://github.com/colinaaa/caddy/blob/169bb265104d0ade5f8bab05d82e53eef30c0129/caddy.go#L485-L494

here I did not use if-else to check whether PullInterval is non-zero, but I directly use time.After(PullInterval) so that with 0 intervals.
This will return immediately which is the same as executing without time.After

re #4106

@mholt
Copy link
Member

mholt commented Jul 19, 2021

Awesome, thanks for working on this! I'll take a look shortly.

@mholt mholt added this to the 2.x milestone Jul 19, 2021
@mholt mholt added the under review 🧐 Review is pending before merging label Jul 19, 2021
@colinaaa
Copy link
Contributor Author

colinaaa commented Jul 20, 2021

Here is an example config.json for pull interval. Use another http server to serve this file at port 5000.

{
  "admin": {
    "listen": "0.0.0.0:2019",
    "config": {
      "load_interval": "3s",
      "load": { "module": "http", "url": "http://localhost:5000/config.json" }
    }
  },
  "apps": {
    "http": {
      "servers": {
        "local": {
          "listen": [":8080"]
        }
      }
    }
  }
}

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks, this is a good looking PR.

The only thing I'm not real sure about is the fact that all config loading is now asynchronous. That does make me a little nervous since the vast majority of users won't use the pull feature, and would probably expect synchronous config loading. I am not sure of the consequences if we unblock while the config is loaded...

I do appreciate the simplicity of this approach, but I'm not sure what kinds of effects it will have. What do you think?

admin.go Outdated Show resolved Hide resolved
caddy.go Outdated Show resolved Hide resolved
colinaaa and others added 2 commits July 21, 2021 18:55
use `caddy.Duration`

Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
@colinaaa
Copy link
Contributor Author

The only thing I'm not really sure about is the fact that all config loading is now asynchronous. That does make me a little nervous since the vast majority of users won't use the pull feature, and would probably expect synchronous config loading. I am not sure of the consequences if we unblock while the config is loaded...

I do appreciate the simplicity of this approach, but I'm not sure what kinds of effects it will have. What do you think?

I agree that load config asynchronously is a breaking change. I'm doing this because it's the simplest way to implement this feature.

I will update this pr to keep loading config synchronously when no interval is specified as well as keep the code simple and clear.

admin.go Outdated Show resolved Hide resolved
@francislavoie francislavoie changed the title feat: implement a simple timer to pull config admin: Implement a simple timer to pull config Jul 23, 2021
Co-authored-by: Francis Lavoie <lavofr@gmail.com>
caddy.go Outdated
select {
// if PullInterval is positive, will wait for the interval and then run with new config
// otherwise, time.After(0) will return immediately, and run with new config
case <-time.After(cfg.Admin.Config.PullInterval):
Copy link
Member

Choose a reason for hiding this comment

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

There's still an issue here, CI failing. I think PullInterval needs to be wrapped with a time.Duration() to type convert it to a time.Duration.

admin.go Outdated
// from config loader (eg. a http loader) with given interval.
//
// EXPERIMENTAL: Subject to change.
PullInterval Duration `json:"pull_interval,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

On second thought... this affects how often the function specified by the load property is run. So I wonder if, for consistency, we should call this LoadInterval instead. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me!

@mholt
Copy link
Member

mholt commented Jul 26, 2021

Nice, this is a lot more like what I had envisioned. Does it work well? If so, there's just the matter of deciding the name for the property. Then we can merge this.

@colinaaa
Copy link
Contributor Author

Does it work well?

It works as expected with the config I have given at #4246 (comment)

And I tried another config without load_interval, which work as expected too!

caddy.go Outdated Show resolved Hide resolved
mholt
mholt previously approved these changes Jul 28, 2021
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for working on this and trying it out!

I had one more suggestion that I just noticed; otherwise, LGTM and let's merge this and give it a try.

@mholt mholt removed the under review 🧐 Review is pending before merging label Jul 28, 2021
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
@mholt mholt merged commit c131339 into caddyserver:master Jul 28, 2021
@francislavoie francislavoie modified the milestones: 2.x, v2.4.4 Aug 23, 2021
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