Fix filemarker creation process, update error message on uninstall #4172

fearful-symmetry · 2024-01-31T21:50:06Z

What does this PR do?

Closes #4051

Why is it important?

This does a few things:

moves the command to create the .installed file to earlier on in the install process, so a partial install won't break the uninstall command.
Rename RunningInstalled() to make it more clear what the function does.
Change the error message that's printed when the uninstall command detects an issue with the install path

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~- [ ] I have made corresponding changes to the documentation~~
~~- [ ] I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
I have added an entry in ./changelog/fragments using the changelog tool
I have added an integration test or an E2E test

mergify · 2024-01-31T21:50:40Z

This pull request does not have a backport label. Could you fix it @fearful-symmetry? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

internal/pkg/agent/application/info/agent_metadata.go

internal/pkg/agent/application/upgrade/upgrade.go

elasticmachine · 2024-01-31T22:27:32Z

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

cmacknz · 2024-02-01T21:52:03Z

Coming up with an automated test for this is a bit tricky. To reproduce the original problem, we'd need to SIGINT the install command at the correct moment which would be hard to do deterministically.

@pchila added a good test of the "Copy files" portion of the installer in https://github.com/elastic/elastic-agent/pull/4173/files, see the changes in internal/pkg/agent/install/install_test.go.

Could we split the install up into individual function steps and write a unit test that ensures we can call uninstall if you only complete the first one?

You could also add a hidden test option to the install command to get the install to exit right after the files were copied or something. I like breaking the install code up into explicit steps a bit better since a test that says "ensure we can uninstall after the first step" is better than "ensure we can uninstall after we hit the part of the code I arbitrarily exited from".

fearful-symmetry · 2024-02-01T22:14:16Z

@cmacknz when you say "ensure we can uninstall after the first step" are you thinking of a specific part of the process? After we create the .installed file? After the files are copied?

cmacknz · 2024-02-02T00:02:50Z

Yes, should have been clearer, I am thinking of the install process as a sequence of steps like:

Create directory and install marker
Copy the agent package.
Setup the shell wrapper.
Fix permissions.
Setup the OS service.
Etc

pchila

There a small doubt on the permissions we end up for topPath which should be checked

pchila · 2024-02-05T12:58:31Z

internal/pkg/agent/install/install.go

-	err = os.MkdirAll(filepath.Dir(topPath), 0755)
+	err = os.MkdirAll(topPath, 0755)
 	if err != nil {
 		return utils.FileOwner{}, errors.New(
 			err,
-			fmt.Sprintf("failed to create installation parent directory (%s)", filepath.Dir(topPath)),
-			errors.M("directory", filepath.Dir(topPath)))
+			fmt.Sprintf("failed to create installation parent directory (%s)", topPath),
+			errors.M("directory", topPath))
 	}

+	// create the install marker
+	if err := CreateInstallMarker(topPath, ownership); err != nil {
+		return utils.FileOwner{}, fmt.Errorf("failed to create install marker: %w", err)
+	}


This is a subtle change compared to the previous logic:

before the change we were creating the parent directory of topPath with 0755, while topPath would be created by the copy of files

after the change we are creating topPath as well with 0755 permissions instead of copying the permissions from the package: this could be a little too permissive and it cannot be fixed by setting different permissions in the package directory.

From a quick check without this change the topPath is created with 0750

root@elastic-agent-dev:/opt/Elastic# ls -la total 12 drwxr-xr-x 3 root root 4096 Feb 5 12:55 . drwxr-xr-x 3 root root 4096 Dec 18 14:59 .. drwxr-x--- 4 root root 4096 Feb 5 12:55 Agent

we should check what kind of permissions we get now that we create also topDir with 0755: does the Copy() call fixes those ?

Yah, in the previous version, the Copy call would create the Agent directory. I tested it like this, but wasn't considering permissions.Lemme check...

Good catch, we need to preserve the previous permissions. This is something else we should consider adding tests for here.

So, I ended up moving the CreateInstallMarker to after the initial copy operation, which avoids the permission issue here, and also seems more useful? If we create the install marker and then the copy operation fails than we might not be able to run or uninstall the agent at all, as the executable or base config, etc, might be missing.

internal/pkg/agent/application/info/agent_metadata.go

internal/pkg/agent/application/upgrade/upgrade.go

This reverts commit 400bb0d.

internal/pkg/agent/cmd/uninstall.go

internal/pkg/agent/application/info/agent_metadata.go

internal/pkg/agent/application/upgrade/upgrade.go

cmacknz · 2024-02-13T20:26:58Z

internal/pkg/agent/install/install.go

