wilfred-s commented on a change in pull request #219:
URL:
https://github.com/apache/incubator-yunikorn-core/pull/219#discussion_r522586803
##########
File path: pkg/common/configs/configvalidator.go
##########
@@ -75,65 +75,100 @@ func checkACL(acl string) error {
// Check the queue resource configuration settings.
// - child or children cannot have higher maximum or guaranteed limits than
parents
// - children (added together) cannot have a higher guaranteed setting than a
parent
-func checkResourceConfigurationsForQueue(cur *QueueConfig, parent
*QueueConfig) error {
+func checkResourceConfigurationsForQueue(cur QueueConfig, parentMax
*resources.Resource, parentGuaranteed *resources.Resource) error {
// If cur has children, make sure sum of children's guaranteed <=
cur.guaranteed
- curGuaranteedRes, err :=
resources.NewResourceFromConf(cur.Resources.Guaranteed)
+ actualGuaranteed, err :=
resources.NewResourceFromConf(cur.Resources.Guaranteed)
if err != nil {
return err
}
- if curGuaranteedRes.HasNegativeValue() {
- return fmt.Errorf("invalid guaranteed resource %v for queue %s,
cannot be negative", curGuaranteedRes, cur.Name)
+ if actualGuaranteed.HasNegativeValue() {
+ return fmt.Errorf("invalid guaranteed resource %v for queue %s,
cannot be negative", actualGuaranteed, cur.Name)
}
- curMaxRes, err := resources.NewResourceFromConf(cur.Resources.Max)
+ actualMax, err := resources.NewResourceFromConf(cur.Resources.Max)
if err != nil {
return err
}
- if curMaxRes.HasNegativeValue() {
- return fmt.Errorf("invalid max resource %v for queue %s, cannot
be negative", curMaxRes, cur.Name)
+ if actualMax.HasNegativeValue() {
+ return fmt.Errorf("invalid max resource %v for queue %s, cannot
be negative", actualMax, cur.Name)
}
+ maxToPass := resources.ComponentWiseMinPermissive(actualMax, parentMax)
+ guaranteedToPass := resources.ComponentWiseMinPermissive(maxToPass,
actualGuaranteed)
+
if len(cur.Queues) > 0 {
// Check children
for i := range cur.Queues {
- if err :=
checkResourceConfigurationsForQueue(&cur.Queues[i], cur); err != nil {
+ if err =
checkResourceConfigurationsForQueue(cur.Queues[i], maxToPass,
guaranteedToPass); err != nil {
return err
}
}
- sum := resources.NewResource()
- for _, child := range cur.Queues {
- childGuaranteed, err :=
resources.NewResourceFromConf(child.Resources.Guaranteed)
- if err != nil {
- return err
- }
- sum.AddTo(childGuaranteed)
+ err = checkGuaranteedResource(cur.Queues, actualGuaranteed,
parentGuaranteed, cur.Name)
+ if err != nil {
+ return err
}
+ }
- if cur.Resources.Guaranteed != nil {
- if !resources.FitIn(curGuaranteedRes, sum) {
- return fmt.Errorf("queue %s has
guaranteed-resources (%v) smaller than sum of children guaranteed resources
(%v)", cur.Name, curGuaranteedRes, sum)
- }
- } else {
- cur.Resources.Guaranteed = sum.ToConf()
Review comment:
This `ToConf()` was a new function introduced in the previous PR for
this issue, (see this
[diff](https://github.com/apache/incubator-yunikorn-core/pull/215/files#diff-3c6f56d55b9195cdb71e2693fa0cfce3f28f63a8ff144dcb8e80bcb468c73612R116).
It is the only place it was used we now need to revert the add for this
function and the tests in the Resource object.
##########
File path: pkg/common/configs/configvalidator.go
##########
@@ -75,65 +75,100 @@ func checkACL(acl string) error {
// Check the queue resource configuration settings.
// - child or children cannot have higher maximum or guaranteed limits than
parents
// - children (added together) cannot have a higher guaranteed setting than a
parent
-func checkResourceConfigurationsForQueue(cur *QueueConfig, parent
*QueueConfig) error {
+func checkResourceConfigurationsForQueue(cur QueueConfig, parentMax
*resources.Resource, parentGuaranteed *resources.Resource) error {
// If cur has children, make sure sum of children's guaranteed <=
cur.guaranteed
- curGuaranteedRes, err :=
resources.NewResourceFromConf(cur.Resources.Guaranteed)
+ actualGuaranteed, err :=
resources.NewResourceFromConf(cur.Resources.Guaranteed)
if err != nil {
return err
}
- if curGuaranteedRes.HasNegativeValue() {
- return fmt.Errorf("invalid guaranteed resource %v for queue %s,
cannot be negative", curGuaranteedRes, cur.Name)
+ if actualGuaranteed.HasNegativeValue() {
+ return fmt.Errorf("invalid guaranteed resource %v for queue %s,
cannot be negative", actualGuaranteed, cur.Name)
Review comment:
`HasNegativeValue()` is only called after a `NewResourceFromConf()` why
have we not folded the check into the creation of the new resource from the
config? Having a negative resource in the configuration should immediately be a
failure.
We had the split into a config check and later a create of the resource as
the processing was split. Now that the processing is one step we should also do
the same for the check inside the create.
##########
File path: pkg/common/resources/resources.go
##########
@@ -742,6 +743,37 @@ func ComponentWiseMin(left, right *Resource) *Resource {
return out
}
+// Returns a new Resource with the smallest value for each quantity in the
Resources
+// If either Resource passed in is nil the other Resource is returned
+// If a Resource type is missing from one of the Resource, it is considered
empty and the quantity from the other Resource is returned
+func ComponentWiseMinPermissive(left, right *Resource) *Resource {
+ out := NewResource()
+ if right == nil && left == nil {
+ return out
+ }
Review comment:
Should this not return a nil value?
if max for parent is nil and the max for parent.child is nil the result
should be a nil also. Otherwise we have a zero max as a result.
##########
File path: pkg/common/configs/configvalidator.go
##########
@@ -75,65 +75,100 @@ func checkACL(acl string) error {
// Check the queue resource configuration settings.
// - child or children cannot have higher maximum or guaranteed limits than
parents
// - children (added together) cannot have a higher guaranteed setting than a
parent
-func checkResourceConfigurationsForQueue(cur *QueueConfig, parent
*QueueConfig) error {
+func checkResourceConfigurationsForQueue(cur QueueConfig, parentMax
*resources.Resource, parentGuaranteed *resources.Resource) error {
Review comment:
You do not need to repeat the type for the parameter if it is the same:
i.e. `parentMax, parentGuaranteed *resources.Resource`
This should be fixed in multiple places in this change.
##########
File path: pkg/common/resources/resources.go
##########
@@ -771,6 +803,13 @@ func IsZero(zero *Resource) bool {
return true
}
+func IsEmpty(r *Resource) bool {
+ if r == nil {
+ return true
+ }
+ return len(r.Resources) == 0
+}
+
Review comment:
This does nothing more than `IsZero()`. An empty resource as you have
defined here is always a zero resource.
This is also only called after a `NewResourceFromConf()` I think we need to
fix that one method to do all the checks and in the config validation be able
to handle a `nil` resource to be returned in the case that nothing is set.
##########
File path: pkg/common/resources/resources.go
##########
@@ -742,6 +743,37 @@ func ComponentWiseMin(left, right *Resource) *Resource {
return out
}
+// Returns a new Resource with the smallest value for each quantity in the
Resources
+// If either Resource passed in is nil the other Resource is returned
+// If a Resource type is missing from one of the Resource, it is considered
empty and the quantity from the other Resource is returned
+func ComponentWiseMinPermissive(left, right *Resource) *Resource {
+ out := NewResource()
+ if right == nil && left == nil {
+ return out
+ }
+ if left == nil {
+ return right
+ }
+ if right == nil {
+ return left
+ }
Review comment:
Should we not return a cloned resource in these cases. Returning the
same pointer could cause issues..
##########
File path: pkg/common/configs/configvalidator.go
##########
@@ -75,65 +75,100 @@ func checkACL(acl string) error {
// Check the queue resource configuration settings.
// - child or children cannot have higher maximum or guaranteed limits than
parents
// - children (added together) cannot have a higher guaranteed setting than a
parent
-func checkResourceConfigurationsForQueue(cur *QueueConfig, parent
*QueueConfig) error {
+func checkResourceConfigurationsForQueue(cur QueueConfig, parentMax
*resources.Resource, parentGuaranteed *resources.Resource) error {
// If cur has children, make sure sum of children's guaranteed <=
cur.guaranteed
- curGuaranteedRes, err :=
resources.NewResourceFromConf(cur.Resources.Guaranteed)
+ actualGuaranteed, err :=
resources.NewResourceFromConf(cur.Resources.Guaranteed)
if err != nil {
return err
}
- if curGuaranteedRes.HasNegativeValue() {
- return fmt.Errorf("invalid guaranteed resource %v for queue %s,
cannot be negative", curGuaranteedRes, cur.Name)
+ if actualGuaranteed.HasNegativeValue() {
+ return fmt.Errorf("invalid guaranteed resource %v for queue %s,
cannot be negative", actualGuaranteed, cur.Name)
}
- curMaxRes, err := resources.NewResourceFromConf(cur.Resources.Max)
+ actualMax, err := resources.NewResourceFromConf(cur.Resources.Max)
if err != nil {
return err
}
- if curMaxRes.HasNegativeValue() {
- return fmt.Errorf("invalid max resource %v for queue %s, cannot
be negative", curMaxRes, cur.Name)
+ if actualMax.HasNegativeValue() {
+ return fmt.Errorf("invalid max resource %v for queue %s, cannot
be negative", actualMax, cur.Name)
}
+ maxToPass := resources.ComponentWiseMinPermissive(actualMax, parentMax)
+ guaranteedToPass := resources.ComponentWiseMinPermissive(maxToPass,
actualGuaranteed)
+
if len(cur.Queues) > 0 {
// Check children
for i := range cur.Queues {
- if err :=
checkResourceConfigurationsForQueue(&cur.Queues[i], cur); err != nil {
+ if err =
checkResourceConfigurationsForQueue(cur.Queues[i], maxToPass,
guaranteedToPass); err != nil {
return err
}
}
- sum := resources.NewResource()
- for _, child := range cur.Queues {
- childGuaranteed, err :=
resources.NewResourceFromConf(child.Resources.Guaranteed)
- if err != nil {
- return err
- }
- sum.AddTo(childGuaranteed)
+ err = checkGuaranteedResource(cur.Queues, actualGuaranteed,
parentGuaranteed, cur.Name)
+ if err != nil {
+ return err
}
+ }
- if cur.Resources.Guaranteed != nil {
- if !resources.FitIn(curGuaranteedRes, sum) {
- return fmt.Errorf("queue %s has
guaranteed-resources (%v) smaller than sum of children guaranteed resources
(%v)", cur.Name, curGuaranteedRes, sum)
- }
- } else {
- cur.Resources.Guaranteed = sum.ToConf()
+ // If max resource exist, check guaranteed fits in max, cur.max fit in
parent.max
+ err = checkMaxResource(parentMax, actualMax, actualGuaranteed, cur.Name)
+ if err != nil {
+ return err
+ }
+ return nil
+}
+
+func checkGuaranteedResource(childQueues []QueueConfig, actualGuaranteed
*resources.Resource, parentGuaranteed *resources.Resource, queueName string)
error {
+ sum := resources.NewResource()
+ for _, child := range childQueues {
+ childGuaranteed, err :=
resources.NewResourceFromConf(child.Resources.Guaranteed)
+ if err != nil {
+ return err
+ }
+ sum.AddTo(childGuaranteed)
+ }
+ //the sum of guaranteed resources of all child queues of a queue may
not be larger
+ //than the guaranteed resources of that queue
Review comment:
I thought that this would fail the lint test but I was wrong.
Comments however should start with a space unless there is a special reason
not to (i.e. the `//nolint` comment)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
