Add list and delete commands in policy recommendation CLI #56

dreamtalen · 2022-06-21T20:43:48Z

Fix #49

Signed-off-by: Yongming Ding dyongming@vmware.com

docs/networkpolicy-recommendation.md

pkg/theia/commands/policy_recommendation_delete.go

pkg/theia/commands/policy_recommendation_list.go

heanlan

LGTM

test/e2e/policyrecommendation_test.go

pkg/theia/commands/policy_recommendation_delete.go

pkg/theia/commands/policy_recommendation_list.go

yanjunz97 · 2022-06-24T19:03:11Z

pkg/theia/commands/policy_recommendation_list.go

+	policyRecommendationListCmd.Flags().Bool(
+		"use-cluster-ip",
+		false,
+		`Enable this option will use Service ClusterIP instead of port forwarding when connecting to the ClickHouse service.
+It can only be used when running theia in cluster.`,
+	)


Not sure whether it would be better to put this into global flags, as I think up to now all commands requires this flag.

I think both clickhouse-endpoint and use-cluster-ip might be added to glabal flags?

Hi Yanjun and Yuntang, yes there are some duplicates here, but status command uses use-cluster-ip only and is used for Spark Monitoring Service instead of ClickHouse Service in other commands.
Ultimately, I think the connection to Spark Monitoring Service and ClickHouse Service should be handled by the middle layer app theia manager. We could remove these flags then imo.

I think for ClickHouse service, we also need these two flags in order to setup clickhouse connection? Currently, I add these two flags when using theia clickhouse status in PR #59

For our theia pr status command, we're fetching status from k8s api server and spark monitoring service, that's why it doesn't need to connect with clickhouse for now.

But I also realize that we could improve the status command to let it can check completed jobs inside clickhouse only. In that case, all commands needs connection with clickhouse, I plan to create a separate PR for this improvement of status command first.

yanjunz97 · 2022-06-24T19:45:17Z

docs/networkpolicy-recommendation.md

+The `theia policy-recommendation delete` command is used to delete a policy
+recommendation job. To delete the policy recommendation job created above, run:


I notice there is a rare case that if users try to delete a job before the driver pod is created, the job may still keep running till completion. And users will receive a message saying the job is successfully deleted. Not sure if we should deal with this case by returning another message or doc it here.

Thanks Yanjun, I have seen this corner case too during e2e test. In that case, the sparkapplication cr has been deleted successfully but the deriver/exec pods will still be created later, maybe I should create an issue in the spark operator repo to hear their solutions.

yanjunz97 · 2022-06-24T20:06:21Z

Not introduced by this PR, I just recall previously there is a discussion on replacing the empty state string when we get a NewState by running theia pr status xxx. Maybe we can fix it here or in a new PR?

dreamtalen · 2022-06-24T20:50:30Z

Not introduced by this PR, I just recall previously there is a discussion on replacing the empty state string when we get a NewState by running theia pr status xxx. Maybe we can fix it here or in a new PR?

No problem, I will take care of it.

pkg/theia/commands/utils.go

Signed-off-by: Yongming Ding <dyongming@vmware.com>

docs/networkpolicy-recommendation.md

ziyouw

LGTM, thanks!

dreamtalen · 2022-07-12T17:29:35Z

/theia-test-e2e

salv-orlando · 2022-07-14T15:38:56Z

docs/networkpolicy-recommendation.md

@@ -36,17 +38,21 @@ CLI. `theia` is the command-line tool which provides access to Theia network
 flow visibility capabilities. To get more information about `theia`, please
 refer to its [user guide](theia-cli.md).

-There are 3 `theia` commands for the NetworkPolicy Recommendation feature:
+There are 5 `theia` commands for the NetworkPolicy Recommendation feature:


Perhaps it's easier if we stop numbering them...
Something like
The following theia commands for .... are available

salv-orlando · 2022-07-14T15:41:18Z

pkg/theia/commands/policy_recommendation_delete.go

