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

Run containers with --init by default to avoid leaking zombie processes. #312

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Romain-Geissler-1A
Copy link
Contributor

@Romain-Geissler-1A Romain-Geissler-1A commented Apr 27, 2024

People running containers manually usually run with pid 1 being a $SHELL, in which case waiting for child termination is something implemented already as it's the core job of a $SHELL. However with this jenkins plugin the command run is usually "cat" on Linux, which doesn't wait at all for children. To have in jenkins a situation a bit like during interactive container experience, start containers with --init so zombie processes are correctly reaped.

Testing done

Your CI still works (and it was broken before, as I had to fix it in #313 that I included in this pull request to have a green build).

Submitter checklist

People running containers manually usually run with pid 1 being a
$SHELL, in which case waiting for child termination is something
implemented already as it's the core job of a $SHELL. However with this
jenkins plugin the command run is usually "cat" on Linux, which doesn't
wait at all for children. To have in jenkins a situation a bit like
during interactive container experience, start containers with --init
so zombie processes are correctly reaped.
@Romain-Geissler-1A
Copy link
Contributor Author

@MarkEWaite @jglick Now that you have merged the pre-requisite pull request, what do you think about the principle of that one (which actually was my reason for trying to fix the unit tests in the first pull requests) ;)

@jglick
Copy link
Member

jglick commented May 28, 2024

I have no opinion and prefer not to merge any behavioral changes to this plugin ever again unless required for test suites to continue passing. Its use should be avoided. Someone else who purports to maintain the plugin is free to review and merge.

@MarkEWaite
Copy link
Contributor

@Romain-Geissler-1A I'm not a maintainer of this plugin. I think that the other people who are listed as plugin maintainers are the ones that would need to decide if they are willing to support this change and continue to maintain this change after it is released.

@Romain-Geissler-1A
Copy link
Contributor Author

Well @jglick made it clear that there is no future for this plugin but to just make it build and "work" as is, without change. Not sure what's the strategy of cloudbees to be honest, and it happens that @jglick replied just one day after we had a visite from some cloudbees representative in my company, so we couldn't ask specifically about what will happen with this plugin. Maybe I will raise this pull request via our contact there then, because I honestly fail to see how a docker plugin used by quite some instance entered such a frozen state, and how such a rather innocent pull request can affect stability of the plugin.

@bence42
Copy link

bence42 commented Oct 8, 2024

Its use should be avoided.

@jglick Do you have an alternative in mind? The declarative syntax cannot be used for e.g. generating jobs on the fly that run in parallel. All the community has is this plugin or rolling their own. So now what?

@Romain-Geissler-1A What's your view? Roll our own? Fork this one? There's clearly interest in this plugin.

@Romain-Geissler
Copy link

I would fork with care, maintaining a repository on the long run is a time consuming task, and requires commitment (if the fork is left abandoned in 6 months, it will be worse).

Now I also fail to understand how CloudBee can decide to stop supporting such an important plugin, but I work myself in a big company and there quite some choices made which I fail to understand as well. For me here paying CloudBee client who really need this shall use their internal connection to influence this choice and go back to maintaining this. On our side we are on the long run moving away to GitHub actions, so I am afraid my company won't be a great help here.

@jglick
Copy link
Member

jglick commented Oct 8, 2024

Do you have an alternative in mind?

Minimally, docker CLI commands run from sh steps, whether directly or via wrapper scripts or Groovy libraries. (#105 as a very rough example, though of course it is old, and does not take advantage of newer features in BuildKit and so on.) In the longer run, perhaps Dagger or Earthly.

The declarative syntax…

Sorry, I am not sure what this refers to.

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.

5 participants