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

monitor: Refactor listener registration logic #9924

Merged
merged 1 commit into from Feb 19, 2020
Merged

monitor: Refactor listener registration logic #9924

merged 1 commit into from Feb 19, 2020

Conversation

michi-covalent
Copy link
Contributor

@michi-covalent michi-covalent commented Jan 22, 2020 •
edited

  • Modify registerNewListener() to take MonitorListener as a parameter
    to allow arbitrary listener to be registered instead of assuming the
    type of listener is always listenerv1_2.
  • Add Close() method to MonitorListener so that the Monitor can close
    listeners without knowing implementation details.

Ref #9925

Signed-off-by: Michi Mutsuzaki michi@isovalent.com

This change is Reviewable

@michi-covalent michi-covalent added kind/enhancement This would improve or streamline existing functionality. pending-review area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. labels Jan 22, 2020
@michi-covalent michi-covalent requested a review from a team as a code owner January 22, 2020 00:46
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

2 similar comments
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.7.0 Jan 22, 2020
@michi-covalent michi-covalent added the release-note/misc This PR makes changes that have no direct user impact. label Jan 22, 2020
@coveralls
Copy link

coveralls commented Jan 22, 2020 •
edited

Coverage Status

Coverage increased (+0.02%) to 44.573% when pulling 79cc0f3 on pr/michi/listener into f5c42e7 on master.

joestringer
joestringer approved these changes Jan 22, 2020
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.

Minor nits below, lgtm.

pkg/monitor/agent/listener1_2.go Outdated Show resolved Hide resolved
pkg/monitor/agent/listener1_2.go Outdated Show resolved Hide resolved
@joestringer
Copy link
Member

test-me-please

@aanm aanm added this to the 1.8 milestone Jan 22, 2020
@aanm aanm added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Jan 22, 2020
@michi-covalent michi-covalent force-pushed the pr/michi/listener branch 2 times, most recently from 187d8c0 to d869304 Compare January 22, 2020 16:18
@joestringer
Copy link
Member

test-me-please

1 similar comment
@michi-covalent
Copy link
Contributor Author

test-me-please

@michi-covalent
Copy link
Contributor Author

@aanm @joestringer can this be merged now?

@tgraf tgraf removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Feb 12, 2020
@michi-covalent
monitor: Refactor listener registration logic
79cc0f3 
- Modify registerNewListener() to take MonitorListener as a parameter
  to allow arbitrary listener to be registered instead of assuming the
  type of listener is always listenerv1_2.
- Add Close() method to MonitorListener so that the Monitor can close
  listeners without knowing implementation details.
- Explicitly call close() on listenerv1_2.queue so that drainQueue gets
  unblocked during unit test.

Ref #9925

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@tgraf
Copy link
Member

tgraf commented Feb 12, 2020

test-me-please

@tgraf tgraf added this to In progress in 1.8.0 via automation Feb 12, 2020
@tgraf tgraf removed this from In progress in 1.7.0 Feb 12, 2020
@michi-covalent
Copy link
Contributor Author

test-me-please

@aanm aanm added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Feb 14, 2020
@aanm aanm modified the milestones: 1.8, 1.7 Feb 18, 2020
@aanm aanm removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Feb 18, 2020
@aanm
Copy link
Member

aanm commented Feb 18, 2020

test-me-please

@michi-covalent
Copy link
Contributor Author

michi-covalent commented Feb 19, 2020 •
edited by aanm

test-me-please

failure https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/17269

@aanm
Copy link
Member

aanm commented Feb 19, 2020

test-me-please

@aanm aanm mentioned this pull request Feb 19, 2020
Merged
@tgraf tgraf merged commit 926ad33 into master Feb 19, 2020
1.8.0 automation moved this from In progress to Merged Feb 19, 2020
@tgraf tgraf deleted the pr/michi/listener branch February 19, 2020 15:03
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

@tgraf tgraf tgraf approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Milestone
1.8
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@michi-covalent @coveralls @joestringer @tgraf @aanm