Variance's value may be zero, there may crash, you'd better verify it.

The only way for it to happen is requested memory, cpu etc are 0's or < 0, in either case we won't be reaching any of the priority functions but I understand your point. I will add a check.

nit: I would rename to BalanceAttachedNodeVolumes and would update the comment to say that this impacts scheduling decisions and scheduler tries to place a Pod that requires an attached volume on a Node with fewer volumes.

Sure.

Please remove the blank line.

Can't we have shared volumes?

cc/ @msau42

Do you care about counting bind mounts? There is always a bind-mount (done by the container runtime) for all the volumes referenced in the container spec.

At the pod level, you can share between pods, but the number of mounts depends on the volume type. Sharing between pods on the same node is not as common. Typically, scheduler tries to spread pods across nodes.

• In the case of NFS, each pod has its own NFS mount.
• For something like GCE PD (sharing should be a rare use case), you will have only one global node mount, but a bind mount per pod.
• Volume types like EmptyDir, ConfigMap, Secrets, downward api are not mounted
• Hostpath volumes can be mounted or not, we can't tell from the spec.

This is a function used in testing, thats why I did not bother checking that, if you think its important, I can add a check.

Thanks Ravi for pointing out. I missed the fact that it is a test only function. We don't need to change it.

NodeInfo is a part of scheduler cache, but we update the information in this predicate. This is not a good idea. If the prediate is not executed, the information in the cache gets stale. This could lead to subtle bugs. Why don't we update the values in NodeInfo.AddPod() and .RemovePod()?

Actually, we are not caching anything now. For example, say we chose node n1 during pod p1's scheduling. When we are scheduling next pod p2, we are going to recompute the volumes attached, max number of volumes again. As I mentioned in the comment, for lack of proper data structure through we can share information across predicates and priorities, I am using it. I don't say that this clear or better. I am open to suggestions.

Actually, we are not caching anything now.

I understand you don't need to cache anything and this is just a way of passing information around, but the nodeInfo pointer is a part of scheduler's cache. When you update the fields of the struct, you are effectively keeping the information in the scheduler's cache. The existance of such information in the scheduler cache would be confusing for someone who reads the code later.
Ideally, we should keep this in some temporary state that is valid in a single schedule cycle and is destructed at the end of the cycle, but we don't have such a structure and we need to add it if needed.

Ok let me think of some alternate ways to do this and get back to you. Thanks again for the review.

Agree with Bobby, prefer not to update NodeInfo outside of cache.

variance could be larger than 1, causing 1 - variance (below) to be negative. I guess you have forgotten to divide by 3.

I guess you have forgotten to divide by 3.

🤣 Lol, thanks for catching it :)

variance could be larger than 1, causing 1 - variance (below) to be negative.

I thought about that, but it seems our sample set always has fractions and

• fraction-mean(which is a fraction) would be fraction again.
• square-root of (fraction-mean) will be further lower than original fraction

Not sure if I missed any edge case.

You are right, but given that you have not divided by 3, it could be larger than 1. For example, if each component is 0.9, then variance will be 0.81 + 0.81 + 0.81. If you divide by 3, variance will be less than 1.

So, you were commenting on my version of variance(without division by 3) I thought you were commenting on variance in general that it could be greater than 1(which is true as well). Sure, I will make that change.

Yes, I was referring to this implementation.

Here's the explanation of variance, not sure why we use it?

In probability theory and statistics, variance is the expectation of the squared deviation of a random variable from its mean.

So, we wanted to know how close CPU, memory utilization, no of volumes on nodes are to each other. The closer means the node is well balanced. I tried explaining the same here-#58232 (comment)

I could have used standard deviation as well but I thought of stopping at variance.

Great, got that.

Maybe standard deviation is better; it'll also address Bobby's comments above :)

Just deviding by 3 addresses my comment and since we want to compare one node against others, standard deviation and variance both work equally well here. Given that taking square root is computationally expensive, I would just use variance.

Yes, computational cost was the reason that I stopped going towards standard deviation.

ok to me :)

This volume predicate only counts for 3 volume types, and only one of them can run in a single cluster.

