Lock upgrade marker

[pchila]

What does this PR do?

Introduce file locking for modifying and reading elastic-agent update marker consistently.
FileLocker is a wrapper around github.com/gofrs/flock that exposes both blocking and non-blocking file locking.
A lock on an update marker lockfile has been introduced at creation, reading and writing: this should ensure that no concurrent read/writes happen between elastic-agent main process and watcher.
This is a prerequisite for manual rollback feature: with such a feature both process will read/write the update marker concurrently.

Why is it important?

With the manual rollback feature that will be introduced by #6889, both elastic-agent main process and watcher need to modify the update marker. Locking before modification should avoid inconsistent writes.

Checklist

• I have read and understood the pull request guidelines of this project.
• 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

Disruptive User Impact

How to test this PR locally

Aside from unit tests that have been introduced under internal/pkg/agent/application/filelock and internal/pkg/agent/application/upgrade/step_mark.go, the only use case here is to perform an upgrade and verify that it succeeds.
If we want to test the file locking we can use the testlocker binary in internal/pkg/agent/application/filelock/testlocker/main.go and lock the update marker lock file in <elastic agent root>/data/.update-marker.lock (holding this lock for too long may generate some failures as there's a timeout of 30s for locking operations.

Related issues

• Closes Lock upgrade marker before writing both in agent and agent watcher #6878

Questions to ask yourself

• How are we going to support this in production?
• How are we going to measure its adoption?
• How are we going to debug this?
• What are the metrics I should take care of?
• ...

[mergify]

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

• backport-./d./d is the label that automatically backports to the 8./d branch. /d is the digit
• backport-active-all is the label that automatically backports to all active branches.
• backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
• backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b lock-upgrade-marker upstream/lock-upgrade-marker
git merge upstream/main
git push upstream lock-upgrade-marker

[pchila]

buildkite test this

[elasticmachine]

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

[swiatekm]

From what I can see, all the locking when reading or writing to the marker file is non-blocking. What will actually happen if the agent and watcher try to access the file concurrently? One will get an error, and then what?

[pchila]

From what I can see, all the locking when reading or writing to the marker file is non-blocking. What will actually happen if the agent and watcher try to access the file concurrently? One will get an error, and then what?

@swiatekm
We don't use flock's Lock() API as in the documentation we read It's recommended that TryLock() be used over this function..

As an alternative we are using a TryLockContext() with a timeout

https://github.com/elastic/elastic-agent/pull/8254/files#diff-a21d895563344387fd97f5e4b5f5769a266362240b0b6ac420113eabfa1301a6R99

https://github.com/elastic/elastic-agent/pull/8254/files#diff-a21d895563344387fd97f5e4b5f5769a266362240b0b6ac420113eabfa1301a6R59-R62

so the FileLocker will keep trying to acquire a lock periodically for the timeout duration and if it fails it will error out.
Right now such an error is fatal when creating the update marker (but under normal condition there should be no contention at that point).

Reading and writing the update marker right now consists of a single writer (main process or watcher) in every phase.
As #6890 and #6889 get implemented we are gonna have multiple concurrent writers on update marker where the lock needs to be in place to avoid overwriting information. If a writer gets a locking error it will need to reload the update marker data and retry the (partial) modification

[elastic-sonarqube]

Quality Gate passed

Issues
1 New issue
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
70.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 955b09a

History

• 💚 Build #21974 succeeded ee52547
• 💛 Build #21867 was flaky 0b75e26
• 💔 Build #21856 failed a32c2ee
• 💛 Build #21818 was flaky fbfd6ab
• 💔 Build #21806 failed 588122c
• 💔 Build #21803 failed 8498b72

cc @pchila

[ycombinator]

Left a question about whether we need the CHANGELOG entry for this change but the implementation itself + tests LGTM!

[michalpristas]

i'm not fun of panics but the alternative is not good as well. this will send us to startup spiral loop but at least we know something's wrong right away - should not happen in very most of the cases.
LGTM otherwise.