only add resources from cond ready and status true #410
Conversation
Sure, thanks, done |
| Nodes: make(map[string]*api.NodeInfo), | ||
| } | ||
|
|
||
| cache.addNode(node1) |
There was a problem hiding this comment.
If you add an "unllocatable node" to this test, it will be "LGTM" :)
There was a problem hiding this comment.
Node with status v1.NodeDiskPressure is unallocatable according to MCAD, do we need another test and if yes what do we think should be the node condition?
There was a problem hiding this comment.
Yes, to both questions. I would try to get close to 100% coverage for this test case.
There was a problem hiding this comment.
v1. NodeDiskPressure is already part of this PR
|
@tardieu please let us know if you have any feedback, thanks |
|
@z103cb do we know what is pending for this PR to be merged? |
There was a problem hiding this comment.
See my comments to your questions on what tests. These is holding up the approval of the PR.
It would be great if you are to add a test covering concurrent reads and writes in the cache. Something along the line: one thread is updating the cache while the other is trying to read it, mimicking MCAD usage.
| Nodes: make(map[string]*api.NodeInfo), | ||
| } | ||
|
|
||
| cache.addNode(node1) |
There was a problem hiding this comment.
Yes, to both questions. I would try to get close to 100% coverage for this test case.
Thanks, May be I am missing something, I don't think the cache is updated concurrently, scheduleNext() thread reads it by making a copy of it. This PR is about adding nodes that are in condition Ready and status True to the cache. Increasing Testing coverage I think is orthogonal. do you think a few tests are still missing wrt to the functionality provided in this PR? |
Thanks, May be I am missing something, I don't think the cache is updated concurrently, scheduleNext() thread reads it by making a copy of it. This PR is about adding nodes that are in condition Ready and status True to the cache. Increasing Testing coverage I think is orthogonal. do you think a few tests are still missing wrt to the functionality provided in this PR? Adding the concurency tests are probably outside the scope of this PR (but never the less they would be nice). In regards to functionallity changed by this PR, adding a test case that "loops" over a set of nodes with or without conditions is probably a must. |
sure, I will implement the looping test, thank you! |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
No description provided.