• GCE PD
• AWS EBS
• Azure disk

Did you also want to count mounts for other volume types not included here, like NFS, Gluster, Ceph?

Also, the limits here are more about how many disks can be attached to a single node, not how many could be mounted. So if you had multiple pods sharing the same volume on the same node, this predicate would only count 1 of them, even though there will be a mount for each pod. But multiple pods sharing the same volume on a single node is not the common use case.

In the future, it may be possible that you can have multiple volume types in the same cluster that each have their own max limit as well.

Thanks for the information @msau42. I thought we are going in the direction of using a configmap which consists of max number of volumes per node based on cloud provider and CSI plugin, after which we can use configmap to get the information.

I think the main challenge is that this predicate is only meant to restrict max number of attached volumes, not overall number of mounts. If you want to spread number of attached volumes, then this predicate will work.

But it sounds like you want to spread out number of mounts, including for non-attachable plugins.

If that's the case, then you may need to consider a different method of counting mounts, such as just counting the total number of volumes in assigned pods. But that still won't be completely accurate because each volume type may create a different number of mounts, like I mentioned earlier.

But it sounds like you want to spread out number of mounts, including for non-attachable plugins.

So, we wanted to find nodes that have balanced number of volumes here. Not sure where this PR gave the impression of spreading mounts. While finding node that has balanced mounts will be even more useful, I think it has to be dependent on lower level API talking to cloud provider or listing /etc/mtab in case of linux etc and is beyond the scope of what we want to do here.

Maybe improving the wording in the comments here to explicitly say we're balancing attachable volume cpunts would make it more clear.

Apologies if the comments are causing confusion. I thought I made it clear atleast from the start of PR, if you think otherwise, I can edit the existing comments. Please let me know, where the confusion is. Even if it is in code, I am happy to modify it.

Maybe we can be more explicit in some of the variable names. Like, instead of AllocableVolumes, something like AttachableVolumesLimit.

I don't think, AttachableVolumesLimit actually reflects the allocatable volumes on the node. My intention there was to have a variable which holds the value for number of allocatable volumes currently on the node. AttachableVolumesLimit may be more suited for c.maxVolumes. I don't have strong opinion on it though.

Oh sorry, yes, what about something like "CurrentAttachedVolumesCount"? I want to be explicit that we are talking about attachable counts here. There is another completely unrelated volume feature that uses the term "allocatable", so it would be better to align with the terminology that we use in the documentation and API fields.

Sure. I can do that.

is it possible to also merge volumes into Resource? so no interface changed.

I thought about it but it comes mostly under scalar resource and I think there is no definitive way on kubelet side to introspect node and get that information but it would be ideal to have that kind of information there. @bsalamat WDYT?

is it possible to also merge volumes into Resource? so no interface changed.

That would require updating the scheduler cache with the volume information and that in turn would require Kubelets to report volume counts, etc. as a part of allocatable resources. This would be a larger project requiring involvement of the node team. I won't go this path, if we can solve the problem without changes to the kubelet.

@gnufied is working on a design for dynamic node reporting of attachable volume limits per plugin. So I think when that is implemented, the limits will actually will be in the Node object.

I think when that is implemented, the limits will actually will be in the Node object.

That will be great and will make our logic cleaner.

So I think when that is implemented, the limits will actually will be in the Node object.

Thanks @msau42 . That will be awesome. Ohh btw, then we could actually cache in nodeinfo :).

That may related to our previous comments: did not update nodeInfo outside of cache :)

I think when that is implemented, the limits will actually will be in the Node object.

That will be great and will make our logic cleaner.

Great, @gnufied any related doc/issue?

@k82cn @bsalamat kubernetes/community#2051

You do not need the "if" part.

In fact, given that TransientInfo is initialized with nodeInfo, please remove the whole else part and not initialize it here.

Done Bobby. PTAL.

One last thing. This method is not needed. I would remove and inline the body in resetTransientSchedulerInfo to prevent future mistakes of using it directly (without locking).

Updated. Thanks. I did not want to have a big function which resets all the transient values, that was the reason, I separated that block into different function. But I understand your point of calling this function directly without acquiring lock.