Use descriptive container names #4670
Conversation
pat-s
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4670 +/- ##
==========================================
- Coverage 28.16% 28.15% -0.02%
==========================================
Files 398 398
Lines 28205 28220 +15
==========================================
Hits 7944 7944
- Misses 19555 19568 +13
- Partials 706 708 +2 ☔ View full report in Codecov by Sentry. |
pipeline/backend/docker/convert.go
Outdated
| func toContainerName(step *types.Step) string { | ||
| return "wp_" + step.UUID | ||
| func toContainerName(step *types.Step, workflowName string) string { | ||
| return "wp_" + workflowName + "-" + step.Name + "-" + step.UUID[:5] |
There was a problem hiding this comment.
What kind of UUID are we using? Some have that first part is a timestamp component so that could be a problem, not sure if it is here tho
There was a problem hiding this comment.
It seems indeed that the first part is somehow static, at least on the last part changes on "restart" runs of the same pipeline.
Hence I will change it to the last 5 digits of the generated UUID.
IMO long UUIDs make all outputs a bit more messy for no real benefit. A 5-char one should suffice and do "the job". However, I just noticed that we could actually drop the UUID at all. I thought that a unique one will be created for each pipeline, i.e. also when using "restart". This is not the case and hence such actions will conflict with a previous pipeline if "auto cancel on push" is off. |
|
Update to my previous comment: it seems that the UID is identical for pipelines on the first x digits but differs on the latter ones (e.g. from mid to end). Not sure why that is but this means instead of using the first 5 chars we should use the last 5 ones for a unique pod name. This is important to be able to restart pipelines which don't automatically cancel the previous one without getting a conflict in the container name. |
|
Maybe we should just use a simple random char generator instead of a specific one for uuids? This way we can make sure that it's completely different and we also don't have to generate the full uuid if we don't needed it anyways. |
|
Maybe you could give a use case for this? I have a server running quite some pipelines daily and we still had no case for the need to check running pipelines in containerization side. Also workflow name&step name still does not sound very useful as they will most likely be very similar if not the same between multiple repos (build, deploy etc) |
Why not? And if you state so, it would be good to also name an alternative. I don't see any. And using a random ID as of now is certainly not better. The only information we have about a pipeline it the name and the enclosing step names. Users are responsible for the names, so it is up to them to use descriptive ones that don't repeat themselves everywhere. |
|
@qwerty287 @lafriks Anything left? |
|
Personally I don't see use for this but I don't really care how containers are named as long as they don't conflict (reason why we moved away from similar naming previously) Imho better use would be to use labels to add metadata about org/repo/workflow/step for container |
|
As said, if you have dozens of builds running over days you value descriptive container names.
This is something different and doesn't help when inspecting pods interactively and wanting to know which one does what. But it can surely be added in a different PR, more metadata is always better. |
|
To be honest my personal opinion is debugging in running pods is the wrong way in any case. Besides that Im pretty sure uuids are not guaranteed to be unique anymore if you are shortening them randomly.
Why? Just use label selectors to get your pod why do we need this information in the name? |
It is not necessarily primarily about "debugging". As said, I have builds running for days sometimes (the use case doesn't matter) and logs in the UI will stop streaming at some point. It is essential to me at some point to check interactively what is happening in the pods. I do this in Having the workflow and step name in the name is really not hurting but only helps (?). I can't understand the discussion around this actually, especially as you both argue like "I never do/need this and don't care about pod names" but at the same time keep arguing about it without approval ❓️
They are generated uniquely now and are not shortened anymore. According to math calculators, the collision probability of two consecutive strings being identical is |
|
Alright, then I will step back from reviews of features I dont care about? My opinion is, adding meaningful magic to container names (in this case container names can get very long due to all the metadata controlled by users) is wrong and could lead to issues and should therefor be avoided. One example moby/moby#46928 Adding feedback even if I dont have a use case is an essential part of the review process. However I have not blocked this PR so if another maintainer is fine with this approach it can still be approved and merged. |
Cmon, I am just arguing with the same enthusiasm as you often do...no hard feeling ;)
Alright, this is a valid point in this discussion. I thought about this as well and the PR already contains this: const maxPodNameLength = 253
if len(res) > maxPodNameLength {
res = res[:maxPodNameLength]
}
And this is very welcome, I asked for it! Okay, last argument: I wouldn't have opened this PR, changed all these files and modified the generic if this "was just a cosmetic change" for me. While I agree my use case for this not being super standard, I might surely not be the only one out there. |
But this logic only applies to the Kubernetes backend and as the UUID is the last part of the name it is truncated in the worst case, leading to duplicate name issues. Besides that, I can see even more issues with the following criteria https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names How do you want to ensure these criteria? Edit: I see you have added a regex already, but I have still some questions:
Example:
I still believe this is the wrong direction and not achievable without adding a lot of complexity. |
|
Can we not use an opposide aprroach? What about adding the required metadata to the pipeline/step? We could store the agent and container/pod information and provide an API (and maybe cli/ui helper) to get those information. Drone has implemented a different approach https://docs.drone.io/runner/docker/configuration/reference/drone-tmate-enabled/ |
|
Can't we just put this behind a server env var and let the admin decide? All it takes in the code is then a one line if condition on how container names are constructed. |
|
Sure we can but this has still a lot of disadvantages as it massively limits the possible steb names for all users. E.g. this means und public instances like codeberg users cant even use their native languages for step names anymore. Long story short, for me this is devinetly the wrong way, sorry. |
|
I wouldn't set it as the default. Still, I need this so heavily for my workflows that I will have to use a forked version forever then.
I doubt that so many emoji-based stepnames exists. And we can add a warning to the setting with its limitations.
I wouldn't deploy/enable it there as users don't have access to the containers anyhow. |
|
I have also made multiple suggestions for alternative solutions. I see your use case and dont say we should provide a solution.
Its not only about emojies not even chines or cyrillic step names would work |
So what about my previous suggestion with adding it as an opt-in setting with a warning in the docs?
Maybe there is a way to "sanitize" such characters automatically somehow. But imo this is also somewhat of an edge case I'd argue, if one really opts-in and then uses non-latin characters in step names... Not sure if WP needs to account for every edge case like this in a opt-in setting then? (but overall good point I haven't thought about). |
|
I really think this approach is wrong regardless of opt-in, sorry. But maybe someone else disagrees. |
|
Can't understand your final reasoning, especially when adding it as an opt-in and outlining the potential downsides in the docs. I understand the general doubts and potential downsides and you convinced me not setting it as the default, but I am somewhat disappointed that even an opt-in is denied (as I can't understand the reasons for this argument). The metadata workaround doesn't help me in any way for the outlined use case. |
|
Then I maybe dont understand your use case good enough. My understanding: There are long running pipelines and you need to check the container logs. To do so you need to know which container is executing the step you want to inspect. This can be done by descriptive container names. But it should also be possible to lookup the container name by e.g. |
supersedes #4245
The UUID needs to stay as there can be multiple parallel runs of the same workflow which must have distinct names.