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 handedeletePod repeat 4 times #2789

Merged
merged 2 commits into from
May 11, 2023
Merged

Conversation

changluyi
Copy link
Collaborator

@changluyi changluyi commented May 11, 2023

What type of this PR

Examples of user facing changes:

  • Features
  • Bug fixes
  • Docs
  • Tests

Which issue(s) this PR fixes:

Fixes #2555

做了两个调整:

  1. 重新调整进入 deletePodQueue 队列的对象是key,而不是pod指针,能够用到workqueue的去重功能,减少进入handle函数,
    维护deletingPodObjMap map来作为删除pod的对象映射。在handleDeletePod 后触发一个5min 协程来释放deletingPodObjMap 对应的pod

但是workqueue的去重功能触发时间非常短,因为进入队列后,一旦deletePodQueue.Get()被调用到, workqueue就会把对象从dirty(表示进入队列还未处理)转到processing,再次进来的对象就算是一样,但workqueue也不会去重。

加了这个改动后,基本上能减少一次handle处理。

  1. if !isPodAlive(newPod) && !isStateful && !isVmPod {处 触发enqueue deletePodQueue 和

下面的 newPod.DeletionTimestamp != nil 触发 enqueue deletePodQueue 感觉逻辑上重叠,应该只需要一次触发就够了。

WHAT

🤖 Generated by Copilot at 2b6387a

This pull request enhances the pod controller to track and handle pod deletion more reliably. It adds a new feature to the controller to store pod objects that are being deleted in a map and uses the pod status phase to check for deletion. It also updates the pod controller to use the pod objects from the map instead of the cache. It modifies the files pkg/controller/controller.go and pkg/controller/pod.go.

🤖 Generated by Copilot at 2b6387a

The pod controller had a new feature
To track pods that were no longer creature
It used pod status phase
And a map in a new way
To handle pod deletion more mature

HOW

🤖 Generated by Copilot at 2b6387a

  • Import the k8s.io/api/core/v1 package as k8sv1 to use the Pod type (link)
  • Add a new field deletingPodObjMap to the Controller type to store the pod objects that are being deleted (link)
  • Initialize the deletingPodObjMap field with an empty map in the Run method of the Controller (link)
  • Split the isPodAlive function into two functions: isPodAlive and isPodStatusPhaseAlive to check the pod deletion based on different criteria (link)
  • Use the isPodStatusPhaseAlive function instead of the isPodAlive function to check the pod deletion in the enqueueUpdatePod function (link)
  • Store the pod object in the deletingPodObjMap and enqueue the pod key instead of the pod object in the deletePodQueue in the following functions: enqueueAddPod (link), enqueueDeletePod (link), enqueueUpdatePod (link, link, link), reconcileAllocateSubnets (link), and reconcileRouteSubnets (link) (link)
  • Get the pod key and the pod object from the deletingPodObjMap instead of the deletePodQueue in the processNextDeletePodWorkItem function (link)
  • Change the argument of the handleDeletePod function from the pod object to the pod key and get the pod object from the deletingPodObjMap (link)

@github-actions
Copy link
Contributor

  • The code changes add a new map called deletingPodObjMap to the Controller struct. This map is used to store pods that are being deleted, and it is populated in several places throughout the code. However, there is no clear explanation of why this map is needed or how it is used. It would be helpful to add comments explaining the purpose of this map and how it fits into the overall design of the controller.
  • In the enqueueAddPod function, the c.deletingPodObjMap[key] = p line is redundant since p is already passed as an argument to the function and assigned to the pod variable. This line can be removed to simplify the code.
  • In the handleDeletePod function, the podName variable is assigned the result of c.getNameByPod(pod), but pod is not used anywhere else in the function. Instead, the pod variable could be removed from the function signature and replaced with podName.
  • The isPodStatusPhaseAlive function is defined after it is used in the isPodAlive function. It would be clearer to define isPodStatusPhaseAlive before isPodAlive to avoid confusion.
  • There are several instances where the key variable is constructed using fmt.Sprintf("%s/%s", namespace, name). It would be more efficient to use path.Join(namespace, name) instead.

@github-actions
Copy link
Contributor

  • The code changes in controller.go and pod.go should be separated into different commits to make it easier to review and understand the changes.
  • In controller.go, the newly added deletingPodObjMap map is not being used consistently. It is being used to store the pod object when adding a pod to the delete queue, but then it is also being used to retrieve the pod object when processing the delete queue. This can lead to inconsistencies if the pod object is deleted from the map before it is processed. Instead, it would be better to use the key of the pod (in the format "namespace/name") as the item in the delete queue, and then retrieve the pod object using this key when processing the queue.
  • In pod.go, the function isPodStatusPhaseAlive should be renamed to something more descriptive, such as isPodReadyForDeletion. This will make it clearer what the function does and how it is being used.
  • In pod.go, the function handleDeletePod should take the key of the pod (in the format "namespace/name") instead of the pod object itself. This will allow the function to retrieve the pod object from the deletingPodObjMap consistently with other functions that use this map.
  • In pod.go, the gc pod obj in c.deletingPodObjMap code block in processNextDeletePodWorkItem should be removed. This is because the map is only used to retrieve the pod object when processing the delete queue, and there is no need to keep the object in memory after it has been processed.

@changluyi changluyi force-pushed the fix_handleDeletePod_repeat branch from 1094f74 to 7afc452 Compare May 11, 2023 08:48
@github-actions
Copy link
Contributor

  • The code changes add a new map called deletingPodObjMap to the Controller struct. This map is used to store pods that are being deleted, but have not yet been processed by the handleDeletePod function. However, it is not clear why this map is necessary and how it improves the performance or functionality of the controller. It would be helpful to add comments explaining the purpose of this map and how it is used in the code.
  • In the enqueueAddPod function, the deletingPodObjMap map is updated with the pod object before adding the pod key to the deletePodQueue. However, if an error occurs while adding the pod to the queue, the pod object is not removed from the deletingPodObjMap, which could lead to memory leaks. It would be better to remove the pod object from the map if an error occurs.
  • In the enqueueUpdatePod function, the deletingPodObjMap map is updated with the new pod object before adding the pod key to the deletePodQueue. However, if the pod is already in the deletingPodObjMap, the old pod object is not removed from the map, which could lead to memory leaks. It would be better to remove the old pod object from the map if it exists.
  • In the processNextDeletePodWorkItem function, the deletingPodObjMap map is used to retrieve the pod object associated with the pod key. However, if the pod has already been deleted, the pod object will not exist in the map, which could cause a panic. It would be better to check if the pod object exists in the map before using it.
  • In the handleDeletePod function, the podName variable is assigned the result of the getNameByPod function, which is not defined in the code provided. This could cause a compilation error. It would be better to define this function or use a different method to retrieve the pod name.

@changluyi changluyi merged commit 8f43028 into master May 11, 2023
@changluyi changluyi deleted the fix_handleDeletePod_repeat branch May 11, 2023 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle delete pod log appear after create a pod.
2 participants