Re-introduce the check if the number of VFs is 0 #472
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
SchSeba
force-pushed
the
remove_drain_on_none_vfs
branch
from
aa401d8
to
7af9cf7
Compare
Pull Request Test Coverage Report for Build 5541150289
💛 - Coveralls |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
cc @adrianchiris @e0ne @zeeke @Eoghan1232 please take a look :) |
zeeke
reviewed
Jul 12, 2023
zeeke
reviewed
Jul 12, 2023
SchSeba
force-pushed
the
remove_drain_on_none_vfs
branch
from
7af9cf7
to
b15ceab
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks @zeeke can you give another round? |
| // once https://github.com/kubernetes/kubernetes/issues/109595 will be fixed | ||
| if ifaceStatus.NumVfs == 0 { | ||
| glog.V(2).Infof("generic-plugin needDrainNode(): no need drain, for PCI address %s current NumVfs is 0", iface.PciAddress) | ||
| break |
There was a problem hiding this comment.
nit: return here like we do below ? WDYT ?
There was a problem hiding this comment.
no we can't because we need to continue and check all the other status vs desire.
in the NeedUpdate case we return because if we find even one that needs to update we need to drain, but that is not the case if we find one that doesn't need to drain we need to check for all of them
There was a problem hiding this comment.
ah, didnt see that nested loop. (it was not showing in browser) :\
yes you are right !
now that the issue with kubelet with fixed all the way to k8s 1.25 we can add again the check if the number of existing vfs is 0 there is no need to drain the node because there are no workloads running with vfs from that nic Signed-off-by: Sebastian Sch <sebassch@gmail.com>
SchSeba
force-pushed
the
remove_drain_on_none_vfs
branch
from
b15ceab
to
6b946e9
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
adrianchiris
approved these changes
Jul 13, 2023
|
Thanks for the quick review folks! merging |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
now that the issue with kubelet with fixed all the way to k8s 1.25 we can add again the check if the number of existing vfs is 0 there is no need to drain the node because there are no workloads running with vfs from that nic