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.

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

NETOBSERV-1283: Make ifaces watcher netns aware #171

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

msherif1234
Copy link
Contributor

@msherif1234 msherif1234 commented Aug 28, 2023

today netobserv agent only subscribe to watch interface in default netns
for PODs with VF SRIOV interfaces they only show in custom netns made them invisible to network observability to fix this netobserv agent interface manager component needs to netns aware
needs netobserv/network-observability-operator#406 to setup netns mount

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Patch coverage: 40.54% and project coverage change: -0.02% ⚠️

Comparison is base (6d9d2e7) 38.60% compared to head (cfeb767) 38.58%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
- Coverage   38.60%   38.58%   -0.02%     
==========================================
  Files          31       31              
  Lines        2259     2340      +81     
==========================================
+ Hits          872      903      +31     
- Misses       1338     1383      +45     
- Partials       49       54       +5     
Flag Coverage Δ
unittests 38.58% <40.54%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
pkg/ebpf/tracer.go 0.00% <0.00%> (ø)
pkg/ifaces/informer.go 0.00% <0.00%> (ø)
pkg/ifaces/watcher.go 58.46% <48.38%> (-23.36%) ⬇️

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 28, 2023
@github-actions
Copy link

New image:
quay.io/netobserv/netobserv-ebpf-agent:a960c44

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=a960c44 make set-agent-image

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 28, 2023
@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 28, 2023
@github-actions
Copy link

New image:
quay.io/netobserv/netobserv-ebpf-agent:e401fa0

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=e401fa0 make set-agent-image

@msherif1234 msherif1234 changed the title Make ifaces watcher netns aware OCPBUGS-17785: Make ifaces watcher netns aware Aug 29, 2023
@openshift-ci-robot
Copy link
Collaborator

@msherif1234: This pull request references Jira Issue OCPBUGS-17785, which is invalid:

  • expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

today netobserv agent only subscribe to watch interface in default netns
for PODs with VF SRIOV interfaces they only show in netns make them invisible to network observability

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.

@msherif1234
Copy link
Contributor Author

/jira refresh

@openshift-ci-robot
Copy link
Collaborator

@msherif1234: This pull request references Jira Issue OCPBUGS-17785, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.14.0) matches configured target version for branch (4.14.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @memodi

In response to this:

/jira refresh

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 openshift-ci bot requested a review from memodi August 29, 2023 16:46
@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 29, 2023
@msherif1234
Copy link
Contributor Author

/ok-to-test

1 similar comment
@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 29, 2023
@github-actions
Copy link

New image:
quay.io/netobserv/netobserv-ebpf-agent:e26f1d5

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=e26f1d5 make set-agent-image

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 29, 2023
@msherif1234
Copy link
Contributor Author

/ok-to-test

1 similar comment
@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 29, 2023
@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 2, 2023
@github-actions
Copy link

github-actions bot commented Sep 2, 2023

New image:
quay.io/netobserv/netobserv-ebpf-agent:652a4b6

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=652a4b6 make set-agent-image

@openshift-ci-robot
Copy link
Collaborator

@msherif1234: This pull request references Jira Issue OCPBUGS-17785, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.14.0) matches configured target version for branch (4.14.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @memodi

In response to this:

today netobserv agent only subscribe to watch interface in default netns
for PODs with VF SRIOV interfaces they only show in custom netns made them invisible to network observability to fix this netobserv agent interface manager component needs to netns aware

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.

if link.Flags&(syscall.IFF_UP|syscall.IFF_RUNNING) != 0 {
iface := Interface{Name: attrs.Name, Index: attrs.Index, NetNS: netnsHandle}
w.mutex.Lock()
if link.Flags&(syscall.IFF_UP|syscall.IFF_RUNNING) != 0 && attrs.OperState == netlink.OperUp {
Copy link

Choose a reason for hiding this comment

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

For the record attrs.OperState == netlink.OperUp is needed to avoid calling EventAdded too soon since it will come up with OperState set to Down first.


func getNetNSHandles(ctx context.Context) ([]netns.NsHandle, error) {
log := logrus.WithField("component", "ifaces.Watcher")
out, err := exec.CommandContext(ctx, "ls", "-lt", "/var/run/netns/").Output()
Copy link

@bn222 bn222 Sep 4, 2023

Choose a reason for hiding this comment

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

Can we use proper golang functions for this instead of exec.CommandContext? Maybe ioutil.ReadDir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point swtiched to os.ReadDir() Thanks!!

@bn222
Copy link

bn222 commented Sep 4, 2023

Added a small comment. Otherwise LGTM

Signed-off-by: msherif1234 <mmahmoud@redhat.com>
Signed-off-by: msherif1234 <mmahmoud@redhat.com>
@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 4, 2023
@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 4, 2023
@github-actions
Copy link

github-actions bot commented Sep 4, 2023

New image:
quay.io/netobserv/netobserv-ebpf-agent:2ef08da

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=2ef08da make set-agent-image

@jotak
Copy link
Member

jotak commented Sep 6, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 6, 2023
@msherif1234
Copy link
Contributor Author

@memodi can u pls do sanity check for this PR with the operator netobserv/network-observability-operator#406 we agreed to make sure it doesn't introduce regression and sriov testing can be done after the merge

@openshift-ci-robot
Copy link
Collaborator

@msherif1234: This pull request references Jira Issue OCPBUGS-17785, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.14.0) matches configured target version for branch (4.14.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @memodi

In response to this:

today netobserv agent only subscribe to watch interface in default netns
for PODs with VF SRIOV interfaces they only show in custom netns made them invisible to network observability to fix this netobserv agent interface manager component needs to netns aware
needs netobserv/network-observability-operator#406 to setup netns mount

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.

@jotak jotak changed the title OCPBUGS-17785: Make ifaces watcher netns aware NETOBSERV-1283: Make ifaces watcher netns aware Sep 7, 2023
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Sep 7, 2023

@msherif1234: This pull request references NETOBSERV-1283 which is a valid jira issue.

In response to this:

today netobserv agent only subscribe to watch interface in default netns
for PODs with VF SRIOV interfaces they only show in custom netns made them invisible to network observability to fix this netobserv agent interface manager component needs to netns aware
needs netobserv/network-observability-operator#406 to setup netns mount

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.

@memodi
Copy link
Contributor

memodi commented Sep 7, 2023

SRIOV testing passed, currently running select e2e regression tests with Operator PR netobserv/network-observability-operator#406

@memodi
Copy link
Contributor

memodi commented Sep 7, 2023

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved QE has approved this pull request label Sep 7, 2023
@jotak
Copy link
Member

jotak commented Sep 8, 2023

thanks @msherif1234 @bn222 @memodi !
/approve

@openshift-ci
Copy link

openshift-ci bot commented Sep 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Sep 8, 2023
@openshift-merge-robot openshift-merge-robot merged commit a7d4eec into netobserv:main Sep 8, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved jira/valid-reference lgtm ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. qe-approved QE has approved this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants