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.

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

Include upgrade details in diagnostics bundle during ongoing upgrade #3624

Merged
merged 16 commits into from
Nov 3, 2023

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Oct 18, 2023

What does this PR do?

This PR enhances the diagnostics bundle to include upgrade details when Agent is being upgraded.

Why is it important?

To gain more visibility into the Agent upgrade process.

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

How to test this PR locally

  1. Build elastic-agent from this PR's branch and install it.
  2. Run elastic-agent diagnostics, expand the resulting archive, and check the bundled state.yaml file. Verify that NO upgrade details are included in it by checking that there is no upgrade_details field.
  3. In a new window, upgrade Elastic Agent to a version that hasn't been released yet. This will ensure that the upgrade starts but remains stalled.
    sudo elastic-agent upgrade 8.13.0
    
  4. In the original window, run elastic-agent diagnostics again, expand the resulting archive, and check the bundled state.yaml file. Verify that upgrade details are included in it by checking that there is an upgrade_details field.

Related issues

@ycombinator ycombinator added the enhancement New feature or request label Oct 18, 2023
@mergify
Copy link
Contributor

mergify bot commented Oct 18, 2023

This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
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.

@ycombinator ycombinator added the Team:Elastic-Agent Label for the Agent team label Oct 18, 2023
@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 18, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-11-02T17:51:53.165+0000

  • Duration: 14 min 38 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages.

  • run integration tests : Run the Elastic Agent Integration tests.

  • run end-to-end tests : Generate the packages and run the E2E Tests.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 18, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 98.824% (84/85) 👍
Files 67.105% (204/304) 👍
Classes 66.19% (370/559) 👍
Methods 53.583% (1174/2191) 👍
Lines 39.779% (13762/34596) 👎 -0.034
Conditionals 100.0% (0/0) 💚

@mergify
Copy link
Contributor

mergify bot commented Oct 19, 2023

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 upgrade-details-in-diagnostics upstream/upgrade-details-in-diagnostics
git merge upstream/main
git push upstream upgrade-details-in-diagnostics

@ycombinator ycombinator force-pushed the upgrade-details-in-diagnostics branch from 081d5ec to eebc69e Compare October 20, 2023 00:48
@ycombinator ycombinator marked this pull request as ready for review October 20, 2023 00:49
@ycombinator ycombinator requested a review from a team as a code owner October 20, 2023 00:49
@elasticmachine
Copy link
Contributor

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

FleetMessage: s.FleetMessage,
LogLevel: s.LogLevel,
Components: compStates,
UpgradeDetails: s.UpgradeDetails,
Copy link
Member

Choose a reason for hiding this comment

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

When is this field present? Do we get details about the upgrade after a failure ? Or those get cleared as soon as the grace period is over ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When is this field present?

This field is populated when the upgrade process starts:

det := details.NewDetails(version, details.StateRequested, actionID)
det.RegisterObserver(c.SetUpgradeDetails)

Do we get details about the upgrade after a failure ?

Yes, if the upgrade process fails at any point, this field will continue to remain populated.

Or those get cleared as soon as the grace period is over ?

I assume you are referring to the Upgrade Watcher's grace period.

Exactly when we clear it out after that is not yet implemented. We need to decide whether we want to clear it out based on some timeout (maybe the grace period or maybe something even beyond that) or due to some user intervention (which could be retrying the upgrade or something else just to clear the upgrade details, not sure yet) or either (whichever happens first).

FleetMessage string `yaml:"fleet_message"`
LogLevel logp.Level `yaml:"log_level"`
Components []StateComponentOutput `yaml:"components"`
UpgradeDetails *details.Details `yaml:"upgrade_details,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any changes in the tests related to this change... could you please add a unit test to document what we can expect in the hook output when UpgradeDetails is populated ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Added in 02625e8.

@ycombinator ycombinator force-pushed the upgrade-details-in-diagnostics branch from eebc69e to 5b1db16 Compare November 1, 2023 06:13
@ycombinator ycombinator requested a review from pchila November 1, 2023 06:14
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Nice! Great this is added to the diagnostics.

Copy link
Member

@pchila pchila left a comment

Choose a reason for hiding this comment

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

LGTM

@ycombinator ycombinator force-pushed the upgrade-details-in-diagnostics branch from 5b1db16 to a8aed84 Compare November 2, 2023 17:51
Copy link

@ycombinator ycombinator merged commit d412c3d into elastic:main Nov 3, 2023
7 checks passed
@ycombinator ycombinator deleted the upgrade-details-in-diagnostics branch November 3, 2023 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip enhancement New feature or request Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants