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

Cleanup containers #124

Merged
merged 1 commit into from
Jan 13, 2016
Merged

Cleanup containers #124

merged 1 commit into from
Jan 13, 2016

Conversation

esteve
Copy link
Contributor

@esteve esteve commented Jan 11, 2016

This forces cleanup of intermediate containers. I also added =true to --rm to make the command line a bit more expressive.

Connects to #120

@esteve
Copy link
Contributor Author

esteve commented Jan 11, 2016

@dirk-thomas @tfoote How can I test this? Thanks.

@dirk-thomas
Copy link
Member

As long as --force-rm only removes the intermediate containers but not the images that looks promising. The intermediate images need to be kept otherwise we would loose all reuse of previously build images.

@tfoote Can comment on how you can test this (with the current build farm).

Regarding the =true|false suffix: I don't think this is very intuitive. Using --xxx options is a very common concept for command line tools. Adding the boolean value like this is not. Especially something like this I find very unintuitive: --rm=false.

@tfoote
Copy link
Member

tfoote commented Jan 11, 2016

We can go into a specific job config and edit the branch of ros_buildfarm that it checks out and then manually trigger it.

Ala: http://54.183.26.131:8080/view/Jbin_uT64/job/Jbin_uT64__ackermann_msgs__ubuntu_trusty_amd64__binary/configure

@esteve
Copy link
Contributor Author

esteve commented Jan 11, 2016

@dirk-thomas I agree, but since docker does not provide a --no- version of its boolean options (as is standard, though), I thought it'd be a bit more expressive this way. I can remove the =true if you like.

@tfoote I've triggered this:

http://54.183.26.131:8080/view/Jbin_uT64/job/Jbin_uT64__ackermann_msgs__ubuntu_trusty_amd64__binary/20/

@dirk-thomas
Copy link
Member

Yes please, I would say it's much cleaner and similar to common command line arguments.

The single run won't tell us much. I think we only see the effect if we start from a "clean" slave and all jobs running on that slave use this change for a while. Then we can look at the list after e.g. a day or so.

@esteve
Copy link
Contributor Author

esteve commented Jan 11, 2016

@dirk-thomas Done 01e8155

@tfoote
Copy link
Member

tfoote commented Jan 11, 2016

Yeah, it's an aggregate state thing and it will only show as effective if we deploy it for an active build cycle. If we validate it on a few jobs as not breaking then we can go ahead and deploy it and make sure to come back and check that there is not significant accumulation after the time of deployment.

@esteve
Copy link
Contributor Author

esteve commented Jan 11, 2016

I triggered this job to check that the latest changes are fine:

http://54.183.26.131:8080/view/Jbin_uT64/job/Jbin_uT64__ackermann_msgs__ubuntu_trusty_amd64__binary/21/

@tfoote
Copy link
Member

tfoote commented Jan 12, 2016

The build passed. Though I'm not sure if it took. the only place I see --force-rm is in the checkout log at the top

There are two instances of docker build where I do not see --force-rm http://54.183.26.131:8080/view/Jbin_uT64/job/Jbin_uT64__ackermann_msgs__ubuntu_trusty_amd64__binary/21/console

@esteve
Copy link
Contributor Author

esteve commented Jan 12, 2016

@tfoote there's a couple of docker build without --force-rm in the job configuration. Do the jobs need to be updated with the new generated XMLs?

@tfoote
Copy link
Member

tfoote commented Jan 12, 2016

Yeah, you're right they're baked in. The good news is we have a test farm now. If you tweak this reconfigure job http://54.183.26.131:8080/view/Manage/job/Jrel_reconfigure-jobs/configure jade on the test farm will have your changes after it reconfigures.

@esteve
Copy link
Contributor Author

esteve commented Jan 12, 2016

@esteve
Copy link
Contributor Author

esteve commented Jan 12, 2016

@esteve
Copy link
Contributor Author

esteve commented Jan 13, 2016

I believe this is ready for merging, though we will see the effects of this after a few more runs.

@tfoote
Copy link
Member

tfoote commented Jan 13, 2016

looks good. lets land it and we can leave the ticket open to check on the slaves in a few days.

@esteve
Copy link
Contributor Author

esteve commented Jan 13, 2016

@tfoote I've updated the reconfigure job to point to master, instead of to this branch.

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.

None yet

3 participants