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.

Sign up for GitHub

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

Wait on the watcher at startup instead of just releasing resources associated with it #4834

Merged
merged 3 commits into from
Jun 7, 2024
+40 −7
Merged

Wait on the watcher at startup instead of just releasing resources associated with it #4834

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b81fea
Wait on the watcher at startup instead of releasing.
cmacknz May 31, 2024
30c0422
Add changelog.
cmacknz May 31, 2024
52e910d
Update changelog/fragments/1717185708-Stop-creating-a-zombie-process-…
cmacknz Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions changelog/fragments/1717185708-Stop-creating-a-zombie-process-on-each-restart.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Kind can be one of:
# - breaking-change: a change to previously-documented behavior
# - deprecation: functionality that is being removed in a later release
# - bug-fix: fixes a problem in a previous version
# - enhancement: extends functionality but does not break or fix existing behavior
# - feature: new functionality
# - known-issue: problems that we are aware of in a given version
# - security: impacts on the security of a product or a user’s deployment.
# - upgrade: important information for someone upgrading from a prior version
# - other: does not fit into any of the other categories
kind: feature

# Change summary; a 80ish characters long description of the change.
summary: Stop creating a zombie process on each restart.

# Long description; in case the summary is not enough to describe the change
# this field accommodate a description without length limits.
# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment.
#description:

# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
component: "elastic-agent"

# PR URL; optional; the PR number that added the changeset.
# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
# Please provide it if you are adding a fragment for a different PR.
pr: https://github.com/elastic/elastic-agent/pull/4834

# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
# If not present is automatically filled by the tooling with the issue linked to the PR number.
#issue: https://github.com/owner/repo/1234
15 changes: 8 additions & 7 deletions internal/pkg/agent/application/upgrade/rollback.go
View file
Original file line number Diff line number Diff line change
@@ -146,23 +146,24 @@ func InvokeWatcher(log *logger.Logger, agentExecutable string) (*exec.Cmd, error
}

cmd := invokeCmd(agentExecutable)
defer func() {
if cmd.Process != nil {
log.Infof("releasing watcher %v", cmd.Process.Pid)
_ = cmd.Process.Release()
}
}()

log.Infow("Starting upgrade watcher", "path", cmd.Path, "args", cmd.Args, "env", cmd.Env, "dir", cmd.Dir)
if err := cmd.Start(); err != nil {
return nil, fmt.Errorf("failed to start Upgrade Watcher: %w", err)
}

upgradeWatcherPID := cmd.Process.Pid
agentPID := os.Getpid()

go func() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't added a test for this, because there is a test in #4822 that catches this 100% of the time.

Specifically it is this block of fixture_install.go:

	f.t.Cleanup(func() {
		// check for running agents after uninstall had a chance to run
		processes := getElasticAgentProcesses(f.t)

		// there can be a single agent left when using --develop mode
		if f.installOpts != nil && f.installOpts.Develop {
			assert.LessOrEqual(f.t, len(processes), 1, "More than one agent left running at the end of the test when --develop was used: %v", processes)
			// The agent left running has to be the non-development agent. The development agent should be uninstalled first as a convention.
			if len(processes) > 0 {
				assert.NotContains(f.t, processes[0].Cmdline, paths.DevelopmentInstallDirName,
					"The agent installed with --develop was left running at the end of the test or was not uninstalled first: %v", processes)
			}
			return
		}

		assert.Empty(f.t, processes, "there should be no running agent at the end of the test")
	})

The assert.LessOrEqual(f.t, len(processes), 1, ... will fail without this change, because if there is an agent process running we alway pick up 2. One is the main process, and the other is the zombie created from the watcher.

That test can't be extracted from that PR since it depends on the --develop option, and I didn't think it made sense to add another separate test, but let me know and I'll come up with something. Probably an integration test that calls elastic-agent restart and checks to make sure there's only one process running afterwards.

if err := cmd.Wait(); err != nil {
log.Infow("Upgrade Watcher exited with error", "agent.upgrade.watcher.process.pid", "agent.process.pid", agentPID, upgradeWatcherPID, "error.message", err)
}
}()

log.Infow("Upgrade Watcher invoked", "agent.upgrade.watcher.process.pid", upgradeWatcherPID, "agent.process.pid", agentPID)

return cmd, nil

}

func restartAgent(ctx context.Context, log *logger.Logger, c client.Client) error {