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

fix: use declarative style for complete variant of the elastic-agent #28526

Merged
60 changes: 25 additions & 35 deletions dev-tools/mage/dockerbuilder.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,29 +66,25 @@ func (b *dockerBuilder) Build() error {
return err
}

// We always have at least one default variant
variants := append([]string{""}, b.PackageSpec.Variants...)
for _, variant := range variants {
if err := b.prepareBuild(variant); err != nil {
return errors.Wrap(err, "failed to prepare build")
}
if err := b.prepareBuild(); err != nil {
return errors.Wrap(err, "failed to prepare build")
}

tag, err := b.dockerBuild(variant)
tries := 3
for err != nil && tries != 0 {
fmt.Println(">> Building docker images again (after 10 s)")
// This sleep is to avoid hitting the docker build issues when resources are not available.
time.Sleep(time.Second * 10)
tag, err = b.dockerBuild(variant)
tries -= 1
}
if err != nil {
return errors.Wrap(err, "failed to build docker")
}
tag, err := b.dockerBuild()
tries := 3
for err != nil && tries != 0 {
fmt.Println(">> Building docker images again (after 10 s)")
// This sleep is to avoid hitting the docker build issues when resources are not available.
time.Sleep(time.Second * 10)
tag, err = b.dockerBuild()
tries -= 1
}
if err != nil {
return errors.Wrap(err, "failed to build docker")
}

if err := b.dockerSave(tag, variant); err != nil {
return errors.Wrap(err, "failed to save docker as artifact")
}
if err := b.dockerSave(tag); err != nil {
return errors.Wrap(err, "failed to save docker as artifact")
}

return nil
Expand Down Expand Up @@ -124,7 +120,7 @@ func (b *dockerBuilder) copyFiles() error {
return nil
}

func (b *dockerBuilder) prepareBuild(variant string) error {
func (b *dockerBuilder) prepareBuild() error {
elasticBeatsDir, err := ElasticBeatsDir()
if err != nil {
return err
Expand All @@ -134,7 +130,6 @@ func (b *dockerBuilder) prepareBuild(variant string) error {
data := map[string]interface{}{
"ExposePorts": b.exposePorts(),
"ModulesDirs": b.modulesDirs(),
"Variant": variant,
}

err = filepath.Walk(templatesDir, func(path string, info os.FileInfo, _ error) error {
Expand Down Expand Up @@ -194,22 +189,18 @@ func (b *dockerBuilder) expandDockerfile(templatesDir string, data map[string]in
return nil
}

func (b *dockerBuilder) dockerBuild(variant string) (string, error) {
imageName := b.imageName
if variant != "" {
imageName = fmt.Sprintf("%s-%s", imageName, variant)
}
taggedImageName := fmt.Sprintf("%s:%s", imageName, b.Version)
func (b *dockerBuilder) dockerBuild() (string, error) {
tag := fmt.Sprintf("%s:%s", b.imageName, b.Version)
if b.Snapshot {
taggedImageName = taggedImageName + "-SNAPSHOT"
tag = tag + "-SNAPSHOT"
}
if repository, _ := b.ExtraVars["repository"]; repository != "" {
taggedImageName = fmt.Sprintf("%s/%s", repository, taggedImageName)
tag = fmt.Sprintf("%s/%s", repository, tag)
}
return taggedImageName, sh.Run("docker", "build", "-t", taggedImageName, b.buildDir)
return tag, sh.Run("docker", "build", "-t", tag, b.buildDir)
}

func (b *dockerBuilder) dockerSave(tag string, variant string) error {
func (b *dockerBuilder) dockerSave(tag string) error {
if _, err := os.Stat(distributionsDir); os.IsNotExist(err) {
err := os.MkdirAll(distributionsDir, 0750)
if err != nil {
Expand All @@ -220,8 +211,7 @@ func (b *dockerBuilder) dockerSave(tag string, variant string) error {
outputFile := b.OutputFile
if outputFile == "" {
outputTar, err := b.Expand(defaultBinaryName+".docker.tar.gz", map[string]interface{}{
"Name": b.imageName,
"Variant": variant,
"Name": b.imageName,
})
if err != nil {
return err
Expand Down
7 changes: 2 additions & 5 deletions dev-tools/mage/pkgtypes.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ const (
packageStagingDir = "build/package"

// defaultBinaryName specifies the output file for zip and tar.gz.
defaultBinaryName = "{{.Name}}-{{if .Variant}}{{.Variant}}-{{end}}{{.Version}}{{if .Snapshot}}-SNAPSHOT{{end}}{{if .OS}}-{{.OS}}{{end}}{{if .Arch}}-{{.Arch}}{{end}}"
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be represented in the packages.yml definition? If this is needed, maybe output_file (used here) can be set in agent_docker_complete_spec.

(I see output_file is not defined anywhere, maybe you find surprises if you try to use it 😬)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about it, this line is part of the revert of the initial commits. Maybe @andrewvc can help us out here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsoriano if you agree, I'd like to merge this one, to avoid more "variants" to appear/be merged (see #28597)

defaultBinaryName = "{{.Name}}-{{.Version}}{{if .Snapshot}}-SNAPSHOT{{end}}{{if .OS}}-{{.OS}}{{end}}{{if .Arch}}-{{.Arch}}{{end}}"
)

// PackageType defines the file format of the package (e.g. zip, rpm, etc).
Expand Down Expand Up @@ -90,7 +90,6 @@ type PackageSpec struct {
Files map[string]PackageFile `yaml:"files"`
OutputFile string `yaml:"output_file,omitempty"` // Optional
ExtraVars map[string]string `yaml:"extra_vars,omitempty"` // Optional
Variants []string `yaml:"variants"` // Optional

evalContext map[string]interface{}
packageDir string
Expand Down Expand Up @@ -329,10 +328,8 @@ func (s *PackageSpec) ExtraVar(key, value string) {

// Expand expands a templated string using data from the spec.
func (s PackageSpec) Expand(in string, args ...map[string]interface{}) (string, error) {
// Assign a default value for variant since it's not always passed in
return expandTemplate("inline", in, FuncMap,

EnvMap(append([]map[string]interface{}{s.evalContext, {"Variant": ""}, s.toMap()}, args...)...))
EnvMap(append([]map[string]interface{}{s.evalContext, s.toMap()}, args...)...))
}

// MustExpand expands a templated string using data from the spec. It panics if
Expand Down
39 changes: 33 additions & 6 deletions dev-tools/packaging/packages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,11 @@ shared:
image_name: '{{.BeatName}}-cloud'
repository: 'docker.elastic.co/beats-ci'

- &agent_docker_complete_spec
<<: *agent_docker_spec
extra_vars:
image_name: '{{.BeatName}}-complete'

# Deb/RPM spec for community beats.
- &deb_rpm_spec
<<: *common
Expand Down Expand Up @@ -1007,9 +1012,20 @@ specs:
<<: *agent_docker_spec
<<: *elastic_docker_spec
<<: *elastic_license_for_binaries
# This image gets a 'complete' variant for synthetics and other large
# packages too big to fit in the main image
variants: ["complete"]
files:
'{{.BeatName}}{{.BinaryExt}}':
source: ./build/golang-crossbuild/{{.BeatName}}-{{.GOOS}}-{{.Platform.Arch}}{{.BinaryExt}}

# Complete image gets a 'complete' variant for synthetics and other large
# packages too big to fit in the main image
- os: linux
arch: amd64
types: [docker]
spec:
<<: *agent_docker_spec
<<: *agent_docker_complete_spec
<<: *elastic_docker_spec
<<: *elastic_license_for_binaries
files:
'{{.BeatName}}{{.BinaryExt}}':
source: ./build/golang-crossbuild/{{.BeatName}}-{{.GOOS}}-{{.Platform.Arch}}{{.BinaryExt}}
Expand Down Expand Up @@ -1046,9 +1062,20 @@ specs:
<<: *agent_docker_arm_spec
<<: *elastic_docker_spec
<<: *elastic_license_for_binaries
# This image gets a 'complete' variant for synthetics and other large
# packages too big to fit in the main image
variants: ["complete"]
files:
'{{.BeatName}}{{.BinaryExt}}':
source: ./build/golang-crossbuild/{{.BeatName}}-{{.GOOS}}-{{.Platform.Arch}}{{.BinaryExt}}

# Complete image gets a 'complete' variant for synthetics and other large
# packages too big to fit in the main image
- os: linux
arch: arm64
types: [docker]
spec:
<<: *agent_docker_arm_spec
<<: *agent_docker_complete_spec
<<: *elastic_docker_spec
<<: *elastic_license_for_binaries
files:
'{{.BeatName}}{{.BinaryExt}}':
source: ./build/golang-crossbuild/{{.BeatName}}-{{.GOOS}}-{{.Platform.Arch}}{{.BinaryExt}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,6 @@ RUN readlink -f {{ $beatBinary }} | xargs setcap {{ .linux_capabilities }}

FROM {{ .from }}

# Contains the elastic agent image variant, an empty string for the standard variant
# or "complete" for the bigger one.
ENV ELASTIC_AGENT_IMAGE_VARIANT={{.Variant}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we still need this environment variable for the complete variant, we can enclose it in an if/else block:

{{- if (and (contains .image_name "-complete")) }}
ENV ELASTIC_AGENT_IMAGE_VARIANT=complete

Copy link
Member

Choose a reason for hiding this comment

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

See #27052 (comment)

I wonder if adding a metadata could be an option, though the docker tag contains that value anywawy.

I was not able to find any references for ELASTIC_AGENT_IMAGE_VARIANT

Copy link
Member

Choose a reason for hiding this comment

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

maybe?

image_name: {{ .image_name }}

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 agree that labels or metadata would be better to consider

Copy link
Member

Choose a reason for hiding this comment

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

BTW, my comment was just a comment, so therefore it's not blocking and I don't see any reasons why ELASTIC_AGENT_IMAGE_VARIANT should be kept

cc @andrewvc , can you please help to understand if removing is harmless?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's currently unused, so can be deleted.

ENV BEAT_SETUID_AS={{ .user }}

{{- if contains .from "ubi-minimal" }}
Expand All @@ -58,7 +55,7 @@ RUN case $(arch) in aarch64) YUM_FLAGS="-x bind-license";; esac; \
(exit $exit_code)
{{- end }}

{{- if (and (eq .Variant "complete") (not (contains .from "ubi-minimal"))) }}
{{- if (and (contains .image_name "-complete") (not (contains .from "ubi-minimal"))) }}
RUN for iter in {1..10}; do \
yum -y install atk cups gtk gdk xrandr pango libXcomposite libXcursor libXdamage \
libXext libXi libXtst cups-libs libXScrnSaver libXrandr GConf2 \
Expand Down Expand Up @@ -156,7 +153,7 @@ RUN mkdir /app
{{- else }}
RUN groupadd --gid 1000 {{ .BeatName }}
RUN useradd -M --uid 1000 --gid 1000 --groups 0 --home {{ $beatHome }} {{ .user }}
{{- if (and (eq .Variant "complete") (not (contains .from "ubi-minimal"))) }}
{{- if (and (contains .image_name "-complete") (not (contains .from "ubi-minimal"))) }}
RUN chown {{ .user }} $NODE_PATH
{{- end }}
{{- if contains .image_name "-cloud" }}
Expand All @@ -167,7 +164,7 @@ RUN chown {{ .user }} /app
{{- end }}
USER {{ .user }}

{{- if (and (eq .Variant "complete") (not (contains .from "ubi-minimal"))) }}
{{- if (and (contains .image_name "-complete") (not (contains .from "ubi-minimal"))) }}
# Setup synthetics env vars
ENV ELASTIC_SYNTHETICS_CAPABLE=true
ENV SUITES_DIR={{ $beatHome }}/suites
Expand Down
4 changes: 2 additions & 2 deletions x-pack/elastic-agent/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,6 @@ If you are in the 7.13 branch, this will create the `docker.elastic.co/beats/ela
elastic-package stack up --version=7.13.0-SNAPSHOT -v
```

Please note that the docker container is built in both standard and 'offline' variants.
The 'offline' variant contains extra files, like the chromium browser, that are too large
Please note that the docker container is built in both standard and 'complete' variants.
The 'complete' variant contains extra files, like the chromium browser, that are too large
for the standard variant.