Bug 2005357: [4.8z] Fixes misuse of pod annotations during update event #753
Conversation
In the update pod logic, we pass the current pod event to addLogicalPort. In addLogicalPort we assume that if the annotations exist for the pod mac/ifaddr, then we use those and do not update annotations on the pod. This assumption is invalid, because this event may not be the current state of the pod. In other words we could have a situation where: 1. A pod add event comes we annotate with 10.0.0.2, assume OVN execute failure 2. Before the annotate is done, the pod is modified in some other way signaling another pod update event 3. A pod update event comes for 2, the pod is annotated with 10.0.0.3 because this was an update to the original pod, before it was annotated with 10.0.0.2, assume OVN execute failure 4. A pod update event comes for 1, since annotations existed, nothing is annotated and 10.0.0.2 is found to be used. OVN logical port is configured with 10.0.0.2. addLogicalPort succeeds. 5. Now the pod has 10.0.0.3 annotated, and 10.0.0.2 in OVN. CNI openflow check will fail and the pod will never come up. Signed-off-by: Tim Rozet <trozet@redhat.com>
openshift-ci
bot
added
the
bugzilla/severity-medium
|
@trozet: This pull request references Bugzilla bug 2005357, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
added
the
bugzilla/invalid-bug
openshift-ci
bot
added
the
approved
|
/bugzilla refresh |
|
@abhat: This pull request references Bugzilla bug 2005357, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/lgtm |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/bugzilla refresh Recalculating validity in case the underlying Bugzilla bug has changed. |
|
@openshift-bot: This pull request references Bugzilla bug 2005357, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/lgtm and /label qe-approved |
|
/bugzilla refresh Recalculating validity in case the underlying Bugzilla bug has changed. |
|
@openshift-bot: This pull request references Bugzilla bug 2005357, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/bugzilla refresh Recalculating validity in case the underlying Bugzilla bug has changed. |
|
@openshift-bot: This pull request references Bugzilla bug 2005357, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/bugzilla refresh |
openshift-ci
bot
added
bugzilla/valid-bug
|
@trozet: This pull request references Bugzilla bug 2005357, which is valid. 6 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (anusaxen@redhat.com), skipping review request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/bugzilla cc-qa |
|
@anuragthehatter: An error was encountered querying GitHub for users with public email (anusaxen@redhat.com) for bug 2005357 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
non-200 OK status code: 403 Forbidden body: "{\n \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"
Please contact an administrator to resolve this issue, then request a bug refresh with In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/lgtm |
openshift-ci
bot
added
the
qe-approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anuragthehatter, trozet, tssurya, yingwang-0320 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
[patch-manager] Approving for cherry-pick with a score of 1.70 |
hardys
added
the
cherry-pick-approved
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
12 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@trozet: All pull requests linked via external trackers have merged: Bugzilla bug 2005357 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
In the update pod logic, we pass the current pod event to
addLogicalPort. In addLogicalPort we assume that if the annotations
exist for the pod mac/ifaddr, then we use those and do not update
annotations on the pod. This assumption is invalid, because this event
may not be the current state of the pod. In other words we could have a
situation where:
failure
signaling another pod update event
because this was an update to the original pod, before it was
annotated with 10.0.0.2, assume OVN execute failure
annotated and 10.0.0.2 is found to be used. OVN logical port is
configured with 10.0.0.2. addLogicalPort succeeds.
check will fail and the pod will never come up.
Signed-off-by: Tim Rozet trozet@redhat.com
- What this PR does and why is it needed
- Special notes for reviewers
- How to verify it
- Description for the changelog