Update go to v1.20 #299

Dhruv-J · 2023-05-16T23:34:00Z

Updating go to version 1.20. Go v1.20 comes with many updates to coverage, additional functionality for some methods, library changes, as well as a method deprecation.

Fixes issue #297
Fixes Issue #178

yuntanghsu · 2023-05-20T18:04:58Z

I see there are couple of errors. Could you fix them?

codecov · 2023-05-22T01:47:54Z

Codecov Report

Merging #299 (def9508) into main (aaeab26) will increase coverage by 18.13%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main     #299       +/-   ##
===========================================
+ Coverage   53.29%   71.43%   +18.13%     
===========================================
  Files          67       40       -27     
  Lines        7244     5220     -2024     
  Branches       43        0       -43     
===========================================
- Hits         3861     3729      -132     
+ Misses       3203     1322     -1881     
+ Partials      180      169       -11

Flag	Coverage Δ		*Carryforward flag
kind-e2e-tests	`63.85% <100.00%> (?)`
python-coverage	`56.37% <ø> (ø)`		Carriedforward from aaeab26
unit-tests	`70.53% <100.00%> (+18.86%)`	⬆️

*This pull request uses carry forward flags. Click here to find out more.

Files Changed	Coverage Δ
plugins/clickhouse-monitor/main.go	`74.88% <100.00%> (+3.90%)`	⬆️

... and 37 files with indirect coverage changes

antoninbas · 2023-06-21T18:34:21Z

We usually keep the Go version consistent across all antrea-io repos. We also usually skip one Go minor version (1.17 -> 1.19 -> 1.21). We were planning to wait until the Go 1.21 release to update all antrea-io repos. Is there a specific feature that requires updating to Go 1.20 sooner for Theia?

Dhruv-J · 2023-06-21T18:36:44Z

We usually keep the Go version consistent across all antrea-io repos. We also usually skip one Go minor version (1.17 -> 1.19 -> 1.21). We were planning to wait until the Go 1.21 release to update all antrea-io repos. Is there a specific feature that requires updating to Go 1.20 sooner for Theia?

Yeah, we wanted to add coverage without having to modify the workflow again when 1.21 releases. We would also update to go 1.21 along Antrea, but would you recommend holding off on the feature till go v1.21 then?

antoninbas · 2023-06-22T00:32:46Z

You can do the update to 1.20 if it unblocks your work, as long as it's understood that Theia will update to 1.21 when it is released to stay in sync with Antrea (that will be in August). Theia devs will need to update their local Go version twice in a short time, but that's a minor inconvenience.

elton-furtado · 2023-07-21T04:33:48Z

/theia-test-e2e

elton-furtado · 2023-07-21T04:42:53Z

build/charts/theia/README.md

@@ -34,6 +34,7 @@ Kubernetes: `>= 1.16.0-0`
 | clickhouse.monitor.deletePercentage | float | `0.5` | The percentage of records in ClickHouse that will be deleted when the storage grows above threshold. Vary from 0 to 1. |
 | clickhouse.monitor.enable | bool | `true` | Determine whether to run a monitor to periodically check the ClickHouse memory usage and clean data. |
 | clickhouse.monitor.execInterval | string | `"1m"` | The time interval between two round of monitoring. Can be a plain integer using one of these unit suffixes ns, us (or µs), ms, s, m, h. |
+| clickhouse.monitor.gocoverdir | string | `"clickhouse-monitor-coverage"` | coverage directory to be used |


is this required now? shouldn't there be a 'theia-monitor-coverdir'

not required as it's a constant, removing it

elton-furtado · 2023-07-21T04:45:27Z

build/charts/theia/values.yaml

@@ -270,3 +272,4 @@ theiaManager:
    tlsMinVersion: ""
  # -- Log verbosity switch for Theia Manager.
  logVerbosity: 0
+  # -- coverage directory to be used


elton-furtado · 2023-07-21T04:46:35Z

ci/kind/test-e2e-kind.sh

+    mkdir -p .coverage/clickhouse-monitor-coverage/
+    mkdir -p .coverage/theia-manager-coverage/
+    mkdir -p .coverage/merged
+    echo "STARTED_RUNNING"


Started running Kind E2E tests code coverage, same comment below

elton-furtado · 2023-07-21T04:46:44Z

ci/kind/test-e2e-kind.sh

@@ -154,6 +165,7 @@ function run_test {
  sed -i -e "s/idleFlowExportTimeout: \"15s\"/idleFlowExportTimeout: \"1s\"/g" $TMP_DIR/antrea.yml

  curl -o $TMP_DIR/flow-aggregator.yml https://raw.githubusercontent.com/antrea-io/antrea/main/build/yamls/flow-aggregator.yml
+  #cp ~/go/src/github.com/antrea/build/yamls/flow-aggregator.yml $TMP_DIR/flow-aggregator.yml


elton-furtado · 2023-07-21T04:46:49Z

ci/kind/test-e2e-kind.sh

@@ -146,6 +156,7 @@ function setup_cluster {
 function run_test {
  TMP_DIR=$(mktemp -d $(dirname $0)/tmp.XXXXXXXX)
  curl -o $TMP_DIR/antrea.yml https://raw.githubusercontent.com/antrea-io/antrea/main/build/yamls/antrea.yml
+  #cp ~/go/src/github.com/antrea/build/yamls/antrea.yml $TMP_DIR/antrea.yml


elton-furtado · 2023-07-21T04:52:21Z

plugins/clickhouse-monitor/main.go

@@ -103,7 +105,12 @@ func main() {
 }

 func startMonitor(connect *sql.DB) {
-	foreverRun(func() {
+	stopCh := signals.RegisterSignalHandlers()
+	klog.InfoS("----------registered stop handler inside loop")


log cleanup

elton-furtado · 2023-07-21T04:52:59Z

plugins/clickhouse-monitor/main_test.go

 )

 func TestMonitorWithMockDB(t *testing.T) {
+	klog.InfoS("into function TestMonitorWithMockDB")


log cleanup

elton-furtado · 2023-07-21T04:57:15Z

test/e2e/framework.go

@@ -1529,7 +1536,243 @@ func (data *TestData) Cleanup(namespaces []string) {
 	}
 }

+// func (data *TestData) copyPodFiles(podName string, containerName string, nsName string, fileName string, covDir string) error {


elton-furtado · 2023-07-21T05:11:23Z

test/e2e/framework.go

 func flowVisibilityCleanup(tb testing.TB, data *TestData, config FlowVisibilitySetUpConfig) {
+	// TODO potentially check for files here
+	data.copyCovFilesFromPods(".coverage", "all")


why are we only copying the coverage from flow visibility tests? what about other tests?

added SIGINT handling to teardownFlowVisibility(), and added a test that only runs after the rests of the tests, copies all coverage files generated by all tests, deletes them off of the node, and finally cleans up local files after go tool covdata is used

tushartathgur · 2023-07-21T07:57:47Z

test/e2e/framework.go

+			err = data.killProcessesAndCollectCovFiles("flow-visibility", pod.Name, clickHouseMonitorContName, clickHouseMonitorContName, covDir+"/"+clickHouseMonitorCovFolder, "cm")
+		} else if strings.Contains(podName, "theia-manager") {
+			err = data.killProcessesAndCollectCovFiles("flow-visibility", pod.Name, theiaManagerContName, theiaManagerContName, covDir+"/"+theiaManagerCovFolder, "tm")
+		} else {


Not required else block

tushartathgur · 2023-07-21T07:57:56Z

test/e2e/framework.go

+		if !strings.Contains(err.Error(), "exit status 1") {
+			return fmt.Errorf("error while running docker cp command[%v] from node: %s: %v", cmd, nodeName, err)
+		}
+	}


add cleaning of coverage files, not required after copy. Also Cleanup files from the local after coverage is generated

tushartathgur · 2023-07-21T07:58:28Z

pkg/theia/commands/anomaly_detection_delete_test.go

@@ -144,4 +144,5 @@ func TestAnomalyDetectionDelete(t *testing.T) {
 			}
 		})
 	}
+	fmt.Println("done running AD delete test")


extra comment ?

tushartathgur · 2023-07-21T08:02:56Z

test/e2e/framework.go

+	if err != nil {
+		return fmt.Errorf("copyCovFolder: error creating absolute file path: %v", err)
+	}
+	cmd := exec.Command("docker", "cp", nodeName+":"+"/var/log/"+covPrefix+"-coverage/.", covDirAbs);


make a separate string beforehand, instead of using "+" operator in this line

Suggested change

cmd := exec.Command("docker", "cp", nodeName+":"+"/var/log/"+covPrefix+"-coverage/.", covDirAbs);

cpCmd := nodeName+":"+"/var/log/"+covPrefix+"-coverage/."

cmd := exec.Command("docker", "cp", cpCmd, covDirAbs);

build/charts/theia/values.yaml

ci/kind/test-e2e-kind.sh

plugins/clickhouse-monitor/main.go

plugins/clickhouse-monitor/main_test.go

test/e2e/framework.go

test/e2e/e2e_coverage_and_cleanup_test.go

ci/kind/test-e2e-kind.sh

test/e2e/e2e_coverage_and_cleanup_test.go

ci/kind/test-e2e-kind.sh

test/e2e/e2e_coverage_and_cleanup_test.go

Dhruv-J · 2023-07-22T16:34:32Z

/theia-test-e2e

Dhruv-J · 2023-07-22T19:57:24Z

/theia-test-e2e

Dhruv-J · 2023-07-22T20:55:32Z

/theia-test-e2e

Dhruv-J · 2023-07-23T04:33:54Z

/theia-test-e2e

elton-furtado · 2023-07-23T16:50:10Z

/theia-test-e2e

elton-furtado · 2023-07-24T15:55:02Z

codecov.yml

@@ -33,6 +33,16 @@ coverage:
        threshold: 1%
        flags:
          - unit-tests
+      theia-kind-e2e-tests:
+        target: auto
+        threshold: 1%


elton-furtado · 2023-07-24T15:55:11Z

codecov.yml

+          - kind-e2e-tests
+      python-unit-tests:
+        target: auto
+        threshold: 1%


elton-furtado · 2023-07-24T15:57:50Z

plugins/policy-recommendation/policy_recommendation_job_test.py

@@ -28,6 +28,7 @@

 @pytest.fixture(scope="session")
 def spark_session(request):
+    # sample change to policy_recommendation_job_test.py


this can be reverted now

tushartathgur · 2023-07-24T19:06:42Z

test/e2e/e2e_coverage_and_cleanup_test.go

+	pathOnNode := nodeName + ":" + "/var/log/" + covPrefix + "-coverage/."
+	cmd := exec.Command("docker", "cp", pathOnNode, covDirAbs)
+	var errb bytes.Buffer
+	cmd.Stderr = &errb


add stdout too for future debugging

Suggested change

cmd.Stderr = &errb

var stdout, stderr bytes.Buffer

cmd.Stderr = &stderr

cmd.Stdout = &stdout

tushartathgur · 2023-07-24T19:06:50Z

test/e2e/e2e_coverage_and_cleanup_test.go

@@ -0,0 +1,111 @@
+// Copyright 2022 Antrea Authors


Suggested change

// Copyright 2022 Antrea Authors

// Copyright 2023 Antrea Authors

tushartathgur · 2023-07-24T19:06:59Z

plugins/clickhouse-monitor/main.go

@@ -115,7 +120,8 @@ func startMonitor(connect *sql.DB) {
 			klog.ErrorS(nil, "Remaining rounds number to be skipped should be larger than or equal to 0", "number", remainingRoundsNum)
 			os.Exit(1)
 		}
-	}, monitorExecInterval)
+	}, monitorExecInterval, stopCh)
+	return


not required

tushartathgur · 2023-07-24T19:07:17Z

test/e2e/e2e_coverage_and_cleanup_test.go

+
+func copyCovFilesBothNodes(covPrefix string) error {
+	log.Infof("Copying coverage files from worker nodes %s and %s", workerNodeA, workerNodeB)
+	if err := copyCovFolder(workerNodeA, cmCovDir, covPrefix); err != nil {


Use for loop

tushartathgur · 2023-07-24T19:07:19Z

test/e2e/e2e_coverage_and_cleanup_test.go

+func clearCovFolder(nodeName, covPrefix string) error {
+	nestedCmd := "`rm -rf /var/log/" + covPrefix + "-coverage/*`"
+	cmd := exec.Command("docker", "exec", nodeName, "sh", "-c", nestedCmd)
+	var errb bytes.Buffer


Use stdout too

tushartathgur · 2023-07-24T19:07:22Z

test/e2e/e2e_coverage_and_cleanup_test.go

+
+func clearCovFilesBothNodes(covPrefix string) error {
+	log.Infof("Clearing coverage files from worker nodes %s and %s", workerNodeA, workerNodeB)
+	if err := clearCovFolder(workerNodeA, covPrefix); err != nil {


use for loop

tushartathgur · 2023-07-24T19:07:24Z

test/e2e/e2e_coverage_and_cleanup_test.go

+		t.Skipf("COVERAGE env variable is not set until all tests are run, skipping test.")
+	}
+	fmt.Println("RUNNING FINAL COVERAGE STUFF")
+	if err := copyCovFilesBothNodes("cm"); err != nil {


use for loop

yuntanghsu · 2023-07-24T19:18:05Z

test/e2e/e2e_coverage_and_cleanup_test.go

+func copyCovFolder(nodeName, covDir, covPrefix string) error {
+	covDirAbs, err := filepath.Abs("../../" + covDir)
+	if err != nil {
+		return fmt.Errorf("copyCovFolder: error creating absolute file path: %v", err)


Suggested change

return fmt.Errorf("copyCovFolder: error creating absolute file path: %v", err)

return fmt.Errorf("error while creating absolute file path: %v", err)

yuntanghsu · 2023-07-24T19:20:28Z

test/e2e/e2e_coverage_and_cleanup_test.go

+func clearCovFolder(nodeName, covPrefix string) error {
+	nestedCmd := "`rm -rf /var/log/" + covPrefix + "-coverage/*`"
+	cmd := exec.Command("docker", "exec", nodeName, "sh", "-c", nestedCmd)
+	var errb bytes.Buffer
+	cmd.Stderr = &errb
+	if err := cmd.Run(); err != nil {
+		errStr := errb.String()
+		if !strings.Contains(errb.String(), "not found") {
+			return fmt.Errorf("error while running docker exec command[%v] from node: %s: %s", cmd, nodeName, errStr)
+		}
+	}
+	return nil
+}


Are we able to combine reuse the code in copyCovFolder. I feel they are quite similar?

yuntanghsu · 2023-07-24T19:21:54Z

test/e2e/framework.go

+func (data *TestData) killProcessesOnPods() error {
+	listOptions := metav1.ListOptions{}
+
+	pods, err := data.clientset.CoreV1().Pods(flowVisibilityNamespace).List(context.TODO(), listOptions)


Suggested change

pods, err := data.clientset.CoreV1().Pods(flowVisibilityNamespace).List(context.TODO(), listOptions)

pods, err := data.clientset.CoreV1().Pods(flowVisibilityNamespace).List(context.TODO(), metav1.ListOptions{})

test/e2e/framework.go

yuntanghsu · 2023-07-24T19:30:52Z

test/e2e/framework.go

+			err = data.killProcesses("flow-visibility", podName, theiaManagerContName, theiaManagerContName)
+		}
+		if err != nil {
+			return fmt.Errorf("error when copying coverage files from pods: copy pod files out, error:%v", err)


Do we copy file in KillProcesses?

Updating go to version 1.20. Go v1.20 comes with many updates to coverage, additional functionality for some methods, library changes, as well as a method deprecation. Fixes issue antrea-io#297 Fixes issue antrea-io#178 Signed-off-by: Dhruv-J <[email protected]>

tushartathgur

lgtm

elton-furtado added this to the Theia v0.7 release milestone May 25, 2023

Dhruv-J force-pushed the update-go-to-v1.20 branch 2 times, most recently from 35a626c to a67d996 Compare July 3, 2023 16:38

elton-furtado requested review from tushartathgur and yuntanghsu July 14, 2023 23:00

Dhruv-J force-pushed the update-go-to-v1.20 branch 3 times, most recently from bf6775d to 87e17fb Compare July 20, 2023 22:25

elton-furtado reviewed Jul 21, 2023

View reviewed changes

tushartathgur reviewed Jul 21, 2023

View reviewed changes

Dhruv-J force-pushed the update-go-to-v1.20 branch from 1a9fd25 to 5c420fb Compare July 21, 2023 09:53

yuntanghsu reviewed Jul 21, 2023

View reviewed changes

yuntanghsu reviewed Jul 22, 2023

View reviewed changes

ci/kind/test-e2e-kind.sh Outdated Show resolved Hide resolved

test/e2e/e2e_coverage_and_cleanup_test.go Show resolved Hide resolved

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

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

Dhruv-J force-pushed the update-go-to-v1.20 branch from 0b10f15 to 954cb8f Compare July 22, 2023 16:33

Dhruv-J force-pushed the update-go-to-v1.20 branch from 954cb8f to d037015 Compare July 22, 2023 19:57

elton-furtado requested review from tushartathgur and yuntanghsu July 24, 2023 15:53

elton-furtado reviewed Jul 24, 2023

View reviewed changes

Dhruv-J force-pushed the update-go-to-v1.20 branch from d037015 to 703c154 Compare July 24, 2023 18:34

tushartathgur reviewed Jul 24, 2023

View reviewed changes

yuntanghsu reviewed Jul 24, 2023

View reviewed changes

Dhruv-J force-pushed the update-go-to-v1.20 branch 2 times, most recently from 1db74b5 to 0275b9e Compare July 24, 2023 20:56

elton-furtado requested review from yuntanghsu and tushartathgur July 25, 2023 01:28

Update go to v1.20

def9508

Updating go to version 1.20. Go v1.20 comes with many updates to coverage, additional functionality for some methods, library changes, as well as a method deprecation. Fixes issue antrea-io#297 Fixes issue antrea-io#178 Signed-off-by: Dhruv-J <[email protected]>

Dhruv-J force-pushed the update-go-to-v1.20 branch from 0275b9e to def9508 Compare July 25, 2023 02:58

tushartathgur approved these changes Jul 25, 2023

View reviewed changes

elton-furtado merged commit 1c5360f into antrea-io:main Jul 25, 2023
45 checks passed

	cmd := exec.Command("docker", "cp", nodeName+":"+"/var/log/"+covPrefix+"-coverage/.", covDirAbs);
	cpCmd := nodeName+":"+"/var/log/"+covPrefix+"-coverage/."
	cmd := exec.Command("docker", "cp", cpCmd, covDirAbs);

-	cmd.Stderr = &errb
+	var stdout, stderr bytes.Buffer
+ 	cmd.Stderr = &stderr
+ 	cmd.Stdout = &stdout

	// Copyright 2022 Antrea Authors
	// Copyright 2023 Antrea Authors

	return fmt.Errorf("copyCovFolder: error creating absolute file path: %v", err)
	return fmt.Errorf("error while creating absolute file path: %v", err)

	pods, err := data.clientset.CoreV1().Pods(flowVisibilityNamespace).List(context.TODO(), listOptions)
	pods, err := data.clientset.CoreV1().Pods(flowVisibilityNamespace).List(context.TODO(), metav1.ListOptions{})

Update go to v1.20 #299

Update go to v1.20 #299

Conversation

Dhruv-J commented May 16, 2023 • edited by elton-furtado Loading

yuntanghsu commented May 20, 2023

codecov bot commented May 22, 2023 • edited Loading

Codecov Report

antoninbas commented Jun 21, 2023

Dhruv-J commented Jun 21, 2023

antoninbas commented Jun 22, 2023

elton-furtado commented Jul 21, 2023

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

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

Dhruv-J commented Jul 22, 2023

Dhruv-J commented Jul 22, 2023

Dhruv-J commented Jul 22, 2023

Dhruv-J commented Jul 23, 2023

elton-furtado commented Jul 23, 2023

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

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

tushartathgur left a comment

Choose a reason for hiding this comment

Dhruv-J commented May 16, 2023 •

edited by elton-furtado

Loading

codecov bot commented May 22, 2023 •

edited

Loading