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

Add initial support for Elastic Stack 9.x #2102

Merged
merged 13 commits into from
Sep 19, 2024

Conversation

mrodm
Copy link
Contributor

@mrodm mrodm commented Sep 13, 2024

Part of elastic/package-registry#1226

Add initial support for 9.0.0 stack in elastic-package.

Changes performed:

  • By default, elastic-package will use wolfi images as it was for 8.16 stack versions.
  • If no version is specified, it will report wolfi image instead (TBD).
  • Added integration test to check elastic-package stack command for 9.0.0-SNAPSHOT

Author's checklist

  • Enable target in updatecli configuration before merging.
  • Add integration test for elastic-package stack command with 9.0.0-SNAPSHOT
  • Test elastic-package stack up -v --version 9.0.0-SNAPSHOT.

}

func TestSelectCompleteElasticAgentImageName_ForceSystemDImage(t *testing.T) {
version := stackVersion8160
selected := selectElasticAgentImageName(version, "systemd")
assert.Equal(t, selected, elasticAgentImageName)
assert.Equal(t, elasticAgentImageName, selected)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the order to ensure that the values are the expected and the actual ones as required by the assert functions.
https://pkg.go.dev/github.com/stretchr/testify/assert#Equal

@@ -14,43 +14,43 @@ import (
func TestSelectElasticAgentImageName_NoVersion(t *testing.T) {
var version string
selected := selectElasticAgentImageName(version, "")
assert.Equal(t, selected, elasticAgentLegacyImageName)
assert.Equal(t, elasticAgentWolfiImageName, selected)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the test changing the value, it would return the wolfi image instead.

Copy link
Member

Choose a reason for hiding this comment

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

In principle it LGTM. Maybe another option is to return an error if the version is empty? Not sure about the implications of the change, but would look safer to me to know where/if we are passing an empty version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commands like elastic-package stack down do not set any version, and if a error is returned here, it will fail the command.

@mrodm mrodm force-pushed the support-elastic-agent-9.0 branch from 2f60421 to 16f9a3c Compare September 16, 2024 09:01
@mrodm
Copy link
Contributor Author

mrodm commented Sep 16, 2024

/test

1 similar comment
@mrodm
Copy link
Contributor Author

mrodm commented Sep 16, 2024

/test

Comment on lines 35 to 44
# targets:
# update-snapshot:
# name: '[updatecli] Update latest snapshot to {{ source "latestSnapshot" }}'
# kind: file
# sourceid: latestSnapshot
# scmid: default
# spec:
# file: Makefile
# matchpattern: '(./scripts/test-stack-command.sh) 9\.[^\s]+-SNAPSHOT'
# replacepattern: '$1 {{ source "latestSnapshot" }}-SNAPSHOT'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be enabled before merging

@@ -153,13 +154,13 @@ func (ac *ApplicationConfiguration) SetCurrentProfile(name string) {
// This is mandatory as "elastic-agent-complete" is available since 7.15.0-SNAPSHOT.
func selectElasticAgentImageName(version, agentBaseImage string) string {
if version == "" { // as version is optional and can be empty
return elasticAgentLegacyImageName
return elasticAgentWolfiImageName
Copy link
Contributor Author

@mrodm mrodm Sep 17, 2024

Choose a reason for hiding this comment

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

WDYT about changing these defaults values to use the wolfi image docker.elastic.co/elastic-agent/elastic-agent-wolfi (e.g. no version specified) instead of the legacy image (beats namespace) docker.elastic.co/beats/elastic-agent?

Or should we keep unchanged this default?

Copy link
Member

Choose a reason for hiding this comment

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

We can adapt this to the new default, yes, if we are sure that in old versions the beats namespace is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the same behaviour is kept.
As a reference, I add here the docker images used in different versions:

  • 7.14.0
     $ docker ps --format "{{.Names}} {{.Image}}" |grep elastic-agent
    elastic-package-stack-elastic-agent-1 docker.elastic.co/beats/elastic-agent:7.14.0
    elastic-package-stack-fleet-server-1 docker.elastic.co/beats/elastic-agent:7.14.0
    
  • 7.15.0
     $ docker ps --format "{{.Names}} {{.Image}}" |grep elastic-agent
    elastic-package-stack-elastic-agent-1 docker.elastic.co/beats/elastic-agent-complete:7.15.0
    elastic-package-stack-fleet-server-1 docker.elastic.co/beats/elastic-agent-complete:7.15.0
    
  • 8.2.0
     $ docker ps --format "{{.Names}} {{.Image}}" |grep elastic-agent
    elastic-package-stack-elastic-agent-1 docker.elastic.co/elastic-agent/elastic-agent-complete:8.2.0
    elastic-package-stack-fleet-server-1 docker.elastic.co/elastic-agent/elastic-agent-complete:8.2.0
    
  • 8.16.0 (SNAPSHOT)
     $ docker ps --format "{{.Names}} {{.Image}}" |grep elastic-agent
    elastic-package-stack-elastic-agent-1 docker.elastic.co/elastic-agent/elastic-agent-wolfi:8.16.0-SNAPSHOT
    elastic-package-stack-fleet-server-1 docker.elastic.co/elastic-agent/elastic-agent-wolfi:8.16.0-SNAPSHOT
    
  • 9.0.0 (SNAPSHOT)
     $ docker ps --format "{{.Names}} {{.Image}}" |grep elastic-agent
    elastic-package-stack-elastic-agent-1 docker.elastic.co/elastic-agent/elastic-agent-wolfi:9.0.0-SNAPSHOT
    elastic-package-stack-fleet-server-1 docker.elastic.co/elastic-agent/elastic-agent-wolfi:9.0.0-SNAPSHOT
    

@mrodm mrodm force-pushed the support-elastic-agent-9.0 branch from 568547b to d918cc2 Compare September 17, 2024 10:49
@@ -45,7 +45,7 @@ fixCRLF

withGolang $env:GO_VERSION
withDocker $env:DOCKER_VERSION
withDockerCompose $env:DOCKER_COMPOSE_VERSION
withDockerCompose $env:DOCKER_COMPOSE_VERSION.Substring(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required to remove v from the version string for the chocolotey tool. Currently, this environment variable has this value v2.24.1.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, how has it worked in the past?

Copy link
Contributor Author

@mrodm mrodm Sep 18, 2024

Choose a reason for hiding this comment

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

Probably, it was failing in the past too. I realized that even if with those commands were failing, the execution of the script continued.
I see that at the beginning of the file is defined

$ErrorActionPreference = "Stop"

but it does not stopped the execution. Not sure if choco command does not return any error in that scenario, or when the error is inside a function there are some differences in how to manage the errors.

logger.Errorf("stack version not in semver format (value: %s): %v", v, err)
return elasticAgentLegacyImageName
logger.Errorf("stack version not in semver format (value: %s): %v", version, err)
return elasticAgentWolfiImageName
}

disableWolfiImages := false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it is now, the Elastic Agent image is chosen as:

  • If stack >= 8.16.0-SNAPSHOT,
    • by default it is used wolfi image_ docker.elastic.co/elastic-agent/elastic-agent-wolfi.
    • if ELASTIC_PACKAGE_DISABLE_ELASTIC_AGENT_WOLFI=true, then it is selected the Elastic Agent image as before. So if stack >= 7.15.0-SNAPSHOT, it will be used the complete image.
  • If the configuration file of the system test has base_image: complete:
    • It will be used the complete version of the Elastic Agent image if the stack version allows it.
  • If the configuration file of the system test has base_image: systemd:
    • It will be used the systemd version of the Elastic Agent image. That means docker.elastic.co/elastic-agent/elastic-agent or docker.elastic.co/beats/elastic-agent depending on the stack version.

Would that be ok? Specially if stack version is set to 9.0.0 and the environment variable to disable wolfi is set to true.

Copy link
Member

Choose a reason for hiding this comment

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

If the same set of images is going to be available in 9.0 keeping the same logic sounds good to me.

@mrodm mrodm marked this pull request as ready for review September 17, 2024 14:06
@mrodm mrodm requested a review from a team as a code owner September 17, 2024 14:06
@@ -45,7 +45,7 @@ fixCRLF

withGolang $env:GO_VERSION
withDocker $env:DOCKER_VERSION
withDockerCompose $env:DOCKER_COMPOSE_VERSION
withDockerCompose $env:DOCKER_COMPOSE_VERSION.Substring(1)
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, how has it worked in the past?

@@ -153,13 +154,13 @@ func (ac *ApplicationConfiguration) SetCurrentProfile(name string) {
// This is mandatory as "elastic-agent-complete" is available since 7.15.0-SNAPSHOT.
func selectElasticAgentImageName(version, agentBaseImage string) string {
if version == "" { // as version is optional and can be empty
return elasticAgentLegacyImageName
return elasticAgentWolfiImageName
Copy link
Member

Choose a reason for hiding this comment

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

We can adapt this to the new default, yes, if we are sure that in old versions the beats namespace is used.

@@ -14,43 +14,43 @@ import (
func TestSelectElasticAgentImageName_NoVersion(t *testing.T) {
var version string
selected := selectElasticAgentImageName(version, "")
assert.Equal(t, selected, elasticAgentLegacyImageName)
assert.Equal(t, elasticAgentWolfiImageName, selected)
Copy link
Member

Choose a reason for hiding this comment

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

In principle it LGTM. Maybe another option is to return an error if the version is empty? Not sure about the implications of the change, but would look safer to me to know where/if we are passing an empty version.

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mrodm

@mrodm mrodm merged commit f64d3fc into elastic:main Sep 19, 2024
3 of 4 checks passed
@mrodm mrodm deleted the support-elastic-agent-9.0 branch September 19, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants