-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
allow features option live reloading #37692
Conversation
ptal @thaJeztah @tiborvass |
daemon/reload.go
Outdated
// reloadFeatures updates configuration with enabled/disabled features | ||
func (daemon *Daemon) reloadFeatures(conf *config.Config, attributes map[string]string) { | ||
// update corresponding configuration | ||
if conf.IsValueSet("features") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this if to be able to unset features ?
ad3d477
to
2472170
Compare
Codecov Report
@@ Coverage Diff @@
## master #37692 +/- ##
=========================================
Coverage ? 36.03%
=========================================
Files ? 609
Lines ? 45063
Branches ? 0
=========================================
Hits ? 16240
Misses ? 26592
Partials ? 2231 |
97a1f12
to
f634ae4
Compare
@tiborvass I've updated the PR and verified this would actually work. |
@@ -7,6 +7,8 @@ import ( | |||
"net/http" | |||
"time" | |||
|
|||
"github.com/docker/docker/api/server/router/build" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unneeded extra space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
f634ae4
to
c11a34e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
c11a34e
to
ac14e27
Compare
@tonistiigi or @thaJeztah can you help review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one nit, but looks good otherwise
daemon/daemon.go
Outdated
@@ -136,6 +136,11 @@ func (daemon *Daemon) HasExperimental() bool { | |||
return daemon.configStore != nil && daemon.configStore.Experimental | |||
} | |||
|
|||
// GetFeatures returns the features map from configStore | |||
func (daemon *Daemon) GetFeatures() *map[string]bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to name this Features()
instead of GetFeatures()
; golang convention is to not use a Get
prefix for getters; https://golang.org/doc/effective_go.html#Getters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good call. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thaJeztah Fixed :)
Signed-off-by: Anda Xu <anda.xu@docker.com>
ac14e27
to
58a75ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// check if the builder feature has been enabled from daemon as well. | ||
if buildOptions.Version == types.BuilderBuildKit && br.builderVersion != "" && br.builderVersion != types.BuilderBuildKit { | ||
if buildOptions.Version == types.BuilderBuildKit && builderVersion != "" && builderVersion != types.BuilderBuildKit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tiborvass As discussed offline, this value should only apply as a hint to the client on negotiation on ping, not override a daemon behavior.
- What I did
reloadFeatures
so that the changes offeatures
field in daemon.json can be live reloaded.features
to the rest of places where it is being used.- How I did it
- How to verify it
First with
buildkit
turned on indaemon.json
we hit the
![image](https://user-images.githubusercontent.com/25592824/44884359-e51c8900-ac6f-11e8-9126-928a93045890.png)
/_ping
endpoint to verify,Builder-Version: 2
Now turn
buildkit
offand run
sudo kill -s SIGHUP <dockerd-PID>
then verify with
![image](https://user-images.githubusercontent.com/25592824/44884429-388ed700-ac70-11e8-9808-0b5ac8bda748.png)
/_ping
again, it showsBuilder-Version: 1
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
Signed-off-by: Anda Xu anda.xu@docker.com