inspect: add hostnetwork information #9411
Conversation
which could allow cadvisor to choose not to report network metrics Signed-off-by: Peter Hunt <pehunt@redhat.com>
|
inspired by https://issues.redhat.com/browse/OCPSTRAT-2346 |
| LogPath: ctr.LogPath(), | ||
| Sandbox: ctr.Sandbox(), | ||
| IPs: sb.IPs(), | ||
| HostNetwork: ptr.To(sb.HostNetwork()), |
There was a problem hiding this comment.
I think this could potentially seg fault.
Should you use deref?
There was a problem hiding this comment.
how would it segfault? HostNetwork() returns a bool, and ptr.To() just creates a pointer out of that returned bool
There was a problem hiding this comment.
HostNetwork is a pointer to a bool though.
couldn't that be nil?
There was a problem hiding this comment.
sb.HostNetwork() returns a bool, the HostNetwork we nest in the ContainerInfo field is a pointer. They're different
There was a problem hiding this comment.
Why the use of a bool pointer here?
In kube we would use it for marshaling non default fields but I don't see a reason to have this here.
There was a problem hiding this comment.
sb.HostNetwork() returns a bool, the HostNetwork we nest in the ContainerInfo field is a pointer. They're different
I see. Yea I got confused.
There was a problem hiding this comment.
There's already a nil check for sb at lines 88-93
There was a problem hiding this comment.
In kube we would use it for marshaling non default fields but I don't see a reason to have this here.
exactly this reason. Eventually cadvisor will consume this field, and we need to be able to differentiate between cri-o not supporting the field and the field being false.
|
critest failures reproduced here #9410 so unrelated |
Looks like the cri-tools CI is also broken with the same failure |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9411 +/- ##
==========================================
- Coverage 66.98% 66.00% -0.98%
==========================================
Files 198 202 +4
Lines 27308 27761 +453
==========================================
+ Hits 18291 18325 +34
- Misses 7508 7887 +379
- Partials 1509 1549 +40 🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haircommander, saschagrunert 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 |
|
/cherry-pick release-1.33 |
|
@haircommander: new pull request created: #9413 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-sigs/prow repository. |
What type of PR is this?
/kind api-change
What this PR does / why we need it:
which could allow cadvisor to choose not to report network metrics
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?