wilfred-s commented on a change in pull request #359:
URL:
https://github.com/apache/incubator-yunikorn-k8shim/pull/359#discussion_r791278454
##########
File path: pkg/cache/context.go
##########
@@ -365,19 +366,68 @@ func (ctx *Context) bindPodVolumes(pod *v1.Pod) error {
zap.String("podName", pod.Name))
} else {
log.Logger().Info("Binding Pod Volumes",
zap.String("podName", pod.Name))
- boundClaims, claimsToBind, _, err :=
ctx.apiProvider.GetAPIs().VolumeBinder.GetPodVolumes(assumedPod)
+ boundClaims, claimsToBind, unboundClaimsImmediate, err
:= ctx.apiProvider.GetAPIs().VolumeBinder.GetPodVolumes(assumedPod)
Review comment:
This same call is made in `AssumePod` line 457 why do we not need the
change there?
##########
File path: pkg/cache/context.go
##########
@@ -365,19 +366,68 @@ func (ctx *Context) bindPodVolumes(pod *v1.Pod) error {
zap.String("podName", pod.Name))
} else {
log.Logger().Info("Binding Pod Volumes",
zap.String("podName", pod.Name))
- boundClaims, claimsToBind, _, err :=
ctx.apiProvider.GetAPIs().VolumeBinder.GetPodVolumes(assumedPod)
+ boundClaims, claimsToBind, unboundClaimsImmediate, err
:= ctx.apiProvider.GetAPIs().VolumeBinder.GetPodVolumes(assumedPod)
if err != nil {
+ log.Logger().Error("Failed to get pod volumes",
+ zap.String("podName", assumedPod.Name),
+ zap.Error(err))
+ return err
+ }
+ if len(unboundClaimsImmediate) > 0 {
+ err = fmt.Errorf("pod %s has unbound immediate
claims", pod.Name)
+ log.Logger().Error("Pod has unbound immediate
claims",
+ zap.String("podName", assumedPod.Name),
+ zap.Error(err))
return err
}
node, err :=
ctx.schedulerCache.GetNodeInfo(assumedPod.Spec.NodeName)
if err != nil {
+ log.Logger().Error("Failed to get node info",
+ zap.String("podName", assumedPod.Name),
+ zap.String("nodeName",
assumedPod.Spec.NodeName),
+ zap.Error(err))
+ return err
+ }
+ volumes, reasons, err :=
ctx.apiProvider.GetAPIs().VolumeBinder.FindPodVolumes(assumedPod, boundClaims,
claimsToBind, node)
+ if err != nil {
+ log.Logger().Error("Failed to find pod volumes",
+ zap.String("podName", assumedPod.Name),
+ zap.String("nodeName",
assumedPod.Spec.NodeName),
+ zap.Int("claimsToBind",
len(claimsToBind)),
+ zap.Error(err))
return err
}
- volumes, _, err :=
ctx.apiProvider.GetAPIs().VolumeBinder.FindPodVolumes(assumedPod, boundClaims,
claimsToBind, node)
+ if len(reasons) > 0 {
+ sReasons := make([]string, 0)
+ for _, reason := range reasons {
+ sReasons = append(sReasons,
string(reason))
+ }
+ sReason := strings.Join(sReasons, ", ")
+ err = fmt.Errorf("pod %s has conflicting volume
claims: %s", pod.Name, sReason)
+ log.Logger().Error("Pod has conflicting volume
claims",
+ zap.String("podName", assumedPod.Name),
+ zap.String("nodeName",
assumedPod.Spec.NodeName),
+ zap.Int("claimsToBind",
len(claimsToBind)),
+ zap.Error(err))
+ return err
+ }
+ if volumes.StaticBindings == nil {
+ // convert nil to empty array
+ volumes.StaticBindings =
make([]*scheduling.BindingInfo, 0)
+ }
+ if volumes.DynamicProvisions == nil {
+ // convert nil to empty array
+ volumes.DynamicProvisions =
make([]*v1.PersistentVolumeClaim, 0)
+ }
Review comment:
The nil checks for the `volumes.StaticBindings` and
`volumes.DynamicProvisions` have been in the code since k8s 1.12. The real
cause is a flow on effect of the change in the `FindPodVolumes` return value as
part of a cleanup commit. That was added in k8s 1.19 and later: 0 length
slices are replaced with nil values:
https://gitlab.cncf.ci/kubernetes/kubernetes/commit/2cdc63aeaab603c61b48e95c044f1e4bb2f32548
We need tests for this code path.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
