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

Add docker support #5757

Merged
merged 6 commits into from
Aug 19, 2024
Merged

Add docker support #5757

merged 6 commits into from
Aug 19, 2024

Conversation

AenBleidd
Copy link
Member

No description provided.

@AenBleidd AenBleidd force-pushed the vko_wsl_docker branch 4 times, most recently from 31be22d to 94a9736 Compare August 14, 2024 09:24
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 66 lines in your changes missing coverage. Please review.

Project coverage is 10.51%. Comparing base (39559b5) to head (0a354f5).
Report is 27 commits behind head on master.

Files Patch % Lines
lib/hostinfo.cpp 0.00% 41 Missing ⚠️
lib/cc_config.cpp 0.00% 11 Missing ⚠️
lib/wslinfo.cpp 0.00% 10 Missing ⚠️
sched/sched_types.cpp 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5757      +/-   ##
============================================
- Coverage     10.53%   10.51%   -0.02%     
  Complexity     1068     1068              
============================================
  Files           279      279              
  Lines         35888    35956      +68     
  Branches       8416     8440      +24     
============================================
  Hits           3780     3780              
- Misses        31714    31782      +68     
  Partials        394      394              
Files Coverage Δ
db/boinc_db_types.h 0.00% <ø> (ø)
lib/cc_config.h 0.00% <ø> (ø)
sched/sched_types.cpp 0.00% <0.00%> (ø)
lib/wslinfo.cpp 0.00% <0.00%> (ø)
lib/cc_config.cpp 0.00% <0.00%> (ø)
lib/hostinfo.cpp 0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

Signed-off-by: Vitalii Koshura <lestat.de.lionkur@gmail.com>
@AenBleidd
Copy link
Member Author

In this PR I have dropped the detection of 'docker-compose' since it's deprecated, and no longer supported for about two years already.
@davidpanderson, please review.
I have tested client on Windows with WSL (with and without docker) and Linux (with and without docker).
I haven't tested server part of it.

@davidpanderson
Copy link
Contributor

There are still references to docker_compose_available;
did you mean to remove these?

@AenBleidd
Copy link
Member Author

AenBleidd commented Aug 15, 2024

@davidpanderson,
No this is intended.
Before the docker v2, there was a separate tool called 'docker-compose', and now it's not supported anymore.
Since ~5 years ago, they have developed a plugin (that could be either installed together with docker engine or not) that is now called 'docker compose' (without the dash between words) that is basically the same but is actively supported.
All the changes related to the old tool made by previous committer were removed.

@davidpanderson
Copy link
Contributor

Oh, I see.

There are two things:

  • Docker is present
  • Docker is allowed (by cc_config.xml)
    Let's keep these separate, rather than combing them in 'available'.
    That will avoid ambiguous messages like
    "Docker is not installed or is not available for running task"

So with Win/WSL, each WSL distro can have or not have Docker?
If so we need to convey this in the scheduler request,
and to the plan class logic.
This needs to be discussed.
It's probably best if BOINC have its own WSL distro,
and it only reports Docker for that one.

@AenBleidd
Copy link
Member Author

@davidpanderson,

Let's keep these separate, rather than combing them in 'available'.
That will avoid ambiguous messages like
"Docker is not installed or is not available for running task"

Basically, we're following here the same logic as with the VBox, but maybe there is something I overlooked. Let's check this together on call.

So with Win/WSL, each WSL distro can have or not have Docker?

Technically yes.

If so we need to convey this in the scheduler request,
and to the plan class logic.

I believe, I did this already, but let's double check.

It's probably best if BOINC have its own WSL distro,
and it only reports Docker for that one.

Might be, but then this will require us to support this image, and update it.
We can have this option (and even provide an installer that will enable and install WSL together with our own distro, but I'd use this for the users who are less technical, and who is not able to do that manually.
But for the rest of the users I'd suggest to have the support of their distros.
This is reasonable to me (here I can provide an analogy to the VBox as well, where we also provide a BOINC installer with VBox and we still can detect and use the existing installation of VBox that was installed by the users on their own).

This needs to be discussed.

Of course, let's discuss this on our next call.

Signed-off-by: Vitalii Koshura <lestat.de.lionkur@gmail.com>
Signed-off-by: Vitalii Koshura <lestat.de.lionkur@gmail.com>
@AenBleidd
Copy link
Member Author

@davidpanderson, I have implemented all the required changes that were discussed during the call last night.
Please take a look.

@davidpanderson
Copy link
Contributor

A couple of minor things.

  • 'available' is confusing.
    Use 'allowed' to mean that it's not disallowed by the config file
    and 'present' to mean that it's there.

  • with #ifdefs I like to put the positive case first, i.e.
    #ifdef _WIN64
    #else
    #endif
    ... rather than #ifndef

  • there shouldn't be messages like
    Docker is not installed or is not available for running task
    It should say e.g.
    Docker is present
    Use of Docker is not allowed by configuration file

  • HOST_INFO::get_docker_info() and similar:
    shouldn't have 'return 1'.
    Should either be true (if boolean) or an error code
    (or void if appropriate).
    The comment should say that it populates version
    and docker_available on success

  • resource_helper:
    structs should have all-caps names
    don't need to say public: (structs are all public)
    Should have a more descriptive name
    Are 4 handles needed? why not 2? (read from out, write to in)
    We're only talking to one WSL at a time - can rs be a global rather than an argument?

  • db/boinc_db_types has
    //Docker available
    bool docker_available;
    ...
    This is not a useful comment, and I don't know what this field means.
    Also, what if a host has multiple available WSLs,
    some with Docker and some without?

Signed-off-by: Vitalii Koshura <lestat.de.lionkur@gmail.com>
@AenBleidd
Copy link
Member Author

@davidpanderson, please review again.

'available' is confusing.
Use 'allowed' to mean that it's not disallowed by the config file
and 'present' to mean that it's there.

Fixed.

with #ifdefs I like to put the positive case first, i.e.
#ifdef _WIN64
#else
#endif
... rather than #ifndef

Fixed.

there shouldn't be messages like
Docker is not installed or is not available for running task
It should say e.g.
Docker is present
Use of Docker is not allowed by configuration file

Fixed.

HOST_INFO::get_docker_info() and similar:
shouldn't have 'return 1'.
Should either be true (if boolean) or an error code
(or void if appropriate).
The comment should say that it populates version
and docker_available on success

Fixed.

resource_helper:
structs should have all-caps names

Fixed.

don't need to say public: (structs are all public)

Fixed.

Should have a more descriptive name

Fixed.

Are 4 handles needed? why not 2? (read from out, write to in)

Yes, we need 2 pipes (4 handles in total).

We're only talking to one WSL at a time - can rs be a global rather than an argument?

I prefer to follow RAII principle here and create/keep objects when they are required only.

@davidpanderson davidpanderson merged commit 38b131d into master Aug 19, 2024
145 of 146 checks passed
@davidpanderson davidpanderson deleted the vko_wsl_docker branch August 19, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Merged
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants