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

Changes to binary distribution #6542

Merged
merged 45 commits into from
Jan 29, 2025

Conversation

michalpristas
Copy link
Contributor

Ready for review but DO NOT MERGE before spec files of dependencies are updated in other repos

What does this PR do?

This PR changes logic of installation and ugprades in following manner.

Default install installs only:

  • agentbeat
  • endpoint-security
  • pf-host-agent

additional flag is added that includes components above and:

  • cloudbeat
  • apm-server
  • fleet-server
  • pf-elastic-symbolizer
  • pf-elastic-collector

Upon installation user choice is backed into .flavor file.

Part of elastic agent package is .flavors file defining flavor for this particular version.
Upon upgrade or install .flavors is read and only components specified in desired flavor (using flag with install or content of .flavor in case of upgrade) are copied.

In case of any issues with parsing any of the above files we fall back to previous way and copy everything.

Each components needs to define files in addition to binary, spec and config in a form

component_files:
 - file1
 - dir1/*

in their spec file.

During upgrade target version spec file is parsed. not the current installation.

Why is it important?

Reducing space on disk by omitting not needed components.

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 TODO: @michalpristas
  • 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

Scenario 1 and 2 (installation)

Build package and install without and with flag --install-server
check components present

Scenario 3 - upgrade to future version

Prepare same agent but with different commit hash (do small change and commit locally)
Run upgrade 9.0.0-SNAPSHOT --source-uri="file://....zip" --skip-verify and check only desired components are present

Scenario 4 - upgrade from stale version

Upgrade from previous version to this one and check ALL components are present

@michalpristas michalpristas added enhancement New feature or request backport-skip labels Jan 17, 2025
@michalpristas michalpristas self-assigned this Jan 17, 2025
@michalpristas michalpristas requested a review from a team as a code owner January 17, 2025 16:13
@elasticmachine
Copy link
Contributor

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

}

// SpecsForFlavor returns spec files associated with specific flavor
func SpecsForFlavor(flavor FlavorDefinition) ([]string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function always returns nil as error, is the error needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

leftover from previous design, thanks for pointing it out


// fix versionedHome
versionedHome = strings.ReplaceAll(versionedHome, "\\", "/")
log.Errorf("using %q", versionedHome)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? It's not an error, right?

Copy link
Contributor Author

@michalpristas michalpristas Jan 23, 2025

Choose a reason for hiding this comment

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

it's not an error, this is because zip on windows are using unix paths 🤦

@cmacknz cmacknz self-requested a review January 24, 2025 21:53
dev-tools/mage/settings.go Outdated Show resolved Hide resolved
dev-tools/mage/settings.go Outdated Show resolved Hide resolved
dev-tools/mage/settings.go Outdated Show resolved Hide resolved
internal/pkg/agent/application/upgrade/step_unpack.go Outdated Show resolved Hide resolved
@cmacknz
Copy link
Member

cmacknz commented Jan 27, 2025

Manually testing this this and it works so far:

  • elastic-agent install omits all server components
  • elastic-agent install --install-servers includes server components
  • elastic-agent upgrade 9.0.0-SNAPSHOT from 8.15.x with no flavor to this branch kept all the components.

Things that still need to be tested:

  • Upgrading from a version with a defined flavor to another version that supports flavors and ensuring it respects the flavor, ideally for both basic and server flavors. We can set this up by messing with the package version during build but we might need to merge this first to allow automated tests of it.

Copy link
Member

@cmacknz cmacknz left a comment

Choose a reason for hiding this comment

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

No big comments mostly nits.

I wonder if you can use the "upgrade to same commit" test to add a basic test of upgrading from an agent installed with a flavor to another one installed with the same flavor to make sure the unpacking logic works properly.

t.Run(fmt.Sprintf("Upgrade on a repackaged version of agent %s (%s)", currentVersion, unPrivilegedString), func(t *testing.T) {

.flavors Outdated Show resolved Hide resolved
internal/pkg/agent/application/upgrade/step_unpack.go Outdated Show resolved Hide resolved
internal/pkg/agent/application/upgrade/upgrade.go Outdated Show resolved Hide resolved
internal/pkg/agent/application/upgrade/upgrade.go Outdated Show resolved Hide resolved
internal/pkg/agent/cmd/install.go Outdated Show resolved Hide resolved
internal/pkg/agent/install/flavors.go Outdated Show resolved Hide resolved
internal/pkg/agent/install/flavors.go Outdated Show resolved Hide resolved
internal/pkg/agent/install/install.go Outdated Show resolved Hide resolved
@michalpristas
Copy link
Contributor Author

@cmacknz i did not want to add same commit upgrade as it may not uncover some issues e.g if there are leftovers from servers install and only basic is applied. but test would evaluate properly. i will add tests once we have snapshot.
but i was testing this manually by adding some artificial commit (e.g logline) and using update with custom source uri

@cmacknz
Copy link
Member

cmacknz commented Jan 28, 2025

@cmacknz i did not want to add same commit upgrade as it may not uncover some issues e.g if there are leftovers from servers install and only basic is applied. but test would evaluate properly. i will add tests once we have snapshot.
but i was testing this manually by adding some artificial commit (e.g logline) and using update with custom source uri

As long as you tested this manually I'm fine added a separate test once a snapshot exists as a follow up.

Copy link
Member

@cmacknz cmacknz left a comment

Choose a reason for hiding this comment

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

LGTM, it works and the only comments are nits. Make sure this is followed up with an integration test for an upgrade from a basic->basic flavor and servers->servers flavor.

Also address any outstanding comments from others before merge if you haven't already.


"github.com/elastic/go-ucfg/yaml"

yamlv3 "gopkg.in/yaml.v3"
Copy link
Member

Choose a reason for hiding this comment

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

nit: we are using both github.com/elastic/go-ucfg/yaml and gopkg.in/yaml.v3 at the same time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i wanted to reduce ucfg step. and keeping ucfg one for NewConfigFrom use cases i don't want to address in this PR if that makes sense.
otherwise insteadn of unmarshal it would be
unmarshal -> ucfg.NewFrom -> Unpack

Copy link
Member

Choose a reason for hiding this comment

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

If it's not trivial to address I'm fine just leaving it as is

@michalpristas
Copy link
Contributor Author

elastic/beats#42348
merged, i'll merge this once beats snapshot is ready

@cmacknz
Copy link
Member

cmacknz commented Jan 29, 2025

We also need to test that the spec file changes to include additional files for every integration work correctly. We have an integration test that installs defend but nothing for osquery or apm server (those are the ones I remember off the top of my head).

@michalpristas michalpristas merged commit 093fb58 into elastic:main Jan 29, 2025
14 checks passed
@cmacknz
Copy link
Member

cmacknz commented Jan 29, 2025

Follow up testing work captured into #6631 so we don't forget.

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-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants