NETOBSERV-2133 CLI flows JSON by memodi · Pull Request #201 · netobserv/netobserv-cli

memodi · 2025-02-28T19:45:15Z

Description

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

memodi · 2025-02-28T19:45:31Z

/ok-to-test

github-actions · 2025-02-28T19:47:17Z

New image:
quay.io/netobserv/network-observability-cli:bb25ca5

It will expire after two weeks.

To use this build, update your commands using:

USER=netobserv VERSION=bb25ca5 make commands

or download the updated commands.

memodi · 2025-02-28T19:51:44Z

or download the updated commands.

I tested using this script, we're now getting valid json:

$ jq '.[0]' < output/flow/2025-02-28T194919Z.json
{
  "AgentIP": "10.0.48.141",
  "Bytes": 1893,
  "Dscp": 0,
  "DstAddr": "10.0.48.141",
  "DstK8S_HostIP": "10.0.48.141",
  "DstK8S_HostName": "ip-10-0-48-141.us-east-2.compute.internal",
  "DstK8S_Name": "ip-10-0-48-141.us-east-2.compute.internal",
  "DstK8S_NetworkName": "primary",
  "DstK8S_OwnerName": "ip-10-0-48-141.us-east-2.compute.internal",
  "DstK8S_OwnerType": "Node",
  "DstK8S_Type": "Node",
  "DstK8S_Zone": "us-east-2b",
  "DstMac": "06:A4:C7:67:22:B9",
  "DstPort": 6081,
  "Etype": 2048,
  "FlowDirection": 0,
  "IfDirections": [
    0,
    0
  ],
  "Interfaces": [
    "ens5",
    "br-ex"
  ],
  "Packets": 3,
  "Proto": 17,
  "SrcAddr": "10.0.25.233",
  "SrcK8S_HostIP": "10.0.25.233",
  "SrcK8S_HostName": "ip-10-0-25-233.us-east-2.compute.internal",
  "SrcK8S_Name": "ip-10-0-25-233.us-east-2.compute.internal",
  "SrcK8S_NetworkName": "primary",
  "SrcK8S_OwnerName": "ip-10-0-25-233.us-east-2.compute.internal",
  "SrcK8S_OwnerType": "Node",
  "SrcK8S_Type": "Node",
  "SrcK8S_Zone": "us-east-2a",
  "SrcMac": "06:78:B8:81:70:CF",
  "SrcPort": 57984,
  "TimeFlowEndMs": 1740772152596,
  "TimeFlowStartMs": 1740772152595,
  "TimeReceived": 1740772153,
  "Udns": [
    "",
    ""
  ]
}

scripts/functions.sh

jpinsonneau · 2025-03-03T08:48:25Z

scripts/functions.sh

  echo "Copying collector output files..."
  mkdir -p ./output
  ${K8S_CLI_BIN} cp -n "$namespace" collector:output ./output
+  flowFile=$(find ./output -name "*json" | sort | tail -1)


copyOutput is called for both flows and packets. Usually you can rely on $command to know which command is running but the user can also run oc netobserv copy and we don't know what been copied in that case since it was a background run.

We should refactor the collector to output a .txt file instead, which will be parsed by the script and converted to a .json file properly formatted. Doing so, we will be able to skip the format step when the text file is not found.

WDYT ?

FYI, to pretty print the output once the json is correct, we can use the following command:

$ yq --inplace -p=json -o=json '.' ./output/flow/<filename>.json

I'm relying on yq instead of jq here as we have it as dependency when we run the script

using inplace to edit the same file

mentionning -p=json means we are parsing a json file and -o=json to output a json file

copyOutput is called for both flows and packets. Usually you can rely on $command to know which command is running but the user can also run oc netobserv copy and we don't know what been copied in that case since it was a background run.

I don't see$command variable set in functions.sh, I see we're checking for numbered arguments for $1 or $3 , any reason why arg numbers are not consistent?

We should refactor the collector to output a .txt file instead, which will be parsed by the script and converted to a .json file properly formatted. Doing so, we will be able to skip the format step when the text file is not found.

yes, that makes sense. I'll look into this.

$command is inside netobserv main script: https://github.com/netobserv/network-observability-cli/blob/765cca1e9400b2502bfd847be95a754f097534b0/commands/netobserv#L14

$1 / $3 is relative to the fonction called. So it differs indeed !

Let's focus on the txt / json files which should be good enough 👍

codecov · 2025-03-03T16:00:58Z

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 23.70%. Comparing base (51e6676) to head (718c95a).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/flow_capture.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #201   +/-   ##
=======================================
  Coverage   23.70%   23.70%           
=======================================
  Files          11       11           
  Lines        1333     1333           
=======================================
  Hits          316      316           
  Misses       1000     1000           
  Partials       17       17

Flag	Coverage Δ
unittests	`23.70% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cmd/flow_capture.go	`31.78% <0.00%> (ø)`

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

memodi · 2025-03-03T17:45:06Z

/ok-to-test

github-actions · 2025-03-03T17:47:10Z

New image:
quay.io/netobserv/network-observability-cli:0be6d3a

It will expire after two weeks.

To use this build, update your commands using:

USER=netobserv VERSION=0be6d3a make commands

or download the updated commands.

github-actions · 2025-03-03T18:01:22Z

New image:
quay.io/netobserv/network-observability-cli:5df14c7

It will expire after two weeks.

To use this build, update your commands using:

USER=netobserv VERSION=5df14c7 make commands

or download the updated commands.

github-actions · 2025-03-03T18:21:01Z

New image:
quay.io/netobserv/network-observability-cli:4ec33dc

It will expire after two weeks.

To use this build, update your commands using:

USER=netobserv VERSION=4ec33dc make commands

or download the updated commands.

memodi · 2025-03-03T18:28:25Z

/ok-to-test

github-actions · 2025-03-03T18:30:18Z

New image:
quay.io/netobserv/network-observability-cli:178ab12

It will expire after two weeks.

To use this build, update your commands using:

USER=netobserv VERSION=178ab12 make commands

or download the updated commands.

jpinsonneau · 2025-03-04T09:14:12Z

scripts/functions.sh

+  # output dir may already include files from previous runs
+  oldFlowFile=$(find ./output -name "*txt" | sort | tail -1)
  ${K8S_CLI_BIN} cp -n "$namespace" collector:output ./output
+  flowFile=$(find ./output -name "*txt" | sort | tail -1)
+  if [[ -n "$flowFile" && "$oldFlowFile" != "$flowFile" ]] ; then


We can assume the latest txt file is the new one since we delete it after the conversion
or even apply the json format to all the txt files found under the output/flow folder 😸

or even apply the json format to all the txt files found under the output/flow folder 😸

I think that would be bit overkill.

We can assume the latest txt file is the new one since we delete it after the conversion

I can delete L300 and remove condition to compare.

jpinsonneau · 2025-03-04T09:22:54Z

scripts/functions.sh

+  TMP_FILE="/$MANIFEST_OUTPUT_PATH/$filename"
+  cp "$file" "$TMP_FILE"
+  filenamePrefix=$(echo "$filename" | sed -E 's/(.*)\..*/\1/')
+  UPDATED_JSON_FILE="/$MANIFEST_OUTPUT_PATH/$filenamePrefix.json"
+  { 
+    echo "["
+    # remove last line and "," (last character) of the last flowlog for valid json
+    sed '$d' "$TMP_FILE" | sed '$ s/.$//'
+    echo "]"
+  } >> "$UPDATED_JSON_FILE"
+  dirpath=$(dirname "$file")
+  mv "$UPDATED_JSON_FILE" "$dirpath"
+  rm "$TMP_FILE"


Now that we have a txt file as input and a json file as output, we don't need to rely on a TMP_FILE anymore.
The json file can be written in the final directory as it's done all at once using the brackets + redirect

We sould be able to simplify all of this by:

Suggested change

TMP_FILE="/$MANIFEST_OUTPUT_PATH/$filename"

cp "$file" "$TMP_FILE"

filenamePrefix=$(echo "$filename" | sed -E 's/(.*)\..*/\1/')

UPDATED_JSON_FILE="/$MANIFEST_OUTPUT_PATH/$filenamePrefix.json"

{

echo "["

# remove last line and "," (last character) of the last flowlog for valid json

sed '$d' "$TMP_FILE" | sed '$ s/.$//'

echo "]"

} >> "$UPDATED_JSON_FILE"

dirpath=$(dirname "$file")

mv "$UPDATED_JSON_FILE" "$dirpath"

rm "$TMP_FILE"

dirpath=$(dirname "$file")

filenamePrefix=$(echo "$filename" | sed -E 's/(.*)\..*/\1/')

{

echo "["

# remove last line and "," (last character) of the last flowlog for valid json

sed '$d' "$file" | sed '$ s/.$//'

echo "]"

} >> "$dirpath/$filenamePrefix.json"

What about the format suggested in #201 (comment) ?

I just tried with yq, it takes about 30 seconds to return:

$ time yq --inplace -p=json -o=json '.' /tmp/2025-03-03T180301Z1.json yq --inplace -p=json -o=json '.' /tmp/2025-03-03T180301Z1.json 34.34s user 2.23s system 165% cpu 22.116 total

vs jq:

$ time jq empty < /tmp/2025-03-03T180301Z1.json jq empty < /tmp/2025-03-03T180301Z1.json 2.11s user 0.25s system 97% cpu 2.421 total

I am hesitant to use yq way to validate json given the high processing time, we can probably rely on jq since it's widely prevalent in usage than yq . If jq is absent we can simply skip validation and assume it'd be valid json, wdyt?

Interesting 👀 Indeed yq seems to be 3 times slower than jq on my machine.

I'm not in favor to set jq as dependency and we may have issues depending on the jq version installed on the machine so I would be in favor to simply remove the format part to keep things simple.

The user can still run the format by himself

cc @msherif1234 thoughts on that ?

@jpinsonneau I am okay to remove that part where we test format. We remove the txt file regardless, correct?

yeah keeping only the json non formated is good enough I feel

memodi · 2025-03-13T14:17:30Z

@jpinsonneau are we good here? let me know if there's anything more to address. thanks!

jpinsonneau

Looks good to me ! Thank you sir !

memodi · 2025-03-13T15:10:05Z

Looks good to me ! Thank you sir !

thanks, I think it needs approve label.

jpinsonneau · 2025-03-13T15:31:48Z

Don't we wait for the QE review ? 😆

openshift-ci · 2025-03-13T15:32:05Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

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

memodi · 2025-03-13T15:35:32Z

Don't we wait for the QE review ? 😆

oh, of course, I shouldn't have thought of my testing as QE testing for this one and probably should have asked @Amoghrd to review :D but on a serious note, would you mind giving a quick try if you have not already?

Amoghrd · 2025-03-13T15:36:54Z

/ok-to-test

github-actions · 2025-03-13T15:38:51Z

New image:
quay.io/netobserv/network-observability-cli:27bc89d

It will expire after two weeks.

To use this build, update your commands using:

USER=netobserv VERSION=27bc89d make commands

or download the updated commands.

Amoghrd · 2025-03-13T15:53:27Z

/label qe-approved

memodi · 2025-03-13T17:05:12Z

thanks @Amoghrd

/retest

jpinsonneau · 2025-03-14T09:12:27Z

/retest

jpinsonneau · 2025-03-14T09:13:24Z

Don't worry I tested it too before approving 😄
Thanks @memodi & @Amoghrd !

memodi requested a review from jpinsonneau February 28, 2025 19:45

openshift-ci bot added the ok-to-test label Feb 28, 2025

jpinsonneau reviewed Mar 3, 2025

View reviewed changes

scripts/functions.sh Outdated Show resolved Hide resolved

jpinsonneau reviewed Mar 3, 2025

View reviewed changes

github-actions bot removed the ok-to-test label Mar 3, 2025

openshift-ci bot added the ok-to-test label Mar 3, 2025

github-actions bot removed the ok-to-test label Mar 3, 2025

memodi force-pushed the 2133 branch from 70b7b42 to abe1f2f Compare March 3, 2025 18:28

openshift-ci bot added the ok-to-test label Mar 3, 2025

memodi requested a review from jpinsonneau March 3, 2025 20:09

jpinsonneau reviewed Mar 4, 2025

View reviewed changes

github-actions bot removed the ok-to-test label Mar 4, 2025

memodi force-pushed the 2133 branch 2 times, most recently from e5b2516 to 6383b30 Compare March 4, 2025 16:40

memodi mentioned this pull request Mar 6, 2025

NETOBSERV-2113 CLI Integration tests #212

Merged

10 tasks

write file in correct JSON format

718c95a

memodi force-pushed the 2133 branch from c6f031f to 718c95a Compare March 7, 2025 18:08

jpinsonneau approved these changes Mar 13, 2025

View reviewed changes

openshift-ci bot assigned jpinsonneau Mar 13, 2025

openshift-ci bot added the lgtm label Mar 13, 2025

jpinsonneau added the approved label Mar 13, 2025

openshift-ci bot added the ok-to-test label Mar 13, 2025

openshift-ci bot added the qe-approved label Mar 13, 2025

openshift-merge-bot bot merged commit cb62cac into netobserv:main Mar 14, 2025
13 checks passed

Conversation

memodi commented Feb 28, 2025

Description

Dependencies

Checklist

Uh oh!

memodi commented Feb 28, 2025

Uh oh!

github-actions bot commented Feb 28, 2025

Uh oh!

memodi commented Feb 28, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpinsonneau Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

memodi commented Mar 3, 2025

Uh oh!

github-actions bot commented Mar 3, 2025

Uh oh!

github-actions bot commented Mar 3, 2025

Uh oh!

github-actions bot commented Mar 3, 2025

Uh oh!

memodi commented Mar 3, 2025

Uh oh!

github-actions bot commented Mar 3, 2025

Uh oh!

jpinsonneau Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

memodi commented Mar 13, 2025

Uh oh!

jpinsonneau left a comment

Choose a reason for hiding this comment

Uh oh!

memodi commented Mar 13, 2025

Uh oh!

jpinsonneau commented Mar 13, 2025

Uh oh!

openshift-ci bot commented Mar 13, 2025

Uh oh!

memodi commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amoghrd commented Mar 13, 2025

Uh oh!

github-actions bot commented Mar 13, 2025

Uh oh!

Amoghrd commented Mar 13, 2025

Uh oh!

memodi commented Mar 13, 2025

jpinsonneau Mar 3, 2025 •

edited

Loading

codecov bot commented Mar 3, 2025 •

edited

Loading

jpinsonneau Mar 4, 2025 •

edited

Loading

memodi commented Mar 13, 2025 •

edited

Loading