+	// do this after the file copy but before installing service files, etc
+	// That way if the service install process fails, a user can still run `elastic-agent uninstall`
+	// to clean up.
+	if err := CreateInstallMarker(topPath, ownership); err != nil {


This is still happening after the copyFiles call, it should be immediately after the err = os.MkdirAll(filepath.Dir(topPath), 0755) earlier in this function.

So, I chose to not put it there, since putting the .installed file before anything has been installed seemed counter-intuitive, and if the copy fails we might not be able to run elastic-agent at all. Unless there's something I'm not factoring in?

You can't run elastic-agent uninstall unless the .installed marker exists, so if you put it after the copy you can't remove the copied files from disk with the elastic-agent uninstall command.

internal/pkg/agent/install/install.go

cmacknz · 2024-02-14T20:41:31Z

Needs a changelog entry.

changelog/fragments/1707951532-change-install-marker-creation.yaml

…yaml Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>

elastic-sonarqube · 2024-02-20T21:45:15Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
66.7% 66.7% Coverage on New Code
0.0% 0.0% Duplication on New Code

See analysis details on SonarQube

cmacknz · 2024-02-21T15:02:21Z

I am going to force merge this since Blake has requested changes but is on PTO. The change is now small enough that I don't expect he'd object.

…4172) * move install file marker create, rename methods, change error message * change error message * change error message * remove calls to InstallMarkerExists * fix windows helper * fix windows, again * Revert "remove calls to InstallMarkerExists" This reverts commit 400bb0d. * change install file locaton, revert removal of filepath.Dir * fix wording * move IsUpgradeable, remove duplication * add tests * add mostly useless test * linter * tinkering with CI * skip supervisor test on windows * tinkering with the linter * remove experiment * refactor install to make adding tests easier * linter * add more tests... * still fixing tests * sonarQube * fix windows tests, fight sonarqube * tinker with file marker create * move install path * fix bad merge * still cleaning up merge * add tests * add tests, docs * fix tests * don't check mode bits * move command, fix permissions * SonarQube... * refactor dir setup to make Sonarqube happy * add changelog * Update changelog/fragments/1707951532-change-install-marker-creation.yaml Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> (cherry picked from commit 65e2c30)

…4172) * move install file marker create, rename methods, change error message * change error message * change error message * remove calls to InstallMarkerExists * fix windows helper * fix windows, again * Revert "remove calls to InstallMarkerExists" This reverts commit 400bb0d. * change install file locaton, revert removal of filepath.Dir * fix wording * move IsUpgradeable, remove duplication * add tests * add mostly useless test * linter * tinkering with CI * skip supervisor test on windows * tinkering with the linter * remove experiment * refactor install to make adding tests easier * linter * add more tests... * still fixing tests * sonarQube * fix windows tests, fight sonarqube * tinker with file marker create * move install path * fix bad merge * still cleaning up merge * add tests * add tests, docs * fix tests * don't check mode bits * move command, fix permissions * SonarQube... * refactor dir setup to make Sonarqube happy * add changelog * Update changelog/fragments/1707951532-change-install-marker-creation.yaml Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> (cherry picked from commit 65e2c30) # Conflicts: # internal/pkg/agent/install/install.go # internal/pkg/agent/install/install_test.go

