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.

Sign up for GitHub

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

Improving uniqueness of templateID string #874

Closed
wants to merge 5 commits into from
Closed

Improving uniqueness of templateID string #874

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fd2ad63
Improving uniqueness of templateID string
davidtlascelles Jan 11, 2022
485880f
Using hashCode() for templateID
davidtlascelles Jan 11, 2022
47c049b
Fixing string casting
davidtlascelles Mar 23, 2022
b84355a
Fixing string casting. I'm bad at Java
davidtlascelles Mar 23, 2022
b5353a1
Merge branch 'master' into patch-1
basil May 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ private static void adjustContainersInProgress(DockerCloud cloud, DockerTemplate
}

private static String getTemplateId(DockerTemplate template) {
final String templateId = template.getImage();
final String templateId = String.valueOf(template.hashCode());
Copy link
Member

Choose a reason for hiding this comment

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

This issue requires a more nuanced solution than this...
(which is why it's remained unsolved for so long)

There are two requirements here, and any solution needs to solve both at once:

  1. On the one hand, we want uniqueness so that different templates have different IDs.
  2. On the other hand, we want "the same template" to keep the same template ID even if the Jenkins-admin changes a setting within the template, otherwise we can easily find ourselves in a situation where we run too many containers of a given specification.

e.g. consider the following scenario:

  • we've got one cloud defined, giving us access to a small docker host
  • we've defined two templates within that cloud
  • the first template is for a container that has a tiny footprint, so small we can run loads of them, but we've set the instanceCap for that to 20
  • containers made from the second template are huge and each consume nearly 20% of the hardware resources of that cloud we've set an instanceCap for the template to just 5 instances.
  • There's a lot of builds queued up needing the heavyweight container
  • The system is currently running at full capacity (i.e. 5 heavyweight builds are running)
  • ... but the Jenkins admin edits the heavyweight template e.g. to increase the stopTimeout field (or any other minor change).
    What Jenkins MUST do is to apply the new setting to any newly-created heavyweight containers, but not run more than 5 at once (otherwise we'll blow the memory limits and everything will fail).
    BUT with the code change above, making even a minor change would alter the hashCode, which would alter the templateId, which would then mean that Jenkins would think it had zero instances of heavyweight template running, and so it would attempt to start 5 more containers ... and then all 10 builds fail because we run out of resources (e.g. out of memory, causing the oom-killer to start killing things).

i.e. the proposed change fixes one problem but (re)introduces a worse one (i.e. not honouring the instance caps).

What we really need is some form of "templateID" field that can be kept a constant between edits so that Jenkins can recognise "this new template is really the same as the old template" instead of thinking it's totally different ... while also recognising when it really is a different template.
This code change doesn't satisfy both of those criteria.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback! This explanation is really valuable. I haven't had the time to really delve into this problem, but it is a constant annoyance at work so whenever I get annoyed, I re-visit this for a few minutes and get nowhere.

Anyway, here's an idea for how I could try solving this:

  • The "Name" field can be required, and must be unique.
  • Making a new template would create a default name, e.g. "docker-###" (The current default behavior for an empty Name field is "docker", this would just append a unique value to the end, possibly an incrementing integer, maybe a hash, etc.)
  • If the name field is altered after the template is created, display some red warning text about instance capacities. Alternatively, make the field immutable and grey it out after template creation, and have some explanation in the help text advising the user to create a new template with a new, immutable name, warning that the instance capacity of the new template will be independent of the current template.

If this sounds good to you, I'll try to bump this up my priority list. Maybe even convince my manager to put it on the backlog at work.

Also, let me know if you have some implementation suggestions. I'm not very familiar with Jenkins plugin development either, so figuring out how to do my suggestions in bullet point 3 is going to be a challenge.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about maybe have a invisible-to-the-user field that was a plugin-assigned ID.
I'm not an expert on Jenkins' "Jelly" so I can't give much advice on how to do that (other than "I'm reasonably sure other plugins have use this"), but I suspect that this might be the best option.

I can certainly advise on how to add new fields, and how to handle the serialisation to/from XML etc, and help you figure out how to make it automatically populate if it's not been given a value.
...but I don't know how to make a field invisible on the configuration page, and we'd want it invisible and immutable by the user.

return templateId;
}

Expand Down