allow filter networks by dangling status #14643

clobrano · 2022-06-17T15:20:47Z

add the ability to filter networks by their dangling status via:

network ls --filter dangling=true/false

Does this PR introduce a user-facing change?

Added support for `network ls --filter dangling=true/false`

rhatdan · 2022-06-17T15:51:42Z

Lint is not happy.

pkg/domain/infra/abi/network.go

rhatdan · 2022-06-17T15:57:04Z

pkg/domain/infra/abi/network.go

@@ -144,6 +172,33 @@ func (ic *ContainerEngine) NetworkExists(ctx context.Context, networkname string

 // Network prune removes unused cni networks
 func (ic *ContainerEngine) NetworkPrune(ctx context.Context, options entities.NetworkPruneOptions) ([]*entities.NetworkPruneReport, error) {
+	// get all filters
+	filters, err := netutil.GenerateNetworkPruneFilters(options.Filters)


Why isn't dangling filters done in the GenerateNetworkPruneFilters function?

my understanding is that GenerateNetworkPruneFilters does not know (or has reasons to know) the list of all containers, which is needed by this feature.

GenerateNetworkPruneFilters Generates a list of filters These filters can return your danglinFilterFunc?

And I could see where I want to Prune all Networks without containers.

GenerateNetworkPruneFilters comes from another repository, do you think it would be better to push this change there? However, I am not sure how to let common/libnetwork use the podman's ContainerEngine struct, then.

@Luap99 Thoughts?

I think it is cleaner to have it in podman. Adding to c/common would require ugly callback function just to get the container networks.

rhatdan · 2022-06-17T15:58:07Z

Doesn't this need a man page update?

clobrano · 2022-06-17T16:23:32Z

Doesn't this need a man page update?

yes indeed, I did not think about it

docs/source/markdown/podman-network-ls.1.md

clobrano · 2022-06-18T09:47:47Z

I noticed a test is failing on the API. I will look into it

pkg/domain/infra/abi/network.go

clobrano · 2022-06-18T12:37:59Z

I noticed a test is failing on the API. I will look into it

A test in test/apiv2/35-networks.at is expecting an error since "dangling" was considered an invalid filter, while now it is supported.

ok 763 [35-networks] GET networks?filters={"id":["b385480e1e5da54a3cf0f17cd44e5a193ea3ab2757a1c28723f495a4edfe4cf8"]} : .[0].Id=b385480e1e5da54a3cf0f17cd44e5a193ea3ab2757a1c28723f495a4edfe4cf8
         not ok 764 [35-networks] GET networks?filters={"dangling":["1"]} : status
         #  expected: 500
         #    actual: 200

Is it ok if I change the expected result of this test?

clobrano · 2022-06-21T14:36:46Z

There are a couple of test still failing, but I don't think it's due to this changes. Let me know if there is anything from my side I can do.

Luap99 · 2022-06-21T14:41:59Z

pkg/domain/infra/abi/network.go

+	if filterDangling {
+		var err error
+
+		wantDangling, err = strconv.ParseBool(val[0])


This can panic. You have to loop over all values here.
While unlikely it is possible that more than one dangling value is set:
'{"dangling":["1","0"]}' or '{"dangling":[]}'

yes, indeed, I will fix this

however, being "dangling" a binary filter, I wonder if a good user experience would be to just error out if more than one value is set (or none).
Also to avoid to decide what to do when two valid, but opposite values are passed (like in your first example)

Well what does docker do? AFAIK if both true and false are set it should just return all.

good point, I just checked

❯ docker network ls --filter dangling=true --filter dangling=false Error response from daemon: got more than one value for filter key "dangling"

Luap99 · 2022-06-21T14:42:38Z

pkg/domain/infra/abi/network.go

+	if filterDangling {
+		danglingFilterFunc, err := ic.createDanglingFilterFunc(wantDangling)
+		if err != nil {
+			return make([]types.Network, 0), err


Suggested change

return make([]types.Network, 0), err

return nil, err

test/apiv2/35-networks.at

Luap99 · 2022-06-21T14:45:37Z

There are a couple of test still failing, but I don't think it's due to this changes. Let me know if there is anything from my side I can do.

They do not look related to your change. so likely flakes. We can restart the tests manually.

add the ability to filter networks by their dangling status via: `network ls --filter dangling=true/false` Fixes: containers#14595 Signed-off-by: Carlo Lobrano <[email protected]>

mheon · 2022-06-21T17:35:00Z

LGTM

One test still running, even though Github shows everything as green except Total Success

baude · 2022-06-21T18:05:06Z

/approve

openshift-ci · 2022-06-21T18:05:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, clobrano

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:

~~OWNERS~~ [baude]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

baude · 2022-06-21T18:06:07Z

is this ready for merge?

rhatdan · 2022-06-21T18:12:49Z

/lgtm

openshift-ci bot added the release-note label Jun 17, 2022

clobrano force-pushed the feature/network/list/dangling/dev branch from 9341185 to 4b12339 Compare June 17, 2022 15:20

rhatdan reviewed Jun 17, 2022

View reviewed changes

pkg/domain/infra/abi/network.go Outdated Show resolved Hide resolved

rhatdan reviewed Jun 17, 2022

View reviewed changes

clobrano force-pushed the feature/network/list/dangling/dev branch from 4b12339 to 9bf8343 Compare June 17, 2022 16:22

clobrano force-pushed the feature/network/list/dangling/dev branch from 9bf8343 to cff7dad Compare June 17, 2022 16:27

rhatdan reviewed Jun 17, 2022

View reviewed changes

docs/source/markdown/podman-network-ls.1.md Show resolved Hide resolved

clobrano force-pushed the feature/network/list/dangling/dev branch from cff7dad to c6e0fca Compare June 17, 2022 17:33

Luap99 reviewed Jun 18, 2022

View reviewed changes

pkg/domain/infra/abi/network.go Outdated Show resolved Hide resolved

pkg/domain/infra/abi/network.go Show resolved Hide resolved

clobrano force-pushed the feature/network/list/dangling/dev branch from c6e0fca to 09f7c1d Compare June 18, 2022 12:08

clobrano force-pushed the feature/network/list/dangling/dev branch 2 times, most recently from 8ea4a1d to 3525681 Compare June 20, 2022 18:41

Luap99 reviewed Jun 21, 2022

View reviewed changes

allow filter networks by dangling status

4a981c4

add the ability to filter networks by their dangling status via: `network ls --filter dangling=true/false` Fixes: containers#14595 Signed-off-by: Carlo Lobrano <[email protected]>

clobrano force-pushed the feature/network/list/dangling/dev branch from 3525681 to 4a981c4 Compare June 21, 2022 15:51

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2022

openshift-ci bot assigned rhatdan Jun 21, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2022

openshift-ci bot merged commit 8863e13 into containers:main Jun 21, 2022

clobrano deleted the feature/network/list/dangling/dev branch June 21, 2022 18:29

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 21, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow filter networks by dangling status #14643

allow filter networks by dangling status #14643

clobrano commented Jun 17, 2022

rhatdan commented Jun 17, 2022

rhatdan Jun 17, 2022

clobrano Jun 17, 2022

rhatdan Jun 17, 2022

rhatdan Jun 17, 2022

clobrano Jun 17, 2022

rhatdan Jun 17, 2022

Luap99 Jun 18, 2022

rhatdan commented Jun 17, 2022

clobrano commented Jun 17, 2022

clobrano commented Jun 18, 2022

clobrano commented Jun 18, 2022 •

edited

Loading

clobrano commented Jun 21, 2022

Luap99 Jun 21, 2022

clobrano Jun 21, 2022

clobrano Jun 21, 2022

Luap99 Jun 21, 2022

clobrano Jun 21, 2022

Luap99 Jun 21, 2022

Luap99 commented Jun 21, 2022

mheon commented Jun 21, 2022

baude commented Jun 21, 2022

openshift-ci bot commented Jun 21, 2022

baude commented Jun 21, 2022

rhatdan commented Jun 21, 2022

allow filter networks by dangling status #14643

allow filter networks by dangling status #14643

Conversation

clobrano commented Jun 17, 2022

Does this PR introduce a user-facing change?

rhatdan commented Jun 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Jun 17, 2022

clobrano commented Jun 17, 2022

clobrano commented Jun 18, 2022

clobrano commented Jun 18, 2022 • edited Loading

clobrano commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 commented Jun 21, 2022

mheon commented Jun 21, 2022

baude commented Jun 21, 2022

openshift-ci bot commented Jun 21, 2022

baude commented Jun 21, 2022

rhatdan commented Jun 21, 2022

clobrano commented Jun 18, 2022 •

edited

Loading