)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":23567,"name":"Luis Tomas Bolivar","email":"ltomasbo@redhat.com","username":"ltomasbo"},"change_message_id":"60664aeed77fe98a9d8ac28a290e0dc3baf8834a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"97ec0747_0e619f74","updated":"2022-01-10 07:35:55.000000000","message":"If the nodeName can be changed, how many times can that happen? What happens if the change happens before the new code gets executed? or after it gets executed? Could we end up creating the port in a wrong node?\n\n-1 just for starting the discussion","commit_id":"9793b369abcecd72fe13581a670ccfcb85782a0c"},{"author":{"_account_id":11600,"name":"Michał Dulko","email":"michal.dulko@gmail.com","username":"dulek"},"change_message_id":"5a4d928b266daf0f619f3c1c00174a3401f0f60d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"3adb412e_c9e3929f","updated":"2022-01-19 12:47:27.000000000","message":"Okay, we have some insight, that this might be a problem with apiserver constraints or some stale data reply from HA apiservers. In any case we need a fix to prevent such a failure and a guarantee that we\u0027re able to recover.\n\nI would think that once we can recover from this once we periodically fetch a full list of resources in the watcher (we called it \"reconciliation\" back when it was implemented), but I\u0027d like a second opinion on that.\n\nI\u0027d only want a warning logged when we hit this and I\u0027m happy with the patch. Something like \"Pod \u003cns\u003e/\u003cname\u003e has no .spec.nodeName set. This is unexpected as it\u0027s supposed to be scheduled. Ignoring\".","commit_id":"9793b369abcecd72fe13581a670ccfcb85782a0c"},{"author":{"_account_id":11600,"name":"Michał Dulko","email":"michal.dulko@gmail.com","username":"dulek"},"change_message_id":"3009bafba9f3c3910065d2e1e41efae5fa5e78c5","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"bdd73a37_f474b786","in_reply_to":"0b228742_b6f4c428","updated":"2022-01-19 15:34:14.000000000","message":"There\u0027s no guarantee how a thread will get scheduled on the CPU between we fetch the resource list and we actually add a resource state to the queue. Imagine this:\n\n1. Queue for resource foo has 2 events A and B, A is waiting in ResourceNotReady, so it\u0027s stuck for a longer while.\n2. Reconciliation thread gets a list of resources and starts injecting them, but not yet gets to the resource foo.\n3. Resource foo changes and watcher gets event C and adds it to the queue.\n4. Our reconciliation loop gets to the resource foo and injects outdated state that reflects event B, not C.\n5. Thanks to [1] events B and C are ignored and we only take the injected event which basically makes us ignore changes in C.\n\nWe could work this around by making sure the queues are priority queues based on resourceVersion. The problem is that K8s docs state that resourceVersion is purely an implementation detail and you shouldn\u0027t rely on it\u0027s content. With etcd it\u0027s actually always going up, so that would work, but again, they deliberately warn against it.\n\n[1] https://github.com/openshift/kuryr-kubernetes/blob/master/kuryr_kubernetes/handlers/asynchronous.py#L100-L104","commit_id":"9793b369abcecd72fe13581a670ccfcb85782a0c"},{"author":{"_account_id":27032,"name":"Maysa de Macedo Souza","email":"maysa.macedo95@gmail.com","username":"maysa"},"change_message_id":"380c5c2a0b91b098d08a918d1e9988ab9430162f","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"0b228742_b6f4c428","in_reply_to":"3adb412e_c9e3929f","updated":"2022-01-19 14:55:06.000000000","message":"Yes, from my understanding the event would be ignored[1] and then retried by reconciliation.\nOn the reconciliation topic, Even though it\u0027s mentioned in the comment here[2] I can\u0027t fully get why we avoid adding the event to the queue as the injected would be latest version, I think.\n[1] https://github.com/openshift/kuryr-kubernetes/blob/master/kuryr_kubernetes/controller/handlers/kuryrport.py#L268-L272\n[2] https://github.com/openshift/kuryr-kubernetes/blob/master/kuryr_kubernetes/handlers/asynchronous.py#L60-L61","commit_id":"9793b369abcecd72fe13581a670ccfcb85782a0c"},{"author":{"_account_id":27032,"name":"Maysa de Macedo Souza","email":"maysa.macedo95@gmail.com","username":"maysa"},"change_message_id":"98d38ffd0947f00bcd8c9c407a2e7460aaaadf94","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"8e3960d5_6eeeb2ec","in_reply_to":"4ca66261_c93ae69b","updated":"2022-01-14 00:14:10.000000000","message":"and we fetch the Pod again before requesting the VIF so it\u0027s supposed to be the current version of the resource.\n\nhttps://github.com/openstack/kuryr-kubernetes/blob/master/kuryr_kubernetes/controller/handlers/kuryrport.py#L226-L229","commit_id":"9793b369abcecd72fe13581a670ccfcb85782a0c"},{"author":{"_account_id":11600,"name":"Michał Dulko","email":"michal.dulko@gmail.com","username":"dulek"},"change_message_id":"502212980b04f5ea9fb971b75b0c4035400de5a8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"c068a1c1_d55b0173","in_reply_to":"97ec0747_0e619f74","updated":"2022-01-12 16:38:43.000000000","message":"According to [1] pods cannot be rescheduled. This has to be a bit different case here.\n\n[1] https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#pod-lifetime","commit_id":"9793b369abcecd72fe13581a670ccfcb85782a0c"},{"author":{"_account_id":27032,"name":"Maysa de Macedo Souza","email":"maysa.macedo95@gmail.com","username":"maysa"},"change_message_id":"01847c325423f8dd2c5fc362ea8250adbf2e459c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"4ca66261_c93ae69b","in_reply_to":"c068a1c1_d55b0173","updated":"2022-01-14 00:00:01.000000000","message":"From the Kuryr-controller logs I see the KuryrPort downloads-754f7b8c6b-b5bj7 had {\u0027podNodeName\u0027: \u00272g9gmg7x-a6e36-78n6m-master-1\u0027, \u0027podUid\u0027: \u002796e393ad-5763-4532-8944-7d3c3d773033\u0027}[0] set, also the events of that namespace [1] states that \"Successfully assigned openshift-console/downloads-754f7b8c6b-b5bj7 to 2g9gmg7x-a6e36-78n6m-master-1\", but yet somehow the Pod spec is with Pending state[2] and the Pod retrieved by the controller had no nodeName set.\n\n\n[0] https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/logs/periodic-ci-shiftstack-shiftstack-ci-main-periodic-4.10-e2e-openstack-kuryr/1479396174820544512/artifacts/e2e-openstack-kuryr/gather-must-gather/artifacts/must-gather/namespaces/openshift-kuryr/pods/kuryr-controller-75d67f7b9d-gt7d6/controller/controller/logs/current.log\n[1] https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/logs/periodic-ci-shiftstack-shiftstack-ci-main-periodic-4.10-e2e-openstack-kuryr/1479396174820544512/artifacts/e2e-openstack-kuryr/gather-must-gather/artifacts/must-gather/namespaces/openshift-console/core/events.yaml\n[2] https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/logs/periodic-ci-shiftstack-shiftstack-ci-main-periodic-4.10-e2e-openstack-kuryr/1479396174820544512/artifacts/e2e-openstack-kuryr/gather-must-gather/artifacts/must-gather/namespaces/openshift-console/pods/downloads-754f7b8c6b-b5bj7/downloads-754f7b8c6b-b5bj7.yaml","commit_id":"9793b369abcecd72fe13581a670ccfcb85782a0c"},{"author":{"_account_id":33240,"name":"Sunday Mgbogu","email":"digitalsimboja@gmail.com","username":"digitalsimboja"},"change_message_id":"1073a532a362995ccf8db36e372c8a3b0dba7975","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"a6cf17c6_8f545cf7","updated":"2022-01-20 07:48:24.000000000","message":"recheck","commit_id":"f36f869fb3efcf6786e0e0c8bbbe03d5a7466637"}],"kuryr_kubernetes/controller/drivers/vif_pool.py":[{"author":{"_account_id":11600,"name":"Michał Dulko","email":"michal.dulko@gmail.com","username":"dulek"},"change_message_id":"5a4d928b266daf0f619f3c1c00174a3401f0f60d","unresolved":true,"context_lines":[{"line_number":1246,"context_line":"    def request_vif(self, pod, project_id, subnets, security_groups):"},{"line_number":1247,"context_line":"        try:"},{"line_number":1248,"context_line":"            pod_vif_type \u003d self._get_pod_vif_type(pod)"},{"line_number":1249,"context_line":"        except KeyError:"},{"line_number":1250,"context_line":"            # NOTE(maysams): No nodeName set. Event should be skipped"},{"line_number":1251,"context_line":"            return None"},{"line_number":1252,"context_line":"        return self._vif_drvs[pod_vif_type].request_vif("},{"line_number":1253,"context_line":"            pod, project_id, subnets, security_groups)"},{"line_number":1254,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9097b712_ce157d0c","line":1251,"range":{"start_line":1249,"start_character":0,"end_line":1251,"end_character":23},"updated":"2022-01-19 12:47:27.000000000","message":"Can we log a warning in this case? It might be good to inform that etcd seems to lose some state.","commit_id":"9793b369abcecd72fe13581a670ccfcb85782a0c"}]}