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

Shorten docker container name #1798

Merged
merged 2 commits into from
Apr 25, 2024
Merged

Conversation

mrodm
Copy link
Contributor

@mrodm mrodm commented Apr 23, 2024

Relates #787
Relates elastic/integrations#9660

Until now the elastic agent names created (hostname) are set as:
elastic-agent-<package>-<data-stream>-<run_id>

Using that pattern, it caused that for some packages the resulting name is longer than 64 characters causing an error in docker-compose.

 $ echo -n "elastic-agent-carbon_black_cloud-asset_vulnerability_summary-62403" |wc -c 
66

This PR changes the prefix to be just agent. For the example above it results in 58 characters:

 $ echo -n "agent-carbon_black_cloud-asset_vulnerability_summary-62403" |wc -c 
58

@mrodm mrodm requested a review from a team April 23, 2024 17:24
@mrodm mrodm self-assigned this Apr 23, 2024
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Looks good, but I wonder if we can have a more future-proof approach.

@@ -163,7 +164,7 @@ func (d *DockerComposeAgentDeployer) SetUp(ctx context.Context, agentInfo AgentI
}

// Service name defined in the docker-compose file
agentInfo.Name = dockerTestAgentNamePrefix
agentInfo.Name = dockerTestAgentServiceName
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if with this solution we are still at risk of having the same issue if some package or data stream name is longer.

Could we truncate the package/data stream part?

So for elastic-agent-carbon_black_cloud-asset_vulnerability_summary-62403 we'd have the truncated version elastic-agent-carbon_black_cloud-asset_vulnerability_s-62403.

I would be ok though with applying both changes, the change in the prefix and the change to truncate.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe we could just call it elastic-agent plus the run id, that should be unique: elastic-agent-62403.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal to add the package and datastream was to help the user to find the agent in the UI. It's true that as it is right now, there could be issues again if some package or data stream name is longer, so this solution would not work if that happen.

Until now the agent was set as docker-fleet-agent. Maybe it's not needed to set in the agent name all that information, since the agents could be found filtering by the Agent Policy. Do you thin that would be good @jsoriano ?

So I think it could be set just adding as a suffix the RunID: elastic-agent-12345

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed here: a161483

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mrodm

@mrodm mrodm requested a review from jsoriano April 24, 2024 17:23
@mrodm mrodm merged commit e31daf2 into elastic:main Apr 25, 2024
3 checks passed
@mrodm mrodm deleted the shorten_elastic_agent_names branch April 25, 2024 08:23
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