-
Notifications
You must be signed in to change notification settings - Fork 25
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.
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
Update go to v1.20 #299
Update go to v1.20 #299
Conversation
I see there are couple of errors. Could you fix them? |
Codecov Report
@@ 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
*This pull request uses carry forward flags. Click here to find out more.
|
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? |
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. |
35a626c
to
a67d996
Compare
bf6775d
to
87e17fb
Compare
/theia-test-e2e |
build/charts/theia/README.md
Outdated
@@ -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 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this required now? shouldn't there be a 'theia-monitor-coverdir'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required as it's a constant, removing it
build/charts/theia/values.yaml
Outdated
@@ -270,3 +272,4 @@ theiaManager: | |||
tlsMinVersion: "" | |||
# -- Log verbosity switch for Theia Manager. | |||
logVerbosity: 0 | |||
# -- coverage directory to be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed
ci/kind/test-e2e-kind.sh
Outdated
mkdir -p .coverage/clickhouse-monitor-coverage/ | ||
mkdir -p .coverage/theia-manager-coverage/ | ||
mkdir -p .coverage/merged | ||
echo "STARTED_RUNNING" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started running Kind E2E tests code coverage, same comment below
ci/kind/test-e2e-kind.sh
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup
ci/kind/test-e2e-kind.sh
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup
plugins/clickhouse-monitor/main.go
Outdated
@@ -103,7 +105,12 @@ func main() { | |||
} | |||
|
|||
func startMonitor(connect *sql.DB) { | |||
foreverRun(func() { | |||
stopCh := signals.RegisterSignalHandlers() | |||
klog.InfoS("----------registered stop handler inside loop") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log cleanup
) | ||
|
||
func TestMonitorWithMockDB(t *testing.T) { | ||
klog.InfoS("into function TestMonitorWithMockDB") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log cleanup
test/e2e/framework.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup?
test/e2e/framework.go
Outdated
func flowVisibilityCleanup(tb testing.TB, data *TestData, config FlowVisibilitySetUpConfig) { | ||
// TODO potentially check for files here | ||
data.copyCovFilesFromPods(".coverage", "all") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we only copying the coverage from flow visibility tests? what about other tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
test/e2e/framework.go
Outdated
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required else block
test/e2e/framework.go
Outdated
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) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add cleaning of coverage files, not required after copy. Also Cleanup files from the local after coverage is generated
@@ -144,4 +144,5 @@ func TestAnomalyDetectionDelete(t *testing.T) { | |||
} | |||
}) | |||
} | |||
fmt.Println("done running AD delete test") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra comment ?
test/e2e/framework.go
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make a separate string beforehand, instead of using "+" operator in this line
cmd := exec.Command("docker", "cp", nodeName+":"+"/var/log/"+covPrefix+"-coverage/.", covDirAbs); | |
cpCmd := nodeName+":"+"/var/log/"+covPrefix+"-coverage/." | |
cmd := exec.Command("docker", "cp", cpCmd, covDirAbs); |
1a9fd25
to
5c420fb
Compare
0b10f15
to
954cb8f
Compare
/theia-test-e2e |
954cb8f
to
d037015
Compare
/theia-test-e2e |
3 similar comments
/theia-test-e2e |
/theia-test-e2e |
/theia-test-e2e |
@@ -33,6 +33,16 @@ coverage: | |||
threshold: 1% | |||
flags: | |||
- unit-tests | |||
theia-kind-e2e-tests: | |||
target: auto | |||
threshold: 1% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target 60%
- kind-e2e-tests | ||
python-unit-tests: | ||
target: auto | ||
threshold: 1% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target 50%
@@ -28,6 +28,7 @@ | |||
|
|||
@pytest.fixture(scope="session") | |||
def spark_session(request): | |||
# sample change to policy_recommendation_job_test.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be reverted now
d037015
to
703c154
Compare
pathOnNode := nodeName + ":" + "/var/log/" + covPrefix + "-coverage/." | ||
cmd := exec.Command("docker", "cp", pathOnNode, covDirAbs) | ||
var errb bytes.Buffer | ||
cmd.Stderr = &errb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add stdout too for future debugging
cmd.Stderr = &errb | |
var stdout, stderr bytes.Buffer | |
cmd.Stderr = &stderr | |
cmd.Stdout = &stdout |
@@ -0,0 +1,111 @@ | |||
// Copyright 2022 Antrea Authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Copyright 2022 Antrea Authors | |
// Copyright 2023 Antrea Authors |
plugins/clickhouse-monitor/main.go
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required
|
||
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use for loop
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use stdout too
|
||
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use for loop
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use for loop
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return fmt.Errorf("copyCovFolder: error creating absolute file path: %v", err) | |
return fmt.Errorf("error while creating absolute file path: %v", err) |
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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we able to combine reuse the code in copyCovFolder
. I feel they are quite similar?
test/e2e/framework.go
Outdated
func (data *TestData) killProcessesOnPods() error { | ||
listOptions := metav1.ListOptions{} | ||
|
||
pods, err := data.clientset.CoreV1().Pods(flowVisibilityNamespace).List(context.TODO(), listOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we copy file in KillProcesses
?
1db74b5
to
0275b9e
Compare
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]>
0275b9e
to
def9508
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
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