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

Fixes https://github.com/moby/moby/issues/44378 #3082

Conversation

s4ke
Copy link
Contributor

@s4ke s4ke commented Nov 6, 2022

AssignedGenericResources in constraint_enforcer.go were falsely checked inside a case that enforced Reservations to be set Furthermore, the if statement had a missing !

Signed-off-by: Martin Braun braun@neuroforge.de

- What I did

Fixed a inverted logic bug in constraint_enforcer.go that should fix moby/moby#44378

- How I did it

I added a test for constraint_enforcer_test.go that checks for proper enforcement of generic resources being present. While doing so, I noticed that the checks for AssignedGenericResources were done inside a check for the resources. This seemed wrong, so I moved the logic outside of the if statement.

- How to test it

- Description for the changelog

Fixed a inverted logic bug in constraint_enforcer.go that should fix moby/moby#44378

@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2022

Codecov Report

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

@@            Coverage Diff            @@
##             master    #3082   +/-   ##
=========================================
  Coverage          ?   62.06%           
=========================================
  Files             ?      156           
  Lines             ?    24619           
  Branches          ?        0           
=========================================
  Hits              ?    15281           
  Misses            ?     7729           
  Partials          ?     1609           

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

@s4ke
Copy link
Contributor Author

s4ke commented Nov 8, 2022

I think that it would make a lot of people happy if this were to make it into 22.06

AssignedGenericResources in constraint_enforcer.go were falsely checked inside a case that enforced Reservations to be set
Furthermore, the if statement had a missing !

Signed-off-by: Martin Braun <braun@neuroforge.de>
@s4ke s4ke force-pushed the 44378-moby-fix-constraint-enforcer-generic-resources branch from bc59ce5 to 770a4d3 Compare November 10, 2022 21:18
@imyller
Copy link

imyller commented Nov 13, 2022

The bug fixed here prevents me from starting GPU containers with generic resource constraint and internal network for load-balancing.

+1 for including this in the next release!

@s4ke
Copy link
Contributor Author

s4ke commented Dec 6, 2022

I don't know if it's too late, but this would be great to have in the final release of 23.00 (if possible) @thaJeztah :)

@s4ke
Copy link
Contributor Author

s4ke commented Jan 8, 2023

I guess this went off the radar: here is a bump :).

@markperri
Copy link

Is it possible to make this patch on my system, or do I need to wait for an official rpm? I'm trying to avoid kubernetes, but this and the --storage-opt flag make it tough.

@s4ke
Copy link
Contributor Author

s4ke commented Jan 29, 2023

Something you could do is vendor this yourself in a fork of moby/moby and then package it yourself. A bit more work but doable.

@markperri
Copy link

Something you could do is vendor this yourself in a fork of moby/moby and then package it yourself. A bit more work but doable.

Thanks, I'll take a look.

@dperny
Copy link
Collaborator

dperny commented Feb 2, 2023

Sorry for the slow response. This looks good.

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.

docker service create doesn't work when network and generic-resource are both attached
5 participants