+		}
+		_, err = uuid.Parse(recoID)
+		if err != nil {
+			return fmt.Errorf("failed to decode input id %s into a UUID, err: %v", recoID, err)


Consider: "Input id ... does not seem a valid uuid, parsing error: ..."

salv-orlando · 2022-07-14T15:41:49Z

pkg/theia/commands/policy_recommendation_delete.go

+		if endpoint != "" {
+			_, err := url.ParseRequestURI(endpoint)
+			if err != nil {
+				return fmt.Errorf("failed to decode input endpoint %s into a url, err: %v", endpoint, err)


(as above)
Consider: "Input endpoint ... does not seem a valid URL, parsing error: ...."

salv-orlando · 2022-07-14T15:50:01Z

pkg/theia/commands/policy_recommendation_list.go

+		if endpoint != "" {
+			_, err := url.ParseRequestURI(endpoint)
+			if err != nil {
+				return fmt.Errorf("failed to decode input endpoint %s into a url, err: %v", endpoint, err)


same comment as above about endpoint parsing (not sure if we can have some utility functions to avoid code duplication for parsing parameters which are used by multiple commands)

Sure, add two utility functions for parsing ID and endpoint.

salv-orlando · 2022-07-14T15:50:52Z

pkg/theia/commands/policy_recommendation_list.go

+		}
+
+		sparkApplicationTable := [][]string{
+			{"CreateTime", "CompleteTime", "ID", "Status"},


consider using "CompletionTime" instead of "CompleteTime"

Sure, to be consistent, I'm changing to use CreationTime and CompletionTime.

salv-orlando · 2022-07-14T15:51:53Z

pkg/theia/commands/policy_recommendation_delete.go

+	if err != nil {
+		return err
+	}
+	query := "ALTER TABLE recommendations DELETE WHERE id = (?);"


is it a clickhouse peculiarity that we need to use ALTER TABLE for deleting records?
Does DELETE FROM
WHERE not work for Clickhouse?

Yes, Clickhouse doesn't have Update/Delete commands like Mysql database, reference doc: https://clickhouse.com/docs/en/sql-reference/statements/alter/delete/

Signed-off-by: Yongming Ding <dyongming@vmware.com>

dreamtalen · 2022-07-27T20:46:12Z

/theia-test-e2e

dreamtalen force-pushed the pr-ls-del branch from d962c6c to 7dff991 Compare June 21, 2022 22:33

dreamtalen added this to the 0.2 milestone Jun 21, 2022

annakhm reviewed Jun 21, 2022

View reviewed changes

docs/networkpolicy-recommendation.md Outdated Show resolved Hide resolved

annakhm reviewed Jun 21, 2022

View reviewed changes

pkg/theia/commands/policy_recommendation_delete.go Outdated Show resolved Hide resolved

annakhm reviewed Jun 21, 2022

View reviewed changes

pkg/theia/commands/policy_recommendation_list.go Outdated Show resolved Hide resolved

dreamtalen requested review from heanlan, yanjunz97 and salv-orlando June 24, 2022 17:24

heanlan reviewed Jun 24, 2022

View reviewed changes

test/e2e/policyrecommendation_test.go Outdated Show resolved Hide resolved

yanjunz97 reviewed Jun 24, 2022

View reviewed changes

yuntanghsu reviewed Jun 24, 2022

View reviewed changes

pkg/theia/commands/utils.go Outdated Show resolved Hide resolved

dreamtalen force-pushed the pr-ls-del branch 3 times, most recently from a4220e6 to 9fe60a8 Compare June 27, 2022 20:16

Add list and delete commands in policy recommendation CLI

f55ec63

Signed-off-by: Yongming Ding <dyongming@vmware.com>

dreamtalen force-pushed the pr-ls-del branch 2 times, most recently from 11671a6 to 5bb4129 Compare June 29, 2022 17:01

ziyouw reviewed Jul 4, 2022

View reviewed changes

docs/networkpolicy-recommendation.md Outdated Show resolved Hide resolved

docs/networkpolicy-recommendation.md Outdated Show resolved Hide resolved

dreamtalen force-pushed the pr-ls-del branch from 5bb4129 to cf1bc63 Compare July 5, 2022 17:30

ziyouw approved these changes Jul 12, 2022

View reviewed changes

salv-orlando reviewed Jul 14, 2022

View reviewed changes

Address comments

4d2f21e

Signed-off-by: Yongming Ding <dyongming@vmware.com>

dreamtalen force-pushed the pr-ls-del branch from cf1bc63 to 4d2f21e Compare July 14, 2022 20:39

heanlan approved these changes Jul 27, 2022

View reviewed changes

dreamtalen merged commit 0e0c7c6 into antrea-io:main Jul 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add list and delete commands in policy recommendation CLI #56

Add list and delete commands in policy recommendation CLI #56

dreamtalen commented Jun 21, 2022

heanlan left a comment

yanjunz97 Jun 24, 2022

yuntanghsu Jun 24, 2022

dreamtalen Jun 24, 2022

yuntanghsu Jun 24, 2022

dreamtalen Jun 24, 2022 •

edited

Loading

yanjunz97 Jun 24, 2022

dreamtalen Jun 24, 2022

yanjunz97 commented Jun 24, 2022

dreamtalen commented Jun 24, 2022

ziyouw left a comment

dreamtalen commented Jul 12, 2022

salv-orlando Jul 14, 2022

salv-orlando Jul 14, 2022

salv-orlando Jul 14, 2022

salv-orlando Jul 14, 2022

dreamtalen Jul 14, 2022

salv-orlando Jul 14, 2022

dreamtalen Jul 14, 2022

salv-orlando Jul 14, 2022

dreamtalen Jul 14, 2022

dreamtalen commented Jul 27, 2022

		The `theia policy-recommendation delete` command is used to delete a policy
		recommendation job. To delete the policy recommendation job created above, run:

Add list and delete commands in policy recommendation CLI #56

Add list and delete commands in policy recommendation CLI #56

Conversation

dreamtalen commented Jun 21, 2022

heanlan left a 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

dreamtalen Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanjunz97 commented Jun 24, 2022

dreamtalen commented Jun 24, 2022

ziyouw left a comment

Choose a reason for hiding this comment

dreamtalen commented Jul 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamtalen commented Jul 27, 2022

dreamtalen Jun 24, 2022 •

edited

Loading