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.

Sign up for GitHub

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

Jump to bottom

Record event when EgressIP remains unassigned #6011

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Record event when EgressIP remains unassigned #6011

merged 1 commit into from
Apr 24, 2024

Conversation

jainpulkit22
Copy link
Contributor

Record event when Egress is uninstalled from a node.

FIxes #5996.

@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch 2 times, most recently from b710677 to d8cee6f Compare February 22, 2024 10:35
@jainpulkit22 jainpulkit22 changed the title Record event when Egress is uninstalled from a node Record event when EgressIP remains unassigned Feb 22, 2024
@jainpulkit22 jainpulkit22 requested a review from tnqn February 22, 2024 11:18
@jainpulkit22 jainpulkit22 marked this pull request as ready for review February 22, 2024 11:18
tnqn
tnqn reviewed Feb 23, 2024
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need unit tests to validate correct events are generated for each case.

pkg/agent/controller/egress/egress_controller.go Outdated Show resolved Hide resolved
@rajnkamr rajnkamr added this to the Antrea v2.0 release milestone Mar 1, 2024
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch from d8cee6f to cf475e7 Compare March 5, 2024 11:09
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch from cf475e7 to 13c4377 Compare March 18, 2024 06:21
@jainpulkit22
Copy link
Contributor Author

I think we need unit tests to validate correct events are generated for each case.

I have added e2e test to validate the correctness of events generated in each case.

@jainpulkit22 jainpulkit22 requested a review from tnqn March 18, 2024 09:33
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch from 13c4377 to fc48567 Compare March 18, 2024 10:01
tnqn
tnqn reviewed Mar 18, 2024
View reviewed changes
test/e2e/egress_test.go Outdated Show resolved Hide resolved
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch from fc48567 to 0af8495 Compare April 8, 2024 04:48
@jainpulkit22 jainpulkit22 requested a review from tnqn April 8, 2024 08:35
tnqn
tnqn reviewed Apr 16, 2024
View reviewed changes
pkg/agent/controller/egress/egress_controller_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/egress/egress_controller_test.go Outdated Show resolved Hide resolved
test/e2e/egress_test.go Outdated Show resolved Hide resolved
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch from 0af8495 to dcbd119 Compare April 18, 2024 07:34
tnqn
tnqn reviewed Apr 19, 2024
View reviewed changes
pkg/agent/controller/egress/egress_controller_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/egress/egress_controller_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/egress/egress_controller_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/egress/egress_controller_test.go Outdated Show resolved Hide resolved
test/e2e/egress_test.go Outdated Show resolved Hide resolved
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch from dcbd119 to 40e620c Compare April 22, 2024 08:20
@jainpulkit22 jainpulkit22 marked this pull request as draft April 22, 2024 08:20
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch 2 times, most recently from e6b4362 to 2f867d7 Compare April 22, 2024 10:25
@jainpulkit22 jainpulkit22 marked this pull request as ready for review April 22, 2024 12:24
@jainpulkit22 jainpulkit22 requested a review from tnqn April 22, 2024 12:25
tnqn
tnqn reviewed Apr 22, 2024
View reviewed changes
test/e2e/egress_test.go Outdated Show resolved Hide resolved
test/e2e/egress_test.go Outdated Show resolved Hide resolved
test/e2e/egress_test.go Outdated Show resolved Hide resolved
test/e2e/egress_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/egress/egress_controller_test.go Show resolved Hide resolved
pkg/agent/controller/egress/egress_controller_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/egress/egress_controller_test.go Outdated Show resolved Hide resolved
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch from 2f867d7 to cad78d9 Compare April 23, 2024 06:10
tnqn
tnqn reviewed Apr 23, 2024
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future PRs, please make sure the comments are really addressed, the tests can pass basic validation, and you have really confirmed answers of questions before resolving them and requesting another review.

test/e2e/egress_test.go Outdated
@@ -600,6 +600,20 @@ func testEgressUpdateEgressIP(t *testing.T, data *TestData) {
return err
})
require.NoError(t, err, "Failed to update Egress")
// Testing the events recorded during deletion of an Egress resource.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment no longer makes sense here, the events are not really specific to deletion of an Egress resource, having the comment just confuses readers.

@jainpulkit22 jainpulkit22 marked this pull request as draft April 23, 2024 06:45
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch from cad78d9 to 1c232f9 Compare April 23, 2024 10:00
@jainpulkit22 jainpulkit22 marked this pull request as ready for review April 23, 2024 10:02
tnqn
tnqn previously approved these changes Apr 23, 2024
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tnqn
Copy link
Member

tnqn commented Apr 23, 2024

/test-e2e
/skip-conformance
/skip-networkpolicy

@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch from 1c232f9 to dfa547f Compare April 23, 2024 13:12
tnqn
tnqn reviewed Apr 24, 2024
View reviewed changes
test/e2e/egress_test.go Outdated Show resolved Hide resolved
@jainpulkit22
Record event when EgressIP remains unassigned
c9d82ab 
Modified the code to record an event for case, when an egressIP is
unassigned from a node and is not assigned to any other node.

Signed-off-by: Pulkit Jain <pulkit.jain@broadcom.com>
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch from dfa547f to c9d82ab Compare April 24, 2024 04:29
tnqn
tnqn approved these changes Apr 24, 2024
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tnqn
Copy link
Member

tnqn commented Apr 24, 2024

/skip-all

1 similar comment
@tnqn
Copy link
Member

tnqn commented Apr 24, 2024

/skip-all

@tnqn tnqn merged commit 6dfc306 into antrea-io:main Apr 24, 2024
49 of 52 checks passed
@jainpulkit22 jainpulkit22 deleted the egress-record-update branch April 24, 2024 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnqn tnqn tnqn approved these changes

@rajnkamr rajnkamr Awaiting requested review from rajnkamr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Antrea v2.0 release
3 participants
@jainpulkit22 @tnqn @rajnkamr