E2E tests: add basic disruption tests #221
Conversation
|
/assign @msherif1234 |
|
P.S if you want me to add reboot test scenario LMK. I know you wanted to do it previously. |
|
Also, I didnt hide these tests behind a flag because the tests arent adding that much latency to the overall test time. If they get too long later, we can do that. |
openshift-merge-robot
added
the
needs-rebase
to be to reuse unix socket we have to unlink them when the events container is shuting down Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
Signed-off-by: Martin Kennelly <mkennell@redhat.com>
Daemon may miss deletion of this object and therefore not handle any cleanup operations that needs to be performed. Wait for daemon to consume deletion update and only then allow this object to be deleted. Signed-off-by: Martin Kennelly <mkennell@redhat.com>
If the daemon is restarted, we start not knowing about any of the managed interfaces. If we perform activities like reseting everything due to IngressNodeFirewallNodeState deletion for example, we will not cleanup all XDP programs. IngNodeFwController knows all pinned interfaces and ensure we know about them in ebpfSingleton. Future work is needed to refactor this to not have two seperate sources of information about the same thing. Signed-off-by: Martin Kennelly <mkennell@redhat.com>
martinkennelly
force-pushed
the
disruption
branch
from
953ee96
to
957b7a9
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/retest |
|
/retest |
|
|
||
| func isNodeStateDeletionInProgress(nodeState *infv1alpha1.IngressNodeFirewallNodeState) bool { | ||
| return !nodeState.ObjectMeta.DeletionTimestamp.IsZero() | ||
| } |
There was a problem hiding this comment.
Hmm I did that in the past attempting to fix ut race but it really didn't help, I think this the right thing because fw and node state are different scope and there is an issue with garbage collection hence its recommended to use finalizers , the only thing here when I used the same there were issues I hit during testing which I don't remember them atm but given we have much more advanced e2e now and if this works I am fine with adding it
Can u explain what is the exact issue(s) the finalizers helped with currently the nodeState controller taking care of deleting objects.
There was a problem hiding this comment.
If daemon is not listening for events when node state is deleted, it will miss cleaning up the node. Finalizer ensures the daemon consumes the delete event for node state, performs the cleanup ops, and then removes the finalizer.
| @@ -87,6 +87,12 @@ func (e *ebpfSingleton) SyncInterfaceIngressRules( | |||
| return err | |||
| } | |||
|
|
|||
| // Ensure IngNodeFwController's pinned links and our managed interfaces align | |||
| // TODO: refactor to not have managed interfaces names from two sources | |||
| for _, linkName := range e.c.GetPinnedLinkNames() { | |||
There was a problem hiding this comment.
why we need this createNewManager will load pinned map ?
There was a problem hiding this comment.
pls make sure with any changes to this package u run this manual ut from ebpfsyncer folder
ebpfsyncer]$ go test -exec sudo ./...
|
@andreaskaris PTAL |
|
@msherif1234: GitHub didn't allow me to assign the following users: PTAL. Note that only openshift members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. 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. |
|
/test ingress-node-firewall-e2e-metal-ipi |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
openshift-merge-robot
added
the
needs-rebase
|
@martinkennelly: PR needs rebase. 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. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. 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. |
|
@martinkennelly: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: martinkennelly The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com>
|
PR #354 will continue this work |
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com>
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com>
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com>
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com>
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com>
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com>
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com>
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com>
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com>
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com>
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com> (cherry picked from commit 601efda)
This is continuation of github.com/openshift/pull/221 Signed-off-by: msherif1234 <mmahmoud@redhat.com>
Signed-off-by: Martin Kennelly mkennell@redhat.com
Expecting test named
IngressNodeFirewall policy is configurable after daemon deletionto fail - seen with KinD and bare metal server.INF policy configuration isn't working after daemon deletion.
Executed go mod tidy + go mod vendor
Executed make lint and its clean