…4172) (#4307) * move install file marker create, rename methods, change error message * change error message * change error message * remove calls to InstallMarkerExists * fix windows helper * fix windows, again * Revert "remove calls to InstallMarkerExists" This reverts commit 400bb0d. * change install file locaton, revert removal of filepath.Dir * fix wording * move IsUpgradeable, remove duplication * add tests * add mostly useless test * linter * tinkering with CI * skip supervisor test on windows * tinkering with the linter * remove experiment * refactor install to make adding tests easier * linter * add more tests... * still fixing tests * sonarQube * fix windows tests, fight sonarqube * tinker with file marker create * move install path * fix bad merge * still cleaning up merge * add tests * add tests, docs * fix tests * don't check mode bits * move command, fix permissions * SonarQube... * refactor dir setup to make Sonarqube happy * add changelog * Update changelog/fragments/1707951532-change-install-marker-creation.yaml Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> (cherry picked from commit 65e2c30) Co-authored-by: Alex K <8418476+fearful-symmetry@users.noreply.github.com>

fearful-symmetry added 3 commits January 31, 2024 13:18

move install file marker create, rename methods, change error message

fdf8b28

change error message

e6b77f1

change error message

1e52607

fearful-symmetry self-assigned this Jan 31, 2024

fearful-symmetry requested a review from a team as a code owner January 31, 2024 21:50

fearful-symmetry requested review from AndersonQ and pchila January 31, 2024 21:50

fearful-symmetry changed the title ~~Uninstall filemarker~~ Fix filemarker creation process, update error message on uninstall Jan 31, 2024

mergify bot added the backport-skip label Jan 31, 2024

cmacknz reviewed Jan 31, 2024

View reviewed changes

internal/pkg/agent/application/info/agent_metadata.go Outdated Show resolved Hide resolved

cmacknz reviewed Jan 31, 2024

View reviewed changes

internal/pkg/agent/application/upgrade/upgrade.go Outdated Show resolved Hide resolved

fearful-symmetry added skip-changelog Team:Elastic-Agent Label for the Agent team labels Jan 31, 2024

fearful-symmetry added 3 commits January 31, 2024 14:41

remove calls to InstallMarkerExists

400bb0d

fix windows helper

43bf5f0

fix windows, again

75c93dc

pchila reviewed Feb 5, 2024

View reviewed changes

blakerouse requested changes Feb 5, 2024

View reviewed changes

internal/pkg/agent/application/info/agent_metadata.go Outdated Show resolved Hide resolved

internal/pkg/agent/application/upgrade/upgrade.go Outdated Show resolved Hide resolved

fearful-symmetry and others added 2 commits February 5, 2024 13:15

Revert "remove calls to InstallMarkerExists"

d7538fe

This reverts commit 400bb0d.

change install file locaton, revert removal of filepath.Dir

fdb90cc

AndersonQ reviewed Feb 6, 2024

View reviewed changes

internal/pkg/agent/cmd/uninstall.go Outdated Show resolved Hide resolved

fearful-symmetry added 2 commits February 6, 2024 11:31

fix wording

97fdec6

Merge remote-tracking branch 'upstream/main' into uninstall-filemarker

297bb1a

cmacknz reviewed Feb 6, 2024

View reviewed changes

internal/pkg/agent/application/info/agent_metadata.go Outdated Show resolved Hide resolved

cmacknz reviewed Feb 6, 2024

View reviewed changes

internal/pkg/agent/application/upgrade/upgrade.go Outdated Show resolved Hide resolved

fix tests

aa4e4c1

cmacknz reviewed Feb 13, 2024

View reviewed changes

fearful-symmetry added 4 commits February 13, 2024 13:05

don't check mode bits

791ba0e

move command, fix permissions

b848e5b

SonarQube...

91ee83d

refactor dir setup to make Sonarqube happy

4cff2f1

pierrehilbert requested review from pchila and michalpristas February 14, 2024 17:04

cmacknz added backport-v8.13.0 Automated backport with mergify backport-v8.12.0 Automated backport with mergify and removed skip-changelog labels Feb 14, 2024

mergify bot removed the backport-skip label Feb 14, 2024

cmacknz reviewed Feb 14, 2024

View reviewed changes

internal/pkg/agent/install/install.go Show resolved Hide resolved

add changelog

c0e82e9

fearful-symmetry requested a review from cmacknz February 15, 2024 18:06

cmacknz reviewed Feb 15, 2024

View reviewed changes

changelog/fragments/1707951532-change-install-marker-creation.yaml Outdated Show resolved Hide resolved

Update changelog/fragments/1707951532-change-install-marker-creation.…

28ad7df

…yaml Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>

cmacknz approved these changes Feb 15, 2024

View reviewed changes

pchila approved these changes Feb 20, 2024

View reviewed changes

fearful-symmetry and others added 2 commits February 20, 2024 08:45

Merge branch 'main' into uninstall-filemarker

d6d0fac

Merge branch 'main' into uninstall-filemarker

e2d5d89

cmacknz merged commit 65e2c30 into elastic:main Feb 21, 2024
9 checks passed

This was referenced Feb 21, 2024

[8.12](backport #4172) Fix filemarker creation process, update error message on uninstall #4306

Closed

[8.13](backport #4172) Fix filemarker creation process, update error message on uninstall #4307

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix filemarker creation process, update error message on uninstall #4172

Fix filemarker creation process, update error message on uninstall #4172

fearful-symmetry commented Jan 31, 2024 •

edited

Loading

mergify bot commented Jan 31, 2024

elasticmachine commented Jan 31, 2024

cmacknz commented Feb 1, 2024

fearful-symmetry commented Feb 1, 2024

cmacknz commented Feb 2, 2024

pchila left a comment

pchila Feb 5, 2024 •

edited

Loading

fearful-symmetry Feb 5, 2024

cmacknz Feb 5, 2024

fearful-symmetry Feb 5, 2024

cmacknz Feb 13, 2024

fearful-symmetry Feb 13, 2024

cmacknz Feb 13, 2024

cmacknz commented Feb 14, 2024

elastic-sonarqube bot commented Feb 20, 2024

cmacknz commented Feb 21, 2024

Fix filemarker creation process, update error message on uninstall #4172

Fix filemarker creation process, update error message on uninstall #4172

Conversation

fearful-symmetry commented Jan 31, 2024 • edited Loading

What does this PR do?

Why is it important?

Checklist

mergify bot commented Jan 31, 2024

elasticmachine commented Jan 31, 2024

cmacknz commented Feb 1, 2024

fearful-symmetry commented Feb 1, 2024

cmacknz commented Feb 2, 2024

pchila left a comment

Choose a reason for hiding this comment

pchila Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

fearful-symmetry Feb 5, 2024

Choose a reason for hiding this comment

cmacknz Feb 5, 2024

Choose a reason for hiding this comment

fearful-symmetry Feb 5, 2024

Choose a reason for hiding this comment

cmacknz Feb 13, 2024

Choose a reason for hiding this comment

fearful-symmetry Feb 13, 2024

Choose a reason for hiding this comment

cmacknz Feb 13, 2024

Choose a reason for hiding this comment

cmacknz commented Feb 14, 2024

elastic-sonarqube bot commented Feb 20, 2024

Quality Gate passed

cmacknz commented Feb 21, 2024

fearful-symmetry commented Jan 31, 2024 •

edited

Loading

pchila Feb 5, 2024 •

edited

Loading