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

Add a flag for antctl to print OVS table names only #5895

Merged
merged 1 commit into from
Feb 21, 2024
Merged

Add a flag for antctl to print OVS table names only #5895

merged 1 commit into from
Feb 21, 2024

Conversation

luolanzone
Copy link
Contributor

@luolanzone luolanzone commented Jan 19, 2024 •
edited

Add a new flag to print OVS table names only. Fixes #5810

./antctl get ovsflows --table-names-only

ARPResponder
ARPSpoofGuard
AntreaPolicyEgressRule
AntreaPolicyIngressRule
Classifier
ConntrackCommit
ConntrackState
ConntrackZone
EgressDefaultRule
EgressMark
EgressMetric
EgressRule
EndpointDNAT
IngressDefaultRule
IngressMetric
IngressRule
IngressSecurityClassifier
L2ForwardingCalc
L3DecTTL
L3Forwarding
Output
PipelineRootClassifier
PreRoutingClassifier
SNAT
SNATMark
ServiceLB
SessionAffinity
SpoofGuard
UnSNAT

antoninbas
antoninbas reviewed Jan 19, 2024
View reviewed changes
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

should reference issue #5810

pkg/agent/apiserver/handlers/ovsflows/handler.go Outdated
@@ -194,6 +203,20 @@ func HandleFunc(aq agentquerier.AgentQuerier) http.HandlerFunc {
namespace := r.URL.Query().Get("namespace")
table := r.URL.Query().Get("table")
groups := r.URL.Query().Get("groups")
tableNamesOnly := r.URL.Query().Get("table-names")
Copy link
Contributor

Choose a reason for hiding this comment

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

intuitively, maybe a subresource (/ovsflows/tablenames) would make more sense to me
but that may be a better change for not much value
so maybe /ovsflows?table-names-only instead? There is no need for a value IMO (=true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I was thinking table-names is shorter to type. I will remove the true value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, In current antctl command framework, it doesn't support boolean type of flags. All flags' arguments will be saved into argMap which is a type of "map[string]string", so the "true" value is replaced by an empty string.

luolanzone
luolanzone commented Jan 20, 2024
View reviewed changes
pkg/agent/apiserver/handlers/ovsflows/handler.go
}
}

var _ common.TableOutput = new(Response)

func (r Response) GetTableHeader() []string {
return []string{"FLOW"}
return []string{""}
Copy link
Contributor Author

@luolanzone luolanzone Jan 20, 2024 •
edited

Choose a reason for hiding this comment

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

The "FLOW" doesn't provide extra info and it's strange to have this header when printing the table name only, so removed this table header.

pkg/agent/apiserver/handlers/ovsflows/handler.go Outdated
@@ -194,6 +203,20 @@ func HandleFunc(aq agentquerier.AgentQuerier) http.HandlerFunc {
namespace := r.URL.Query().Get("namespace")
table := r.URL.Query().Get("table")
groups := r.URL.Query().Get("groups")
tableNamesOnly := r.URL.Query().Get("table-names")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I was thinking table-names is shorter to type. I will remove the true value.

@luolanzone luolanzone changed the title Add a subcommand for antctl to print OVS table names only Add a flag for antctl to print OVS table names only Jan 22, 2024
antoninbas
antoninbas reviewed Jan 22, 2024
View reviewed changes
pkg/agent/apiserver/handlers/ovsflows/handler.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go
t := obj.(binding.Table)
t := obj.(*Table).ofTable
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like a bug? Was this function (GetTableList) not used by anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think it was a bug, this function is only used in antctl get ovsflows, it probably never worked before.

pkg/antctl/antctl.go Outdated Show resolved Hide resolved
pkg/antctl/antctl.go Outdated Show resolved Hide resolved
pkg/antctl/antctl.go Outdated
{
name: "table-names-only",
usage: "Print all Antrea OVS flow table names only",
shorthand: "b",
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we don't need the shorthand for this flag? I don't see a connection between --table-names-only and -b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

pkg/antctl/antctl.go Outdated Show resolved Hide resolved
pkg/antctl/command_definition.go Outdated Show resolved Hide resolved
pkg/antctl/command_definition.go Outdated Show resolved Hide resolved
@luolanzone
Add a flag for antctl to print OVS table names only
91d080c 
Signed-off-by: Lan Luo <luola@vmware.com>
@antoninbas
Copy link
Contributor

/test-all

@luolanzone
Copy link
Contributor Author

/test-conformance
/test-e2e

1 similar comment
@luolanzone
Copy link
Contributor Author

/test-conformance
/test-e2e

@luolanzone
Copy link
Contributor Author

Hi @antoninbas could you help to check and merge this one. The failed tests were caused by the testbed instead of code. I rerun them and they are passed.

@antoninbas antoninbas merged commit e3f07df into antrea-io:main Feb 21, 2024
47 of 53 checks passed
@antoninbas antoninbas added the action/release-note Indicates a PR that should be included in release notes. label Feb 21, 2024
@luolanzone luolanzone deleted the fix-antctl-print-tables branch March 5, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoninbas antoninbas antoninbas approved these changes

Assignees
No one assigned
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to Run "antctl get ovsflow --help" Command
2 participants
@luolanzone @antoninbas