[Elastic Agent] Improve version, restart, enroll CLI commands #20359
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
botelastic
bot
added
needs_team
|
Pinging @elastic/ingest-management (Team:Ingest Management) |
blakerouse
changed the title
💚 Build Succeeded
Expand to view the summary
Build stats
|
| @@ -99,3 +99,6 @@ | |||
| - Add --staging option to enroll command {pull}20026[20026] | |||
| - Add `event.dataset` to all events {pull}20076[20076] | |||
| - Prepare packaging for endpoint and asc files {pull}20186[20186] | |||
| - Improved version CLI {pull}20359[20359] | |||
There was a problem hiding this comment.
do we need 3 entries here, they link the same pr
There was a problem hiding this comment.
I thought it was easier to read being 3 different things, but if you prefer it to be 1 line, I can do that.
| execSingletonOnce.Do(func() { | ||
| execSingleton = newManager(log, exec) | ||
| }) | ||
| return execSingleton | ||
| } | ||
|
|
||
| // Manager returns the global reexec manager. | ||
| func Manager() ExecManager { | ||
| return execSingleton |
There was a problem hiding this comment.
This does not look like a good idea why not having just one method with Once-initialization
There was a problem hiding this comment.
I would like to, but that deep into the code, I didn't want to have to pass in the logger and the path to the executable. I wanted that to be set at startup and later usages of the manager, can get the singleton.
| daemon := client.New() | ||
| err = daemon.Start(context.Background()) | ||
| if err == nil { | ||
| defer daemon.Stop() |
There was a problem hiding this comment.
why are you stopping after restart is done?
There was a problem hiding this comment.
Stop just disconnects the client from the socket, it does not stop the running daemon.
There was a problem hiding this comment.
this all makes sense then :-)
| @@ -17,6 +17,9 @@ import ( | |||
|
|
|||
| func createListener() (net.Listener, error) { | |||
| path := strings.TrimPrefix(control.Address(), "unix://") | |||
| if _, err := os.Stat(path); !os.IsNotExist(err) { | |||
| os.Remove(path) | |||
There was a problem hiding this comment.
can we check for errors here, if file exists but you're revoked access to it (too many fd, process reading/writing to it already whatever...) this will fail and we wont know
There was a problem hiding this comment.
I will log it, but the error of not creating the listener socket will still be the really reported error.
|
|
||
| func cleanupListener() { | ||
| path := strings.TrimPrefix(control.Address(), "unix://") | ||
| os.Remove(path) |
There was a problem hiding this comment.
same here, at least log error returned from remove, will make troubleshooting easier
There was a problem hiding this comment.
Same as above, will log it.
| c := client.New() | ||
| err := c.Start(context.Background()) | ||
| if err != nil { | ||
| return errors.New(err, "Failed talking to running daemon") |
There was a problem hiding this comment.
Want me to add the path to the unix socket or the npipe?
There was a problem hiding this comment.
no i meant talking to phrase feels weird to me
| if err != nil { | ||
| return errors.New(err, "Failed talking to running daemon") | ||
| } | ||
| defer c.Stop() |
There was a problem hiding this comment.
why are we stopping after restart?
There was a problem hiding this comment.
As above, this just disconnects from the running daemon.
| c := client.New() | ||
| daemonError = c.Start(context.Background()) | ||
| if daemonError == nil { | ||
| defer c.Stop() |
There was a problem hiding this comment.
i think what stop does and what i think it does are two different things
michalpristas
approved these changes
Aug 3, 2020
|
@michalpristas Thanks for the +1. I have updated the client from |
|
@blakerouse thanks for rename, it makes it much more readable |
❕ Build Aborted
Expand to view the summary
Build stats
Log outputExpand to view the last 100 lines of log output
|
Merged
4 tasks
blakerouse
added a commit
to blakerouse/beats
that referenced
this pull request
Aug 4, 2020
…c#20359) * Add improve version CLI cmd. * Add new restart cmd. Perform restart at end of enroll. * Fix yaml annotations on version struct. * Fix control.Address on Windows. * Fix control.Address on Windows. * Fix windows dialer. * Fix control.Address on Windows. * Add to CHANGELOG. * Review cleanups. * Fix go vet. * Update talking to communicating. (cherry picked from commit 77b3b07)
blakerouse
added a commit
that referenced
this pull request
Aug 4, 2020
…enroll CLI commands (#20431) * [Elastic Agent] Improve version, restart, enroll CLI commands (#20359) * Add improve version CLI cmd. * Add new restart cmd. Perform restart at end of enroll. * Fix yaml annotations on version struct. * Fix control.Address on Windows. * Fix control.Address on Windows. * Fix windows dialer. * Fix control.Address on Windows. * Add to CHANGELOG. * Review cleanups. * Fix go vet. * Update talking to communicating. (cherry picked from commit 77b3b07) * [Elastic Agent] Fix agent control socket path to always be less than 107 characters (#20426) * Fix agent control socket path to always be less than 107 characters. * Use os.TempDir. * Don't use os.TempDir.
v1v
added a commit
to v1v/beats
that referenced
this pull request
Aug 6, 2020
…ne-2.0 * upstream/master: [docs] Promote ingest management to beta (elastic#20295) Upgrade elasticsearch client library used in tests (elastic#20405) Disable logging when pulling on python integration tests (elastic#20397) Remove pillow from testing requirements.txt (elastic#20407) [Filebeat][ATP Module]Setting user agent field required by the API (elastic#20440) [Ingest Manager] Send datastreams fields (elastic#20402) Add event.ingested to all Filebeat modules (elastic#20386) [Elastic Agent] Fix agent control socket path to always be less than 107 characters (elastic#20426) Improve cgroup_regex docs with examples (elastic#20425) Makes `metrics` config option required in app_insights (elastic#20406) Ensure install scripts only install if needed (elastic#20349) Update container name for the azure filesets (elastic#19899) Group same timestamp metrics values in app_insights metricset (elastic#20403) add_process_metadata processor adds container id even if process metadata not accessible (elastic#19767) Support "cluster" scope in Metricbeat elasticsearch module (elastic#18547) [Filebeat][SophosXG Module] Renaming module and fileset (elastic#20396) Update Suricata dashboards (elastic#20394) [Elastic Agent] Improve version, restart, enroll CLI commands (elastic#20359) Prepare home directories for docker images in a different stage (elastic#20356)
v1v
added a commit
to v1v/beats
that referenced
this pull request
Aug 6, 2020
…allation * upstream/master: (23 commits) [docs] Promote ingest management to beta (elastic#20295) Upgrade elasticsearch client library used in tests (elastic#20405) Disable logging when pulling on python integration tests (elastic#20397) Remove pillow from testing requirements.txt (elastic#20407) [Filebeat][ATP Module]Setting user agent field required by the API (elastic#20440) [Ingest Manager] Send datastreams fields (elastic#20402) Add event.ingested to all Filebeat modules (elastic#20386) [Elastic Agent] Fix agent control socket path to always be less than 107 characters (elastic#20426) Improve cgroup_regex docs with examples (elastic#20425) Makes `metrics` config option required in app_insights (elastic#20406) Ensure install scripts only install if needed (elastic#20349) Update container name for the azure filesets (elastic#19899) Group same timestamp metrics values in app_insights metricset (elastic#20403) add_process_metadata processor adds container id even if process metadata not accessible (elastic#19767) Support "cluster" scope in Metricbeat elasticsearch module (elastic#18547) [Filebeat][SophosXG Module] Renaming module and fileset (elastic#20396) Update Suricata dashboards (elastic#20394) [Elastic Agent] Improve version, restart, enroll CLI commands (elastic#20359) Prepare home directories for docker images in a different stage (elastic#20356) New multiline mode in Filebeat: while_pattern (elastic#19662) ...
melchiormoulin
pushed a commit
to melchiormoulin/beats
that referenced
this pull request
Oct 14, 2020
…c#20359) * Add improve version CLI cmd. * Add new restart cmd. Perform restart at end of enroll. * Fix yaml annotations on version struct. * Fix control.Address on Windows. * Fix control.Address on Windows. * Fix windows dialer. * Fix control.Address on Windows. * Add to CHANGELOG. * Review cleanups. * Fix go vet. * Update talking to communicating.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
versionhas been changed to show both the version of the running daemon and the version of the executing binary. Along with--binary-onlyargument to skip connecting and reporting the version of the daemon and--yamlto print machine parsable version information.enrollhas been updated to connect to the running daemon and perform a restart. This is because after enroll if the daemon is already started it needs to be restart to load the new mode of communicating with Fleet.restartis a new command added to just trigger restart of the current running daemon. This is very useful to test re-execution (especially on Windows, because SIGHUP doesn't exist on Windows).This also includes some fixes that I thought was in my last PR for Windows, except I forgot to commit before merging the PR.
Why is it important?
To show the current running daemon version of the Agent, which could be different then the user executing binary. To improve the UX of the enroll command to re-start the daemon once successfully enrolled. To help debugging of re-execution.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration filesCHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Related issues
Logs
versionenrollrestart