Skip to content
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

fix: The gc delete multus ip cr and lsp setting when enable keep vm ip #3378

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

watermelon-brother
Copy link
Contributor

What type of this PR

The vm multus ip cr and lsp setting will be deleted by gc logic when vm define multus networkName as below:

spec:
  template:
    spec:
      networks:
      - multus:
          networkName: <nad-name> # not <nad-namespace>/<nad-name>
        name: <nic-name>

Which issue(s) this PR fixes

The vm ip and mac somethime change when enable keep VM IP.

WHAT

I have remove not need logic in the pr.

HOW

Keep vm ip and mac not change is very important, because it can cause the vm nic not get ip when mac address change.

@watermelon-brother watermelon-brother changed the title fix: gc delete multus ip cr and lsp setting when enable keep VM IP fix: The gc delete multus ip cr and lsp setting when enable keep vm ip Nov 6, 2023
@bobz965
Copy link
Collaborator

bobz965 commented Nov 7, 2023

我存在一个疑问,感觉bug原因还需要澄清下:

情况1: networkName: nad-name
之前:vmLsps 从 attachNets 缓存到了 vm 的所有 lsp (这里应该是能缓存到吧?)
现在:vmLsps 从 vm.Spec.Template.Spec.Networks 缓存到了 vm 的所有 lsp

结果一致

情况2: networkName: nad-namespace/nad-name
之前:vmLsps 从 attachNets 缓存到了 vm 的所有 lsp
现在:vmLsps 从 vm.Spec.Template.Spec.Networks 缓存到了 vm 的所有 lsp

结果一致

@watermelon-brother
Copy link
Contributor Author

我存在一个疑问,感觉bug原因还需要澄清下:

情况1: networkName: nad-name 之前:vmLsps 从 attachNets 缓存到了 vm 的所有 lsp (这里应该是能缓存到吧?) 现在:vmLsps 从 vm.Spec.Template.Spec.Networks 缓存到了 vm 的所有 lsp

结果一致

情况2: networkName: nad-namespace/nad-name 之前:vmLsps 从 attachNets 缓存到了 vm 的所有 lsp 现在:vmLsps 从 vm.Spec.Template.Spec.Networks 缓存到了 vm 的所有 lsp

结果一致

vm is not define below:

vm.Spec.Template.ObjectMeta.Annotations[util.AttachmentNetworkAnnotation]

The virt-launcher-pod have multus annotations is auto add by virt-controller-pod.

@bobz965
Copy link
Collaborator

bobz965 commented Nov 7, 2023

@watermelon-brother, if vm.Spec.Template.ObjectMeta.Annotation is not used, How about the logic in pod.go which used vm.Spec.Template.ObjectMeta.Annotation should be dealt with too?

image

@watermelon-brother
Copy link
Contributor Author

@watermelon-brother, if vm.Spec.Template.ObjectMeta.Annotation is not used, How about the logic in pod.go which used vm.Spec.Template.ObjectMeta.Annotation should be dealt with too?

image

I mean that the vm.Spec.Template.ObjectMeta.Annotation key util.AttachmentNetworkAnnotation is not need.

@bobz965
Copy link
Collaborator

bobz965 commented Nov 7, 2023

Spec.Template.ObjectMeta.Annotations[

Oh, I get it. thanks

@hongzhen-ma
Copy link
Collaborator

hongzhen-ma commented Nov 7, 2023

vm.Spec.Template.Spec.Networks

跟兵兵沟通后,确认情况如下
1、使用 vm.Spec.Template.Spec.Networks 参数指定 multus网络,在包含 ns 参数的时候,不会出现 lsp 丢失的情况。没有 ns 参数的时候,有 issue 描述的问题,所以需要支持 不带ns 的情况。
2、multus 支持使用 annotation 指定 multus 网卡的情况,这种使用方式是符合正常需求的。具体可以参考
https://github.com/k8snetworkplumbingwg/multus-cni/blob/master/docs/quickstart.md

所以基于annotation 检查 multus 配置的逻辑,应该保留

@@ -756,22 +756,11 @@ func (c *Controller) getVMLsps() []string {
vmLsp := ovs.PodNameToPortName(vm.Name, ns.Name, util.OvnProvider)
vmLsps = append(vmLsps, vmLsp)

attachNets, err := util.ParsePodNetworkAnnotation(vm.Spec.Template.ObjectMeta.Annotations[util.AttachmentNetworkAnnotation], vm.Namespace)
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep this section for util.AttachmentNetworkAnnotation case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is not need for kubevirt. If you add this annotation, it is work only you not set vm multus network.

@watermelon-brother
Copy link
Contributor Author

watermelon-brother commented Nov 7, 2023

vm.Spec.Template.Spec.Networks

跟兵兵沟通后,确认情况如下 1、使用 vm.Spec.Template.Spec.Networks 参数指定 multus网络,在包含 ns 参数的时候,不会出现 lsp 丢失的情况。没有 ns 参数的时候,有 issue 描述的问题,所以需要支持 不带ns 的情况。 2、multus 支持使用 annotation 指定 multus 网卡的情况,这种使用方式是符合正常需求的。具体可以参考 https://github.com/k8snetworkplumbingwg/multus-cni/blob/master/docs/quickstart.md

所以基于annotation 检查 multus 配置的逻辑,应该保留

This logic is not need, because if you set vm.Spec.Template.ObjectMeta.Annotation key util.AttachmentNetworkAnnotation, It can override by the kubevirt controller logic. this logic you can see below:

https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-controller/services/template.go -> generatePodAnnotations

@bobz965
Copy link
Collaborator

bobz965 commented Nov 7, 2023

hi @watermelon-brother ,那段之前就有的逻辑应该不会产生 bug, 而且大家使用的kubevirt的版本可能不太一样,而且查看所有关于 AttachmentNetworkAnnotation 历史代码逻辑变更的操作也是一个比较繁琐的操作。我当前不确定 kubevirt 是否一直以来都是只用你说的这一种设计。毕竟这段代码应该是为了兼容另外一种逻辑。

Signed-off-by: wujixin <wujix@yealink.com>
@watermelon-brother
Copy link
Contributor Author

hi @watermelon-brother ,那段之前就有的逻辑应该不会产生 bug, 而且大家使用的kubevirt的版本可能不太一样,而且查看所有关于 AttachmentNetworkAnnotation 历史代码逻辑变更的操作也是一个比较繁琐的操作。我当前不确定 kubevirt 是否一直以来都是只用你说的这一种设计。毕竟这段代码应该是为了兼容另外一种逻辑。

OK, I had reserve it in the pr.

@bobz965 bobz965 merged commit 08d8321 into kubeovn:master Nov 8, 2023
10 checks passed
@bobz965 bobz965 added bug Something isn't working need backport labels Nov 8, 2023
bobz965 pushed a commit that referenced this pull request Nov 8, 2023
…3378)

Signed-off-by: wujixin <wujix@yealink.com>
Co-authored-by: wujixin <wujix@yealink.com>
bobz965 pushed a commit that referenced this pull request Nov 8, 2023
…3378)

Signed-off-by: wujixin <wujix@yealink.com>
Co-authored-by: wujixin <wujix@yealink.com>
bobz965 pushed a commit to bobz965/kube-ovn that referenced this pull request Nov 15, 2023
…ubeovn#3378)

Signed-off-by: wujixin <wujix@yealink.com>
Co-authored-by: wujixin <wujix@yealink.com>
Signed-off-by: bobz965 <zhangbingbing2_yewu@cmss.chinamobile.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working need backport
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants