Use VMCache instead of VMICache to judge if the NAD is in use #131
Conversation
|
@mingshuoqiu can you also add steps in Testplan for verifying deletion of vlan config as well with stopped vm. |
mingshuoqiu
force-pushed
the
issue_6961
branch
2 times, most recently
from
bdf0dbd to
bec79e5
Compare
The VMI status will be none if the VM is turned off from the GUI. The VMI information can't not be relied to make sure if particular NAD is attched to any VM or not. Use VMCache instead to prevent from deleting the NAD if the VM is turned off. Signed-off-by: Chris Chiu <[email protected]>
mingshuoqiu
force-pushed
the
issue_6961
branch
from
bec79e5 to
1477278
Compare
| for _, vm := range vms { | ||
| if vm.Namespace != nad.Namespace { | ||
| continue | ||
| } |
There was a problem hiding this comment.
is this check needed ?
| if err := v.checkVmi(vc, nodes); err != nil { | ||
| return fmt.Errorf(deleteErr, vc.Name, err) | ||
| } | ||
|
|
||
| nads, err := v.nadCache.List(util.HarvesterSystemNamespaceName, labels.Set(map[string]string{ | ||
| utils.KeyClusterNetworkLabel: vc.Spec.ClusterNetwork, | ||
| }).AsSelector()) |
There was a problem hiding this comment.
The namespace must be "" or metav1.NamespaceAll as we need to fetch all nads from all namespaces.
HarvesterSystemNamespaceName will fetch nads only from harvester-system namespace
|
Thanks @mingshuoqiu for taking care of the comments. |
|
|
||
| for _, vm := range vmsTmp { | ||
| if vm.Namespace != nad.Namespace { | ||
| continue |
There was a problem hiding this comment.
I am not sure about this existing check and if its needed, we can still attach a nad from any namespace to a vm in any namespace.This operation is supported currently.And with this check we might miss vms that have attached a nad from another namespace and the nad could be deleted while the vm is still present.
There was a problem hiding this comment.
I have a general question regarding the fix: Do we still need the VmiGetter? Under what circumstances do we rely on VMIs to check whether an action is allowed or not? Maybe the logic in the WhoUseNad for VMI and VM is redundant, and we can completely replace VMI with VM. Please advise. Thank you.
|
Need to re-implement the |
The VMI status will be none if the VM is turned off from the GUI. The VMI information can't not be relied to make sure if particular NAD is attched to any VM or not. Use VMCache instead to prevent from deleting the NAD if the VM is turned off.
Problem:
Fail to delete an Off VM if its network was deleted. The VM network can be deleted if the attached VM is off. It will be a problem if user try to turn the VM back to on.
Solution:
Do not allow VM network removing if the VM still exists, even it is OFF.
Related Issue:
harvester/harvester#6961
Test plan: