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]: delete the NIC regardless of whether the Pod was found or not. #4500

Merged
merged 6 commits into from
Sep 19, 2024

Conversation

liyh-yusur
Copy link
Contributor

Pull Request

What type of this PR

Examples of user facing changes:

  • Bug fixes

Describe

当删除pod时,若pod被直接删除cni来不及处理时,通过pod, err := csh.Controller.podsLister.Pods(podRequest.PodNamespace).Get(podRequest.PodName)获取pod进行后续的处理条件,会导致此时无法处理,直接在k8serrors.IsNotFound中返回。

复现

例如 在pod的部署yaml中设置了terminationGracePeriodSeconds: 0,导致 Pod 在删除 Deployment 后被立即终止,而 CNI 插件的 handleDel 函数还没来得及处理删除网卡的操作。
使用 podRequest.ContainerID 来识别 Pod,而不是使用 podRequest.PodName。这样即使 Pod 已经被删除,CNI 插件仍然可以通过容器 ID 来删除网卡。

    spec:
      containers:
      - name: iperf3-server
        image: harbor.yusur.tech/yusur_ovn/iperf3
        args: ['-s','-p','5001']
        resources:
            requests:
              yusur.tech/sriov_dpu: '1'
            limits:
              yusur.tech/sriov_dpu: '1'
        ports:
        - containerPort: 5001
          name: server
      terminationGracePeriodSeconds: 0
      nodeSelector:
        app: node1

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working go Pull requests that update Go code labels Sep 10, 2024
vmName := pod.Annotations[fmt.Sprintf(util.VMAnnotationTemplate, podRequest.Provider)]
if vmName != "" {
podRequest.PodName = vmName
nicType = "veth-pair"
Copy link
Collaborator

Choose a reason for hiding this comment

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

感觉像上面那样用个常量,用 nicType = util.VethType 会更好些

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已经改成了常量

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 16, 2024
@@ -229,7 +229,7 @@ func (csh cniServerHandler) removeDefaultRoute(netns string, ipv4, ipv6 bool) er
return nil
}

func (csh cniServerHandler) deleteNic(podName, podNamespace, containerID, netns, deviceID, ifName, nicType, _ string) error {
func (csh cniServerHandler) deleteNic(podName, podNamespace, containerID, netns, deviceID, ifName, nicType string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个参数在 windows 系统没有用到不需要修改

Copy link
Contributor Author

Choose a reason for hiding this comment

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

此处已恢复到更改前代码

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"not enough arguments in call to csh.deleteNic", 前面的函数调用需要,这里得去掉最后一个参数了

@dosubot dosubot bot removed the lgtm This PR has been approved by a maintainer label Sep 17, 2024
@oilbeater oilbeater merged commit 412f799 into kubeovn:master Sep 19, 2024
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working go Pull requests that update Go code size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants