-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
2995 check if storage network attached before cluster network and vlan config config deletion #115
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question, thanks.
@@ -53,6 +57,17 @@ func (c *CnValidator) Delete(_ *admission.Request, oldObj runtime.Object) error | |||
return fmt.Errorf(deleteErr, cn.Name, fmt.Errorf("it's not allowed")) | |||
} | |||
|
|||
nads, err := c.nadCache.List(StorageNetworkNetAttachDefNamespace, labels.Set(map[string]string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be redundant.
The following vlanconfig
checks if any vc on this cluster is still existing, then block it. The storagenetwork
also relys on one vlanconfig
. Checking vlanconfig
seems to be enough here.
On the other hand, below function on Harvester needs to check VlanConfig
is existing when utilize this vlanconfig
to carry the storagenetwork.
func (v *settingValidator) validateStorageNetworkHelper(value string) error {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@w13915984028 yes makes sense, if storage network is validated for vlan config to be present then validating at cluster network will be redundant.I will remove that check.Thanks.
Regarding storage network validating if vlan config is present, I have another pull request (testing still in progress)
where I am checking if cluster network is ready (cluster network becomes ready only if at least one vlan config attached) before configuring storage networks.
harvester/harvester#6538
fb3f0df
to
255b7d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
255b7d8
to
135ce9f
Compare
135ce9f
to
487f555
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the PR.
@Mergifyio backport v0.5.x |
✅ Backports have been created
|
Problem:
1.User can delete the cluster network when storage network NAD is still attached to it.
2.User can delete the vlan config when storage network NAD is still attached to it.
The above breaks the networking functionality of storage networks without user being notified.
Solution:
Do not delete the cluster network or vlan config if storage network still exists for the cluster.
Related Issue:
Test plan:
Manually validated in a harvester node.
Reference bug report:
harvester/tests#1311
Negative: Delete cluster network when already set on storage network
Negative: Delete vlan config when already set on storage network