-
Notifications
You must be signed in to change notification settings - Fork 12
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
Controller: handle add/remove in the same reconciliation #158
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,6 +248,20 @@ func (pnc *PodNetworksController) processNextWorkItem() bool { | |
} | ||
} | ||
|
||
// handleDynamicInterfaceRequest modifies the pod annotation with the latest status | ||
// so the updated status annotation can be obtained from there. | ||
newIfaceStatus, err := annotations.PodDynamicNetworkStatus(pod) | ||
if err != nil { | ||
klog.Errorf("failed while getting network status annotation: %v", err) | ||
return true | ||
} | ||
|
||
// apply the network status using the Kubernetes API. | ||
if err := nadutils.SetNetworkStatus(pnc.k8sClientSet, pod, newIfaceStatus); err != nil { | ||
klog.Errorf("failed while setting network status annotation: %v", err) | ||
return true | ||
} | ||
Comment on lines
+259
to
+263
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The commit message says:
But I don't think that's true; IIUC, you're setting it 3 times - once in Right ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add and remove is only updating the pod object, it doesn't update the pod in Kubernetes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, thanks for explaining. |
||
|
||
return true | ||
} | ||
|
||
|
@@ -346,8 +360,9 @@ func (pnc *PodNetworksController) addNetworks(dynamicAttachmentRequest *DynamicA | |
return fmt.Errorf("failed to compute the updated network status: %v", err) | ||
} | ||
|
||
if err := nadutils.SetNetworkStatus(pnc.k8sClientSet, pod, newIfaceStatus); err != nil { | ||
return err | ||
err = annotations.SetNetworkStatus(pod, newIfaceStatus) | ||
if err != nil { | ||
Comment on lines
+363
to
+364
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the purpose of this change ? Please keep the old format. It's OK to update the error msg. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This updates the annotation in the pod object passed in parameter. The update of the pod via the Kubernetes API is no longer done here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, I had missed you had changed the package !!! My bad. I do think the function's name is misleading though. And this is not as explicit as I would like it to be. |
||
return fmt.Errorf("failed to set the network status annotation: %v", err) | ||
} | ||
|
||
return nil | ||
|
@@ -404,8 +419,10 @@ func (pnc *PodNetworksController) removeNetworks(dynamicAttachmentRequest *Dynam | |
err, | ||
) | ||
} | ||
if err := nadutils.SetNetworkStatus(pnc.k8sClientSet, pod, newIfaceStatus); err != nil { | ||
return err | ||
|
||
err = annotations.SetNetworkStatus(pod, newIfaceStatus) | ||
if err != nil { | ||
return fmt.Errorf("failed to set the network status annotation: %v", err) | ||
Comment on lines
+422
to
+425
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
} | ||
|
||
return nil | ||
|
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.
hm, maybe I'm missing something, but why are you implementing this function instead of re-using the one provided in the network attachment definition client ?
We're already importing those utils.
If there's a reason for that, please also refer it in the commit message.
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 function in the nad-client will keep the pod object passed in parameter intact. It fetches the pod via the name from the Kubernetes API, updates its annotation and updates it via the Kubenretes API.
https://github.com/k8snetworkplumbingwg/network-attachment-definition-client/blob/v1.4.0/pkg/utils/net-attach-def.go#L87
The function I created is different. It updates the annotation of the pod object passed as parameter without updating it in Kubernetes.
So, when the add happens, the pod object is modified with the new annotations (not yet updated in Kubernetes). The del will then happen with the pod object modified with the added annotation (here is the difference with the previous behavior), and then the del will modify the annotations and then the Kubernetes API is called to update the pod itself.
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.
I see, thanks for the explanation.