Skip to content

Conversation

@davidpanderson
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings August 13, 2025 16:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies the Docker cleanup logic to remove containers named "hello-world" in addition to existing BOINC containers. The change adds a conditional check to identify hello-world containers and includes them in the removal process.

  • Adds detection for "hello-world" containers during cleanup
  • Bypasses BOINC-specific checks for hello-world containers
  • Includes hello-world containers in the removal command execution

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

if (retval) continue;
if (!docker_is_boinc_name(name.c_str())) continue;
if (info.container_present(name)) continue;
if (strstr(line.c_str(), "hello-world")) {
Copy link

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

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

Using strstr() for container name matching could produce false positives. A container named 'my-hello-world-app' would match this condition. Consider using exact name matching or checking if the container name equals 'hello-world'.

Suggested change
if (strstr(line.c_str(), "hello-world")) {
if (name == "hello-world") {

Copilot uses AI. Check for mistakes.
if (!docker_is_boinc_name(name.c_str())) continue;
if (info.container_present(name)) continue;
if (strstr(line.c_str(), "hello-world")) {
// delete hello-world containers
Copy link

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

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

The comment is incomplete and doesn't explain why hello-world containers should be deleted or what creates them. Consider expanding the comment to explain the rationale, such as '// Remove hello-world containers created during Docker testing/validation'.

Suggested change
// delete hello-world containers
// Remove hello-world containers created during Docker installation/testing.
// These are not related to BOINC jobs and can be safely deleted.

Copilot uses AI. Check for mistakes.
@TeeVeeEss
Copy link
Contributor

The created names of the hello-world containers are random picked, like this example in my containers.json:

{
  "id": "124dc77c41b6246f4cd0eca38a5f83d361e8bc1f80dcd4dd7f0de9969ca265d4",
  "names": [
    "reverent_haslett"
  ],
  "image": "74cc54e27dc41bb10dc4b2226072d469509f2f22f1a3ce74f4a59661a1d44602",
  "layer": "1abc6bec1d1a2ea977dbbb62f628d1637e69dc056f29d576a71be4c8503ebe99",
  "metadata": "{\"image-name\":\"docker.io/library/hello-world:latest\",\"image-id\":\"74cc54e27dc41bb10dc4b2226072d469509f2f22f1a3ce74f4a59661a1d44602\",\"name\":\"reverent_haslett\",\"created-at\":1751120539}",
  "created": "2025-06-28T14:22:19.190258588Z",
  "flags": {
    "MountLabel": "",
    "ProcessLabel": ""
  }
}

So maybe check the metadata?

@AenBleidd
Copy link
Member

This is not needed. Now all the created 'hello-world' containers will be automatically removed.
Old created containers could be removed manually if needed, this must not be a part of the client.

@AenBleidd AenBleidd closed this Sep 19, 2025
@github-project-automation github-project-automation bot moved this from In progress to Merged in Client/Manager Sep 19, 2025
@AenBleidd AenBleidd deleted the dpa_docker24 branch September 19, 2025 00:48
@TeeVeeEss
Copy link
Contributor

Imho I disagree. This PR is probably not good enough, but -more generally speaking- : what is created by the client, should be deleted by the client.

@AenBleidd
Copy link
Member

And this will be done with the new released version.
However we will not do any clean up of leftovers made by older clients.
That is why this PR was rejected.

@TeeVeeEss
Copy link
Contributor

I agree to disagree :) Is the de-installation of the client taking care of removing the boinc user and it's files?

@AenBleidd
Copy link
Member

No, and this is done by intention.
Again: new version of BOINC will do this cleanup. For the corrent version we will check how many users are really affected and what is the real number of leftovers.
There might be the case that there is no issue at all.

@TeeVeeEss
Copy link
Contributor

Ok, you can ping me for testing the cleanup. There are ~20 hello-world containers on my host now, running version 8.2.4 x86_64-pc-linux-gnu , dating from 1751120221 to 1758666100 (jun 26 - sep 24 2025)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants