Add cli flag '--disable-hot-restart'.#2576
Conversation
|
Thanks for working on this! Some ultra quick notes:
|
|
Thanks @mattklein123. Actually having --disable_hot_restart as a flag would be great. I hadn't added a flag before and I wasn't sure about all the steps required. Tony -- what do you think of splitting this PR so you just add the flag to Options/OptionsImpl here, but not make it operational. That wouldn't conflict with my PR and I could merge and use your flag in lieu of some explicit args I added. Also, does this need a doc update associated with it? |
Yes needs docs here: https://www.envoyproxy.io/docs/envoy/latest/operations/cli with appropriate links into other parts of the docs. (I did not do a full review, just quick skim w/ comments) |
|
@jmarantz sounds good, I'll just add the flag. Thanks for taking a look! |
|
@mattklein123 is there anything I should do on my end to update the documentation? I grepped around the repo and didn't see files that contained the linked cli docs. |
|
Documentation is in the data-plane-api repo. See here: https://github.com/envoyproxy/data-plane-api/blob/master/CONTRIBUTING.md |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM, thanks! Small comment. Please fix DCO also.
test/mocks/server/mocks.cc
Outdated
There was a problem hiding this comment.
I would use ReturnPointee here.
Signed-off-by: Tony Allen <tallen@lyft.com>
tonya11en
force-pushed
the
disable_hr_flag
branch
from
3280ee9 to
0e375f4
Compare
Signed-off-by: Tony Allen <tallen@lyft.com>
This reverts commit a2dcdf3. Signed-off-by: Tony Allen <tallen@lyft.com>
|
I'll figure out what's going on with that ReturnPointee call one night this week. Can't do much during the work day. |
Signed-off-by: Tony Allen <tallen@lyft.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
This looks great and would be easy to integrate into my PR.
This is the update to the documentation that's required once the --disable-hot-restart cli flag PR is merged. See envoyproxy/envoy#2576. Signed-off-by: Tony Allen <tallen@nutanix.com>
|
@tonya11en Now that #2568 is merged, do you want to merge master and make any needed changes? |
|
@jmarantz once this gets approved/merged, should I just go ahead and work it into your recently merged PR? Not sure if you had other plans regarding this. |
|
@htuch sounds good to me, I just need someone to approve this PR. I'm not authorized to merge this guy. |
This reverts commit 02c34e0.
The --disable-hot-restart flag was added to the binary in #2576 by @tonya11en but not hooked up (at my request). Now that the main_common refactor is in, it's easier to hook it up. This also offers the opportunity for some minor code simplifications. Risk Level: Medium -- this affects the startup path. Testing: //test/... added a new subtest to hotrestart_test.sh to cover --disable-hot-restart. I'd appreciate suggestions though on how to cover the functionality more thoroughly...how should I detect from a test that hot restart is disabled? Release Notes: TBD: I'm not sure if release notes were added already in #2576 -- if not they can be added during this PR review. *Fixes #2029 -- actually #2576 claims to fix that but I don't think it really does, since in that PR the switch is a no-op. Signed-off-by: Joshua Marantz <jmarantz@google.com>
…#2576) * add option to release-binary to skip pushing docker image * add more comment and address comment * fix * add env variable definition
This is the update to the documentation that's required once the --disable-hot-restart cli flag PR is merged. See envoyproxy/envoy#2576. Signed-off-by: Tony Allen <tallen@nutanix.com>
5 participants
hot restart: add cli flag to disable hot restart functionality
Description:
Risk Level: Low | Medium | High
Testing:
Docs Changes:
Release Notes:
Fixes #2029
Signed-off-by: Tony Allen tallen@lyft.com