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

Extend the monitor notification interface with endpoint id getter #15391

Merged
merged 2 commits into from
Mar 23, 2021
Merged

Extend the monitor notification interface with endpoint id getter #15391

merged 2 commits into from
Mar 23, 2021

Conversation

aditighag
Copy link
Member

See commit message.

@aditighag aditighag requested a review from a team as a code owner March 18, 2021 23:05
@aditighag aditighag requested a review from a team March 18, 2021 23:05
@aditighag aditighag requested a review from a team as a code owner March 18, 2021 23:05
@aditighag aditighag requested review from a team, nathanjsweet and glibsm March 18, 2021 23:05
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 18, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Mar 18, 2021
@aditighag aditighag added the release-note/misc This PR makes changes that have no direct user impact. label Mar 19, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 19, 2021
joestringer
joestringer approved these changes Mar 19, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

I didn't know about the nil cast thing for compile time assertion on types. Nice :)

@aditighag aditighag force-pushed the pr/aditighag/monitor-endpoint-id-method branch from 11235f2 to c661536 Compare March 19, 2021 17:04
@aditighag aditighag changed the title Extend the monitor notification interface with method to get endpoint id Extend the monitor notification interface with endpoint id getter Mar 19, 2021
@aditighag aditighag requested a review from rolinh March 19, 2021 17:05
@aditighag
Copy link
Member Author

test-me-please

@aditighag
Copy link
Member Author

test-1.20-4.9

@aditighag
Copy link
Member Author

aditighag commented Mar 20, 2021 •
edited

The sole test failure is k8s-1.20-kernel-4.9. I see that this test "Checks ClusterIP Connectivity Checks DualStack services" has been failing on other PRs too -

https://jenkins.cilium.io/job/Cilium-PR-K8s-1.20-kernel-4.9/1012/testReport/Suite-k8s-1/20/K8sServicesTest_Checks_ClusterIP_Connectivity_Checks_DualStack_services/history/

@aditighag
Copy link
Member Author

test-1.20-4.9

1 similar comment
@aditighag
Copy link
Member Author

test-1.20-4.9

@aditighag
endpoint: Use nil type cast for compile time assertions
ca351f5 
Memory allocation may occur for the big `Endpoint` struct
if compiler optimizations are disabled.
The official Go documentation recommends using `nil` type cast
to ensure that (*T) implements an interface at compile time.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag
monitor: Extend monitor notification with 16-bit Endpoint ID getter
626266a 
Endpoint ID field is defined as 16-bit. However, the monitor
notification interface converts this to 64-bit. To keep things
consistent, and not break consumers of the interface, add a new
method to the existing interface. Consumers then won't have to
do unnecessary conversions while retrieving the original endpoint ID.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag force-pushed the pr/aditighag/monitor-endpoint-id-method branch from c661536 to 626266a Compare March 23, 2021 00:18
@aditighag
Copy link
Member Author

One reason why I feel the test failures are unrelated because this PR only extends an existing interface. If there were issues related to run time cast due to which the interface isn't being satisfied, then we would've seen more obvious and deterministic failures.

Since all the other tests had passed in earlier runs, I'm triggering only thetest-1.20-4.9 test.

@aditighag
Copy link
Member Author

test-1.20-4.9

@aditighag
Copy link
Member Author

Quay is down. Will need to re-try.

@aditighag
Copy link
Member Author

test-1.20-4.9

@aditighag
Copy link
Member Author

08:05:27 VBoxManage: error: Details: code VBOX_E_INVALID_OBJECT_STATE (0x80bb0007), component MediumWrap, interface IMedium
08:05:27 VBoxManage: error: Context: "RTEXITCODE handleUnregisterVM(HandlerArg*)" at line 162 of file VBoxManageMisc.cpp

Provisioning related issue.

@aditighag aditighag closed this Mar 23, 2021
@aditighag aditighag reopened this Mar 23, 2021
@aditighag aditighag added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 23, 2021
@aditighag aditighag requested a review from a team March 23, 2021 19:46
@joestringer joestringer merged commit 9d94081 into cilium:master Mar 23, 2021
1.10.0 automation moved this from In progress to Done Mar 23, 2021
This was referenced Apr 28, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@rolinh rolinh rolinh approved these changes

@christarazi christarazi christarazi approved these changes

@nathanjsweet nathanjsweet Awaiting requested review from nathanjsweet

@glibsm glibsm Awaiting requested review from glibsm

Assignees

@glibsm glibsm

@nathanjsweet nathanjsweet

Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aditighag @joestringer @rolinh @christarazi @glibsm @nathanjsweet