OCPBUGS-59311: pkg/.../inspect: Add support for context.Context #2100
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@tchap: This pull request references Jira Issue OCPBUGS-59311, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughPropagates context.Context throughout the inspect subsystem: adds InspectOptions.RunContext, updates InspectResource and many gather/inspect function signatures to accept ctx and a resourceContext, and makes file-writer APIs context-aware. All API calls, listing, streaming, and writes now receive ctx; control flow unchanged. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tchap
force-pushed
the
inspect-add-context
branch
from
222ee8e to
e44e7ec
Compare
|
/lgtm |
|
/lgtm cancel |
openshift-ci
bot
added
the
approved
|
/lgtm |
openshift-ci
bot
added
lgtm
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
pkg/cli/admin/inspect/inspect.go (2)
310-323: Use filepath.Join; current Join drops destDir due to leading “/”path.Join(destDir, "/"+filename) produces an absolute path (e.g., "/foo.yaml"), ignoring destDir. Use filepath.Join without a leading slash.
- if err := o.fileWriter.WriteFromResource(ctx, path.Join(destDir, "/"+filename), objToPrint); err != nil { + if err := o.fileWriter.WriteFromResource(ctx, filepath.Join(destDir, filename), objToPrint); err != nil {Also add:
import "path/filepath"
351-364: Same path bug here; fix Join to keep destDirThis has the same absolute-path issue.
- if err := o.fileWriter.WriteFromResource(ctx, path.Join(destDir, "/"+filename), objToPrint); err != nil { + if err := o.fileWriter.WriteFromResource(ctx, filepath.Join(destDir, filename), objToPrint); err != nil {pkg/cli/admin/inspect/pod.go (5)
35-45: Fix container subdir path and remove double-join
- path.Join(destDir, "/"+container.Name) discards destDir.
- gatherContainerInfo again appends container.Name, duplicating the segment.
Pass a proper container dir once and stop re‑joining inside gatherContainerInfo.
- if err := o.gatherContainerInfo(ctx, path.Join(destDir, "/"+container.Name), pod, container); err != nil { + if err := o.gatherContainerInfo(ctx, filepath.Join(destDir, container.Name), pod, container); err != nil {- if err := o.gatherContainerInfo(ctx, path.Join(destDir, "/"+container.Name), pod, container); err != nil { + if err := o.gatherContainerInfo(ctx, filepath.Join(destDir, container.Name), pod, container); err != nil {Inside gatherContainerInfo:
- if err := o.gatherContainerAllLogs(ctx, path.Join(destDir, "/"+container.Name), pod, &container); err != nil { + if err := o.gatherContainerAllLogs(ctx, destDir, pod, &container); err != nil {Also applies to: 53-59
121-129: Leak: close response body from Stream(ctx)res must be closed to avoid FD/memory leaks.
- res, err := req.Stream(ctx) + res, err := req.Stream(ctx) if err != nil { return err } + defer res.Close()
225-236: Fix output file paths for current logsSame absolute-path issue; use filepath.Join.
- if err := o.fileWriter.WriteFromSource(ctx, path.Join(destDir, "/"+filename), logsReq); err != nil { + if err := o.fileWriter.WriteFromSource(ctx, filepath.Join(destDir, filename), logsReq); err != nil {- if err := o.fileWriter.WriteFromSource(ctx, path.Join(destDir, "/"+filename), logsReq); err != nil { + if err := o.fileWriter.WriteFromSource(ctx, filepath.Join(destDir, filename), logsReq); err != nil {Also applies to: 232-235
3-18: Add filepath import for filesystem pathsSince we’re switching to filepath.Join for files, add this import (keep path for URL joins).
import ( "context" "fmt" "os" - "path" + "path" + "path/filepath" "regexp" "strings" "sync" "time" // ... )
1-1: Fix path.Join misuse — leading '/' discards the base; remove leading '/' and prefer filepath.Join for filesystem pathsSeveral calls use path.Join(..., "/...") which yields the absolute segment (dropping the base dir) — this will write/read the wrong paths. Replace with filepath.Join(base, "rel") or at minimum remove the leading '/' from the joined segments.
Affected locations:
- pkg/cli/admin/mustgather/mustgather.go:822 — path.Join(destDir, "/gather.logs")
- pkg/cli/admin/inspect/resource.go:47 — path.Join(o.DestDir, "/cluster-scoped-resources/config.openshift.io")
- pkg/cli/admin/inspect/resource.go:52 — path.Join(o.DestDir, "/cluster-scoped-resources/operator.openshift.io")
- pkg/cli/admin/inspect/resource.go:274 — path.Join(baseDir, "/"+clusterScopedResourcesDirname, "/"+obj.GroupVersionKind().Group, "/clusteroperators")
- pkg/cli/admin/inspect/resource.go:280 — path.Join(destDir, "/"+filename)
- pkg/cli/admin/inspect/inspect.go:318 — path.Join(destDir, "/"+filename)
- pkg/cli/admin/inspect/inspect.go:359 — path.Join(destDir, "/"+filename)
- pkg/cli/admin/inspect/pod.go:27 — path.Join(destDir, "/"+filename)
- pkg/cli/admin/inspect/pod.go:35 — path.Join(destDir, "/"+container.Name)
- pkg/cli/admin/inspect/pod.go:41 — path.Join(destDir, "/"+container.Name)
- pkg/cli/admin/inspect/pod.go:54 — path.Join(destDir, "/"+container.Name)
- pkg/cli/admin/inspect/pod.go:68 — path.Join(destDir, "/logs")
- pkg/cli/admin/inspect/pod.go:73 — path.Join(destDir, "/logs/rotated")
- pkg/cli/admin/inspect/pod.go:225 — path.Join(destDir, "/"+filename)
- pkg/cli/admin/inspect/pod.go:232 — path.Join(destDir, "/"+filename)
- pkg/cli/admin/inspect/pod.go:260 — path.Join(destDir, "/"+filename)
- pkg/cli/admin/inspect/pod.go:267 — path.Join(destDir, "/"+filename)
- pkg/cli/admin/inspect/namespace.go:57 — path.Join(destDir, "/"+filename)
- pkg/cli/admin/inspect/namespace.go:84 — path.Join(destDir, "/pods/"+pod.GetName())
🧹 Nitpick comments (2)
pkg/cli/admin/inspect/inspect.go (2)
200-203: Wire real cancellation instead of context.TODO()Run() delegating to RunContext(context.TODO()) loses CTRL‑C/parent cancellation. Prefer passing Cobra’s command context.
Apply in NewCmdInspect (outside this hunk):
// in NewCmdInspect(...) Run: func(c *cobra.Command, args []string) { kcmdutil.CheckErr(o.Complete(args)) kcmdutil.CheckErr(o.Validate()) // use command context so cancellations propagate kcmdutil.CheckErr(o.RunContext(c.Context())) },
398-405: Optional: prefer filepath.Join for filesystem paths project‑widelogTimestamp uses path.Join. For OS paths, filepath.Join is more appropriate. Consider a follow‑up sweep for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (10)
pkg/cli/admin/inspect/admission_webhooks.go(3 hunks)pkg/cli/admin/inspect/apiextensions.go(3 hunks)pkg/cli/admin/inspect/inspect.go(7 hunks)pkg/cli/admin/inspect/namespace.go(5 hunks)pkg/cli/admin/inspect/pod.go(8 hunks)pkg/cli/admin/inspect/proxy.go(3 hunks)pkg/cli/admin/inspect/resource.go(10 hunks)pkg/cli/admin/inspect/route.go(3 hunks)pkg/cli/admin/inspect/secret.go(3 hunks)pkg/cli/admin/inspect/writer.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/cli/admin/inspect/route.go
- pkg/cli/admin/inspect/proxy.go
- pkg/cli/admin/inspect/apiextensions.go
- pkg/cli/admin/inspect/admission_webhooks.go
- pkg/cli/admin/inspect/writer.go
- pkg/cli/admin/inspect/namespace.go
- pkg/cli/admin/inspect/resource.go
- pkg/cli/admin/inspect/secret.go
🔇 Additional comments (5)
pkg/cli/admin/inspect/inspect.go (3)
247-249: Good: discovery now honors ctxPropagating ctx into inspectDiscovery enables cancel/timeout during discovery.
288-295: Good: visited set moved under resourceContextThis removes accidental coupling with ctx and keeps state localized.
266-269: Signature change verified — all callers updatedDefinition: pkg/cli/admin/inspect/resource.go:31; callers updated at pkg/cli/admin/inspect/resource.go:84, pkg/cli/admin/inspect/resource.go:247, pkg/cli/admin/inspect/inspect.go:268.
pkg/cli/admin/inspect/pod.go (2)
185-187: LGTM: fileWriter.WriteFromSource with ctxCorrectly passes ctx to the stream write.
20-28: Context propagation changes look good overallAll I/O/network calls now accept ctx; control flow unchanged. With the path fixes and response Close added, this will honor cancellation cleanly.
Also applies to: 35-45, 53-59, 61-81, 102-196, 197-278
context.Context is now being passed around to support interrupting the whole inspection process. RunContext method is the entry point.
tchap
force-pushed
the
inspect-add-context
branch
from
e44e7ec to
4e6421d
Compare
|
/lgtm |
|
/verified by @tchap |
openshift-ci-robot
added
the
verified
|
@tchap: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, tchap 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/cli/admin/inspect/pod.go (1)
120-126: Close response body from Stream to avoid leakres is an io.ReadCloser; html.Parse does not close it. Defer a Close after error check.
- res, err := req.Stream(ctx) + res, err := req.Stream(ctx) if err != nil { return err } + defer res.Close()pkg/cli/admin/inspect/inspect.go (1)
359-363: Same path issue in operator resourcesMirror the fix here.
- if err := o.fileWriter.WriteFromResource(ctx, path.Join(destDir, "/"+filename), objToPrint); err != nil { + if err := o.fileWriter.WriteFromResource(ctx, filepath.Join(destDir, filename), objToPrint); err != nil {pkg/cli/admin/inspect/resource.go (1)
274-281: Fix clusteroperator output paths (absolute path + filepath.Join)Building destDir with “/” components makes it absolute; writing filename also uses a leading slash.
- destDir := path.Join(baseDir, "/"+clusterScopedResourcesDirname, "/"+obj.GroupVersionKind().Group, "/clusteroperators") + destDir := filepath.Join(baseDir, clusterScopedResourcesDirname, obj.GroupVersionKind().Group, "clusteroperators") @@ - return fileWriter.WriteFromResource(ctx, path.Join(destDir, "/"+filename), obj) + return fileWriter.WriteFromResource(ctx, filepath.Join(destDir, filename), obj)
♻️ Duplicate comments (1)
pkg/cli/admin/inspect/pod.go (1)
27-29: Fix absolute/incorrect filesystem paths (leading “/” + use filepath.Join)Using path.Join with components prefixed by “/” makes absolute paths and ignores destDir; also use path/filepath for filesystem paths. This currently risks writing under “/” and scattering outputs. Apply consistently.
Diffs (representative):
@@ - if err := o.fileWriter.WriteFromResource(ctx, path.Join(destDir, "/"+filename), pod); err != nil { + if err := o.fileWriter.WriteFromResource(ctx, filepath.Join(destDir, filename), pod); err != nil { @@ - if err := o.gatherContainerInfo(ctx, path.Join(destDir, "/"+container.Name), pod, container); err != nil { + if err := o.gatherContainerInfo(ctx, filepath.Join(destDir, container.Name), pod, container); err != nil { @@ - if err := o.gatherContainerLogs(ctx, path.Join(destDir, "/logs"), pod, container); err != nil { + if err := o.gatherContainerLogs(ctx, filepath.Join(destDir, "logs"), pod, container); err != nil { @@ - if err := o.gatherContainerRotatedLogFiles(ctx, path.Join(destDir, "/logs/rotated"), pod, container); err != nil { + if err := o.gatherContainerRotatedLogFiles(ctx, filepath.Join(destDir, "logs", "rotated"), pod, container); err != nil { @@ - if err := o.fileWriter.WriteFromSource(ctx, path.Join(destDir, fileName), logsReq); err != nil { + if err := o.fileWriter.WriteFromSource(ctx, filepath.Join(destDir, fileName), logsReq); err != nil { @@ - if err := o.fileWriter.WriteFromSource(ctx, path.Join(destDir, "/"+filename), logsReq); err != nil { + if err := o.fileWriter.WriteFromSource(ctx, filepath.Join(destDir, filename), logsReq); err != nil { @@ - if err := o.fileWriter.WriteFromSource(ctx, path.Join(destDir, "/"+filename), logsReq); err != nil { + if err := o.fileWriter.WriteFromSource(ctx, filepath.Join(destDir, filename), logsReq); err != nil {Add import:
import ( "context" "fmt" "os" - "path" + "path" + "path/filepath" "regexp" "strings" "sync" "time" )Note: keep package path for URL joins (e.g., path.Join(containerPath, fileName)) but use filepath.Join for disk paths.
Also applies to: 35-39, 41-45, 69-71, 73-76, 185-187, 225-235, 260-270
🧹 Nitpick comments (2)
pkg/cli/admin/inspect/inspect.go (1)
200-202: Run delegates to RunContext — good. Consider wiring signal cancellation.Follow-up (optional): in NewCmdInspect’s Run, derive ctx via signal.NotifyContext and call RunContext(ctx) so Ctrl+C cancels in-flight IO.
Also applies to: 204-205
pkg/cli/admin/inspect/resource.go (1)
217-219: Use filepath.Join for filesystem writeNon-breaking cleanup for portability/consistency.
- if err := o.fileWriter.WriteFromResource(ctx, path.Join(dirPath, filename), info.Object); err != nil { + if err := o.fileWriter.WriteFromResource(ctx, filepath.Join(dirPath, filename), info.Object); err != nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (10)
pkg/cli/admin/inspect/admission_webhooks.go(3 hunks)pkg/cli/admin/inspect/apiextensions.go(3 hunks)pkg/cli/admin/inspect/inspect.go(7 hunks)pkg/cli/admin/inspect/namespace.go(5 hunks)pkg/cli/admin/inspect/pod.go(8 hunks)pkg/cli/admin/inspect/proxy.go(3 hunks)pkg/cli/admin/inspect/resource.go(10 hunks)pkg/cli/admin/inspect/route.go(3 hunks)pkg/cli/admin/inspect/secret.go(3 hunks)pkg/cli/admin/inspect/writer.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/cli/admin/inspect/apiextensions.go
- pkg/cli/admin/inspect/admission_webhooks.go
- pkg/cli/admin/inspect/secret.go
- pkg/cli/admin/inspect/writer.go
- pkg/cli/admin/inspect/proxy.go
- pkg/cli/admin/inspect/route.go
- pkg/cli/admin/inspect/namespace.go
🔇 Additional comments (1)
pkg/cli/admin/inspect/pod.go (1)
20-20: Context propagation LGTMSignatures now accept ctx and propagate it through IO/requests. No concerns.
Also applies to: 53-53, 61-61, 102-102, 197-197
|
@tchap: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@tchap: Jira Issue OCPBUGS-59311: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-59311 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload. 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 openshift-eng/jira-lifecycle-plugin repository. |
3 participants
context.Contextis now being passed around to support interrupting the whole inspection process.RunContextmethod is the entry point.There is no functional change.
Split from #2062