Refactor helper binaries to save 161MB of disk space when the agent is installed and reduce RPM by 48MB #1454
Conversation
| @@ -95,6 +96,16 @@ var fRunAsConsole = flag.Bool("console", false, "run as console application (win | |||
| var fSetEnv = flag.String("setenv", "", "set an env in the configuration file in the format of KEY=VALUE") | |||
| var fStartUpErrorFile = flag.String("startup-error-file", "", "file to touch if agent can't start") | |||
|
|
|||
| // config-translator | |||
| var fConfigTranslator = flag.Bool("config-translator", false, "run in config-translator mode") | |||
There was a problem hiding this comment.
I spent some time trying to reduce the copy/paste aspect. Some of these constants are defined twice. There are some things to do to clean it up but they all end up less readable and add a lot of indirection. Open to ideas tho.
If I was forced to make one change I would put the descriptions in a constant
| @@ -38,3 +40,195 @@ func TestTranslateJsonMapToEnvConfigFile(t *testing.T) { | |||
| assert.Equal(t, expectedJson[envconfig.CWAGENT_LOG_LEVEL], actualJson[envconfig.CWAGENT_LOG_LEVEL]) | |||
| assert.Equal(t, expectedJson[envconfig.AWS_SDK_LOG_LEVEL], actualJson[envconfig.AWS_SDK_LOG_LEVEL]) | |||
| } | |||
|
|
|||
| func TestAgentConfig(t *testing.T) { | |||
There was a problem hiding this comment.
this code was moved from translator_test.go
translator/cmdutil/translator.go
Outdated
| ctx *context.Context | ||
| } | ||
|
|
||
| func NewConfigTranslator(inputOs, inputJsonFile, inputJsonDir, inputTomlFile, inputMode, inputConfig, multiConfig string) (*ConfigTranslator, error) { |
There was a problem hiding this comment.
this code was moved from the old config-translator/translator.go
|
This PR was marked stale due to lack of activity. |
chadpatel
changed the title
chadpatel
changed the title
|
This PR was marked stale due to lack of activity. |
tool/cmdwrapper/cmdwrapper.go
Outdated
| if err := cmd.Run(); err != nil { | ||
| var exitErr *exec.ExitError | ||
| if errors.As(err, &exitErr) { | ||
| log.Panicf("E! Translation process exited with non-zero status: %d, err: %v", |
There was a problem hiding this comment.
TODO: update error to not say Translation
tool/cmdwrapper/cmdwrapper.go
Outdated
| log.Printf("Executing %s with arguments: %v", paths.AgentBinaryPath, args) | ||
|
|
||
| // Use execCommand instead of exec.Command directly | ||
| cmd := execCommand(paths.AgentBinaryPath, args...) |
There was a problem hiding this comment.
TODO - we will need to first check the normal path but if this fails to resolve then we need to check for amazon-cloudwatch-agent in the same directory as the current executing process (i.e. config-translator)
There was a problem hiding this comment.
|
This PR was marked stale due to lack of activity. |
movence
force-pushed
the
patchad-config-translate-refactor
branch
from
6c4f2af to
64d101d
Compare
…-agent to save 114M on disk and 35MB on the RPM
…urther decrease deployment size
movence
force-pushed
the
patchad-config-translate-refactor
branch
from
fab8494 to
cef7fa3
Compare
tool/cmdwrapper/cmdwrapper.go
Outdated
| return "", fmt.Errorf("failed to get current executable path: %w", err) | ||
| } | ||
| execDir := filepath.Dir(execPath) | ||
| alternatePath := filepath.Join(execDir, "amazon-cloudwatch-agent") |
There was a problem hiding this comment.
nit - you could use AgentBinaryName. Not sure if that brings in unnecessary binary size tho
| return path, nil | ||
| } | ||
|
|
||
| // if not, check in the current executable's directory |
There was a problem hiding this comment.
nice! this is a great idea
movence
added
the
ready for testing
|
This PR was marked stale due to lack of activity. |
Revisions:
Description of the issue
The cloudwatch-agent has 3 helper binaries that use an excessive amount of disk
The reason is they are directly and indirectly pulling dependencies from cloudwatch-agent. To solve this problem, I updated config-downloader, config-translator and the wizard to be shims that just redirect to amazon-cloudwatch-agent binary
This works fine, the main risk is these binaries are no longer "portable", they depend on finding the path to amazon-cloudwatch-agent at runtime. I am using the same method as
start-amazon-cloudwatch-agentfor finding the pathDescription of changes
High level the approach is to maintain the same argument interface for the existing 3 commands and seamlessly move the logic in to the main binary. To do this the old commands need to keep the same args but we prefix the args when we call CWA so that there are no duplicate args.
The general approach I took was to create a new
cmdwrapperwhich offers two methods.flagAPI for pulling command line args. I considered using subcommands but things got too complicated and ugly, prefixing was simpleramazon-cloudwatch-agentand it calls it with the new flags. It remaps stdin/stdout/stderr so it appears seemless. Things like the wizard which rely on stdin still workI moved the flags in to their own separate
flagspackages which have NO dependencies (keeping the binaries small). And then the old binaries and amazon-cloudwatch-agent pull in the flags and the commands and link everything together. For the wizard I had to move a few other common constants like the inlinuxMigration.goandwindows_migration.goI added and updated unit tests wherever possible. The old translator_test.go was moved in to translatorutil_test.go as that is primarily what those tests were testing
Note: inline diff is VERY hard to follow because of all the moved code, I recommend
splitdiff or we can do a code walk-throughLicense
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
Integ test run: https://github.com/aws/amazon-cloudwatch-agent/actions/runs/15216630285
Manual testing to confirm cloudwatch agent still starts/loads
There were no prior tests that actually tested
config-translatebinary... the old tests basically just testedcmdutilso I moved those in to thecmdutiltests. We could write more tests for the shim and fortranslate.go. Tricky tests to write due to the OS coupling.Before:
After:
Wizard still works:
Downloader/Translator still work
Rough Edges
This is the flow if config-translator fails. I tried to unify the code so behavior is slightly different. We could change this if we needed to
new:
Requirements
Before commit the code, please do the following steps.
make fmtandmake fmt-shmake lint