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 and optimize generator Docker image building #1045

Merged
merged 2 commits into from
Nov 21, 2023
Merged

Fix and optimize generator Docker image building #1045

merged 2 commits into from
Nov 21, 2023

Conversation

hhromic
Copy link
Contributor

@hhromic hhromic commented Nov 11, 2023

This PR implements a fix and an optimization to the generator Docker image building as described below.
To facilitate review, these two improvements were implemented in two separate commits.

  • Fix Docker image of generator always pointing to latest tag
    • new REPO_TAG variable can be used to specify the tag/branch to use for a build
    • the Makefile default is to use the branch of the current repository HEAD
    • the Dockerfile default (if building manually with Docker) is main
    • updated CircleCI configuration to apply the correct REPO_TAG values
  • Optimize Docker image of generator
    • use multi-stage build to reduce final image size
    • ensure to use the same Debian version (bookworm) for the builder and final images
    • use --no-install-recommends to reduce unnecessary dependency downloads
    • removed unnecessary unzip dependency in the image
    • use modern syntax (using equal sign) for ENV Dockerfile directive

The first commit addresses the problem that the Docker images for the generator currently always use the latest tagged version in the repository, instead of the latest commit of the branch pointed by the Docker image tag. This comit now allows to build Docker images for the generator using any tag or branch, no matter which tag is latest in the repository.

The most evident issue of the above is that the prom/snmp-generator:main image in Docker Hub (and also the corresponding image in Quay.io) actually contains the generator binary for the latest tag v0.24.1 instead of a binary generated from the latest main branch as suggested by the image tag.

I spent some signifcant time trying to figure out why a newly built image of the generator was unable to process the current generator.yml configuration file in the main branch until I realized that the Dockerfile is fixed to @latest. This PR now allows the generator to work out of the box as shown below.

At the time of this writing, if you try to run make docker-generate, it fails because the generator binary in the built prom/snmp-generator:main image does not understand the latest changes done in the main branch since the latest tag:

$ git status
On branch main
$ cd generator
$ make docker-generate
(...)
docker run -ti -v "/home/hhromic/snmp_exporter/generator:/opt/" "prom/snmp-generator:main" generate
ts=2023-11-11T19:17:56.898Z caller=net_snmp.go:162 level=info msg="Loading MIBs" from=mibs
ts=2023-11-11T19:17:57.127Z caller=main.go:132 level=error msg="Error generating config netsnmp" err="error parsing yml config: yaml: unmarshal errors:\n  line 159: field scale not found in type main.plain\n  line 602: field scale not found in type main.plain\n  line 603: field offset not found in type main.plain"
make: *** [Makefile:86: docker-generate] Error 1

With this PR, the binary in the image is actually built from the main branch and it works as expected:

$ git status
On branch generator-docker-image
$ cd generator
$ make docker-generate REPO_TAG=main
(...)
docker run -ti -v "/home/hhromic/snmp_exporter/generator:/opt/" "prom/snmp-generator:main" generate
ts=2023-11-11T19:20:09.693Z caller=net_snmp.go:175 level=info msg="Loading MIBs" from=mibs
ts=2023-11-11T19:20:09.942Z caller=main.go:53 level=info msg="Generating config for module" module=paloalto_fw
ts=2023-11-11T19:20:09.970Z caller=main.go:68 level=info msg="Generated metrics" module=paloalto_fw metrics=204
(...)
ts=2023-11-11T19:20:11.813Z caller=main.go:93 level=info msg="Config written" file=/opt/snmp.yml

Note: If this PR is merged, it will not be necessary to provide REPO_TAG while in the main branch.

The second commit optimizes the Docker image size by using a multi-stage build and removing unnecessary dependencies.

Image sizes before and after the optimization:

REPOSITORY            TAG       IMAGE ID       CREATED       SIZE
prom/snmp-generator   main      63565e35e294   7 hours ago   931MB
REPOSITORY            TAG       IMAGE ID       CREATED          SIZE
prom/snmp-generator   main      d140116eda2c   38 minutes ago   144MB

I hope you consider these improvements and let me know if you have any concerns.
If you simply don't want to include these changes, feel free to drop this PR. No worries.

* new `REPO_TAG` variable can be used to specify the tag/branch to use for a build
* the Makefile default is to use the branch of the current repository `HEAD`
* the Dockerfile default (if building manually with Docker) is `main`
* updated CircleCI configuration to apply the correct `REPO_TAG` values

Signed-off-by: Hugo Hromic <[email protected]>
* use multi-stage build to reduce final image size
* ensure to use the same Debian version (`bookworm`) for the builder and final images
* use `--no-install-recommends` to reduce unnecessary dependency downloads
* removed unnecessary `unzip` dependency in the image
* use modern syntax (using equal sign) for `ENV` Dockerfile directive

Signed-off-by: Hugo Hromic <[email protected]>
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks for all the debugging of this build process.

@SuperQ SuperQ merged commit db7cb38 into prometheus:main Nov 21, 2023
6 checks passed
@hhromic hhromic deleted the generator-docker-image branch November 21, 2023 10:53
SuperQ added a commit that referenced this pull request Nov 24, 2023
* [ENHANCEMENT] generator: Add support for subsequent address family #782
* [ENHANCEMENT] generator: Fix lookups to match OIDs closer to the index OID. #828
* [FEATURE] Add a scaling factor #1026
* [FEATURE] generator: Enable passing input file, output file, and mibs dir as flags #1028
* [FEATURE] Add an offset factor #1029
* [BUGFIX] Fix and optimize generator Docker image building #1045

snmp.yml changes:

* Override `bsnAPName` to DisplayString #660
* Import TP-Link EAP MIB  #833
* Updated Mikrotik neighbor indexes make them unique #986
* Update PowerNet MIB to v4.5.1 #1003
* Refactor HOST-RESOURCES-MIB #1027
* Update keepalived MIB files to latest version #1044

Signed-off-by: SuperQ <[email protected]>
@SuperQ SuperQ mentioned this pull request Nov 24, 2023
SuperQ added a commit that referenced this pull request Nov 26, 2023
* [ENHANCEMENT] generator: Add support for subsequent address family #782
* [ENHANCEMENT] generator: Fix lookups to match OIDs closer to the index OID. #828
* [FEATURE] Add a scaling factor #1026
* [FEATURE] generator: Enable passing input file, output file, and mibs dir as flags #1028
* [FEATURE] Add an offset factor #1029
* [BUGFIX] Fix and optimize generator Docker image building #1045

snmp.yml changes:

* Override `bsnAPName` to DisplayString #660
* Import TP-Link EAP MIB  #833
* Updated Mikrotik neighbor indexes make them unique #986
* Update PowerNet MIB to v4.5.1 #1003
* Refactor HOST-RESOURCES-MIB #1027
* Update keepalived MIB files to latest version #1044

Signed-off-by: SuperQ <[email protected]>
SuperQ added a commit that referenced this pull request Dec 6, 2023
* [ENHANCEMENT] generator: Add support for subsequent address family #782
* [ENHANCEMENT] generator: Fix lookups to match OIDs closer to the index OID. #828
* [FEATURE] Add a scaling factor #1026
* [FEATURE] generator: Enable passing input file, output file, and mibs dir as flags #1028
* [FEATURE] Add an offset factor #1029
* [BUGFIX] Fix and optimize generator Docker image building #1045

snmp.yml changes:

* Override `bsnAPName` to DisplayString #660
* Import TP-Link EAP MIB  #833
* Updated Mikrotik neighbor indexes make them unique #986
* Update PowerNet MIB to v4.5.1 #1003
* Refactor HOST-RESOURCES-MIB #1027
* Update keepalived MIB files to latest version #1044

Signed-off-by: SuperQ <[email protected]>
SuperQ added a commit that referenced this pull request Dec 10, 2023
* [ENHANCEMENT] generator: Add support for subsequent address family #782
* [ENHANCEMENT] generator: Fix lookups to match OIDs closer to the index OID. #828
* [FEATURE] Add a scaling factor #1026
* [FEATURE] generator: Enable passing input file, output file, and mibs dir as flags #1028
* [FEATURE] Add an offset factor #1029
* [BUGFIX] Fix and optimize generator Docker image building #1045

snmp.yml changes:

* Override `bsnAPName` to DisplayString #660
* Import TP-Link EAP MIB  #833
* Updated Mikrotik neighbor indexes make them unique #986
* Update PowerNet MIB to v4.5.1 #1003
* Refactor HOST-RESOURCES-MIB #1027
* Update keepalived MIB files to latest version #1044

Signed-off-by: SuperQ <[email protected]>
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.

2 participants