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

ps: support the container... notation for ps --filter network=... #11075

Merged
merged 1 commit into from
Jul 30, 2021
Merged

ps: support the container... notation for ps --filter network=... #11075

merged 1 commit into from
Jul 30, 2021

Conversation

flouthoc
Copy link
Collaborator

Hi team,

Following PR extends ps --filter to support docker like:
podman container ps --filter network=container:<CtrIdorCtrName>.

Closes: #10361

@rhatdan
Copy link
Member

rhatdan commented Jul 29, 2021

@mheon @Luap99 PTAL

@flouthoc flouthoc force-pushed the ps-filter-network-by-container branch 3 times, most recently from 0a04b7d to cf72cd8 Compare July 29, 2021 11:31
Luap99
Luap99 reviewed Jul 29, 2021
View reviewed changes
libpod/container.go Show resolved Hide resolved
pkg/domain/filters/containers.go Outdated Show resolved Hide resolved
pkg/domain/filters/containers.go Outdated Show resolved Hide resolved
pkg/domain/filters/containers.go Show resolved Hide resolved
test/e2e/ps_test.go Show resolved Hide resolved
@flouthoc flouthoc force-pushed the ps-filter-network-by-container branch 3 times, most recently from 6473860 to 6af6f19 Compare July 29, 2021 14:10
@flouthoc flouthoc mentioned this pull request Jul 29, 2021
Merged
@flouthoc flouthoc requested a review from Luap99 July 29, 2021 15:44
Luap99
Luap99 approved these changes Jul 29, 2021
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 29, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, Luap99

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 Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2021
mheon
mheon reviewed Jul 29, 2021
View reviewed changes
libpod/container.go Outdated
@@ -1173,6 +1173,54 @@ func (c *Container) Networks() ([]string, bool, error) {
return c.networks()
}

// NetworkMode gets the configured network mode for the container.
// Get actual value from the database
func (c *Container) NetworkMode(ctrSpec *spec.Spec) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this accept a spec? The container struct already contains a spec, and nothing you're accessing here requires something that would be overwritten from the initial user-provided spec. Just using c.config.Spec is simpler and safer.

Copy link
Collaborator Author

@flouthoc flouthoc Jul 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@mheon Following function is also going to be used by generateInspectContainerHostConfig in inspect code chunk. I think for both inspect and ps --filter their exists a state where config does not exist (e.g., because the container was never started) for such cases I think we can read data from db.

However for inspect block ctrSpec is already fetched from state so i thought would be more optimal to reuse the ctrSpec as argument inoder to reduce lookup cost.

Please correct me if i am getting it wrong and i am also fine with using c.config.Spec

Copy link
Member

Choose a reason for hiding this comment

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

c.config.Spec is always valid; there is never a point at which it is not populated. It is not the final spec that is used for the container (we use it to generate the final spec), but it is very close - the changes made to produce a final spec are minimal. As such it is safe to use here - no part of what you are accessing will change during final spec generation.

@flouthoc flouthoc force-pushed the ps-filter-network-by-container branch from 6af6f19 to cb2ab7d Compare July 29, 2021 19:43
@flouthoc
Copy link
Collaborator Author

@mheon I have resolved all your feedback comments in latest commit. I have a doubt on first feedback , could you please take a look and correct me if i am wrong somewhere.

@flouthoc flouthoc force-pushed the ps-filter-network-by-container branch 3 times, most recently from 7b9054e to 1212996 Compare July 29, 2021 20:16
@flouthoc
Copy link
Collaborator Author

Re-basing with main to add fix for #11077

@flouthoc flouthoc requested a review from mheon July 29, 2021 20:25
@TomSweeneyRedHat
Copy link
Member

LGTM
and happy green test buttons
I'll let @mheon press the final merge button as I'm not certain all of his concerns were addressed.

@flouthoc flouthoc force-pushed the ps-filter-network-by-container branch from 1212996 to 980fef7 Compare July 30, 2021 03:56
@flouthoc
Copy link
Collaborator Author

@TomSweeneyRedHat thanks :) there is one change left which @mheon suggested. @mheon made relevant changes will only try to read spec from db if c.config.Spec is nil for added safety does it looks fine to you ?

mheon
mheon reviewed Jul 30, 2021
View reviewed changes
libpod/container.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the ps-filter-network-by-container branch from 980fef7 to fccacbd Compare July 30, 2021 13:46
@flouthoc flouthoc requested a review from mheon July 30, 2021 13:47
@flouthoc
Copy link
Collaborator Author

Just a sec cleaning error handling for removed block.

@flouthoc flouthoc force-pushed the ps-filter-network-by-container branch from fccacbd to 940196f Compare July 30, 2021 13:53
@flouthoc flouthoc force-pushed the ps-filter-network-by-container branch from 940196f to 2a484e7 Compare July 30, 2021 14:01
@flouthoc
Copy link
Collaborator Author

@mheon This is resolved could you please review again.

@mheon
Copy link
Member

mheon commented Jul 30, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2021
@openshift-ci openshift-ci bot merged commit aaf02cf into containers:main Jul 30, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 approved these changes

@mheon mheon Awaiting requested review from mheon

Assignees

@mheon mheon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support the container:... notation for ps --filter network=...
5 participants
@flouthoc @rhatdan @TomSweeneyRedHat @mheon @Luap99