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 support for devices #3106

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

Add support for devices #3106

wants to merge 2 commits into from

Conversation

zikaeroh
Copy link

@zikaeroh zikaeroh commented Dec 24, 2022

- What I did

This is for:

This simply plumbs devices through everything such that swarm can specify the devices field.

- How I did it

Largely based on:

But, plumbing devices instead of ulimits.

- How to test it

Still working on this; I'm finding it pretty difficult to get all of the pieces set up.

- Description for the changelog

Add devices to the API.

65003: "os.FileMode"
65001: 0
Copy link
Author

Choose a reason for hiding this comment

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

All of these seemingly random changes came from running DOCKER_SWARMKIT_USE_CONTAINER=1 make generate. I'm not sure what the deal is there, so I split this part out to another commit to hopefully drop.

@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@67d2b1b). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3106   +/-   ##
=========================================
  Coverage          ?   62.14%           
=========================================
  Files             ?      153           
  Lines             ?    24183           
  Branches          ?        0           
=========================================
  Hits              ?    15028           
  Misses            ?     7598           
  Partials          ?     1557           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@luisfavila
Copy link

@dperny Mind having a look?

Copy link
Contributor

@olljanat olljanat left a comment

Choose a reason for hiding this comment

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

@zikaeroh generated code is fine as long CI accepts it. If you want to split then please do it way that all generated code is on single commit and your changes on another.

Secondly, you need to use your real name when sign off commits.

There have been also some changes to CI here lately so please also rebase this against of master branch.

Lastly, when you are ready then mark PR for ready for review.

@zikaeroh
Copy link
Author

Sure. Just to clarify the sign-off, does the email provided need to be a "real" address? I generally do not expose that anywhere to reduce spam and increase privacy, hence why the signoff was my default github info, and not fully fleshed out. GitHub in fact will reject pushes from my account which do not use the privacy-respecting email address, and I would rather not expose this information.

Is this approach in this PR acceptable for all of the docker-related repos? I have sent PRs for all of them, but I don't exactly want to end up in the situation where the first in the chain gets merged and then nothing else!

@olljanat
Copy link
Contributor

I have seen others using those GitHub generated emails and https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work only mention about real name.

I don't see anything else than those mentioned earlier which why this would not be accepted but of course maintainers might require some other changes.

Afaik Swarmkit features are lead by this repository so getting it merged here that it should be fine for other repos too. moby/moby mostly just define API which end users see and docker/cli define cli and compose syntaxes which users see.

PS. To make it clear I'm nobody to say anything for sure here but I have got some features accepted earlier so I know what maintainers always like to see.

@dperny
Copy link
Collaborator

dperny commented Feb 17, 2023

So, one of my concerns before reviewing was that I do think that #2682 is a better solution in the long-term. However, I don't think it's necessarily mutually exclusive with this solution, especially in the immediate term. I'll need a minute to work out why the autogenerated protobuf code has gone wonky (it tends to do that, unfortunately), but otherwise this looks good to me @zikaeroh.

Signed-off-by: Zik Aeroh <48577114+zikaeroh@users.noreply.github.com>
Signed-off-by: Zik Aeroh <48577114+zikaeroh@users.noreply.github.com>
@zikaeroh zikaeroh marked this pull request as ready for review March 5, 2023 19:38
@zikaeroh
Copy link
Author

zikaeroh commented Mar 5, 2023

I have rebased this PR and undrafted it. I have no idea what to do for the other PRs, though, as they depend on this one but I can't quite make sense of how to actually test it besides that I'm using go.work with all three repos and it compiles.

@zikaeroh
Copy link
Author

zikaeroh commented Mar 5, 2023

And if someone else wants to drive this to completion, that's totally fine. Even if you don't use this code exactly, all I did was repeat what was done for ulimits.

@olljanat
Copy link
Contributor

olljanat commented Mar 5, 2023

Looks that GitHub Actions needs approval for first time contributors here.

@zikaeroh however if you open another pull request inside of your own fork against of master branch it should run and provide feedback.

@olljanat
Copy link
Contributor

@dperny PTAL

@ConnorNelson
Copy link

So, one of my concerns before reviewing was that I do think that #2682 is a better solution in the long-term. However, I don't think it's necessarily mutually exclusive with this solution, especially in the immediate term. I'll need a minute to work out why the autogenerated protobuf code has gone wonky (it tends to do that, unfortunately), but otherwise this looks good to me @zikaeroh.

It was great to see "Perfect shouldn't be the enemy of good enough." in #3149, and it would be great if we can extend this thinking towards this PR to get basic device support, and worry about a "better solution" in the future. It seems like this project has been waiting over 5 years to do it right (#2682).

As a project maintainer looking to expand towards a multi-node infrastructure, not having feature parity with compose is unfortunately leaving me wondering if I need to take on the additional complexity of Kubernetes just to have access to the same features compose offers in a multi-node environment. Swarm as compose--but multi-node--would be so much better for my use-case!

@zikaeroh
Copy link
Author

I'm happy to rebase all of my PRs when I have some time, I've just been letting it languish as I haven't gotten any review feedback and I still haven't figured out how I'm actually supposed to test all of the different pieces together. Everything being split across 3+ repos that don't all use modules is challenging, even though I am pretty experienced with Go. I would really appreciate help on that front.

It's a bit wild that these PRs are almost a year old now.

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

6 participants