Copilot commented on code in PR #235:
URL:
https://github.com/apache/skywalking-satellite/pull/235#discussion_r2969398580
##########
plugins/client/grpc/static_clients_test.go:
##########
@@ -75,15 +75,27 @@ func TestStaticServer(t *testing.T) {
// init client
c := initClient(ports, t)
- // wait all channel being connected (connect by async)
- time.Sleep(time.Second * 1)
-
- // send request
+ // send requests and retry until all receivers have received data,
+ // since grpc.NewClient connects lazily and connections may become
READY at different times.
jvmClient :=
agent.NewJVMMetricReportServiceClient(c.GetConnectedClient().(*grpc.ClientConn))
- for inx := 0; inx < serverCount*3; inx++ { // the lb connection will be
connected with async, so we need to send more request
- if _, err := jvmClient.Collect(context.Background(),
&agent.JVMMetricCollection{}); err != nil {
- t.Errorf("send request error: %v", err)
+ deadline := time.Now().Add(30 * time.Second)
+ for time.Now().Before(deadline) {
+ for inx := 0; inx < serverCount; inx++ {
+ if _, err := jvmClient.Collect(context.Background(),
&agent.JVMMetricCollection{}); err != nil {
+ t.Logf("send request error (may be transient):
%v", err)
+ }
+ }
+ allReceived := true
+ for _, r := range receivers {
+ if r.receiveCount <= 0 {
+ allReceived = false
+ break
+ }
+ }
Review Comment:
`receiveCount` is read here while it's incremented from the gRPC handler
goroutines (see `Collect()`), which introduces a data race (and can make this
test flaky under `-race`). Make `receiveCount` concurrency-safe (e.g., use
`sync/atomic` for increments/loads or guard it with a mutex) and update the
read/check logic accordingly.
##########
test/e2e/case/istio/metrics/e2e.yaml:
##########
@@ -52,37 +52,40 @@ setup:
command: bash test/e2e/base/scripts/prepare/setup-e2e-shell/install.sh
helm
- name: Install kubectl
command: bash test/e2e/base/scripts/prepare/setup-e2e-shell/install.sh
kubectl
+ - name: Install ECK operator
+ command: |
+ helm pull oci://ghcr.io/apache/skywalking-helm/skywalking-helm \
+ --version "0.0.0-${SW_KUBERNETES_COMMIT_SHA}" --untar
+ helm dep up skywalking-helm
+ helm -n istio-system install eck-operator
skywalking-helm/charts/eck-operator-*.tgz \
+ --create-namespace
+ kubectl -n istio-system rollout status --watch --timeout=120s
statefulset/elastic-operator
- name: Install SkyWalking
command: |
- rm -rf skywalking-kubernetes && git clone
https://github.com/apache/skywalking-kubernetes.git
- cd skywalking-kubernetes
- git reset --hard $SW_KUBERNETES_COMMIT_SHA
- cd chart
- mkdir -p skywalking/files/conf.d/oap/ && cp
../../test/e2e/case/istio/metadata-service-mapping.yaml
skywalking/files/conf.d/oap/metadata-service-mapping.yaml
- helm dep up skywalking
- helm -n istio-system install skywalking skywalking \
- --set fullnameOverride=skywalking \
- --set elasticsearch.replicas=1 \
- --set elasticsearch.minimumMasterNodes=1 \
- --set
oap.env.SW_ENVOY_METRIC_ALS_HTTP_ANALYSIS=$ALS_ANALYZER \
- --set
oap.env.SW_ENVOY_METRIC_ALS_TCP_ANALYSIS=$ALS_ANALYZER \
- --set
oap.env.K8S_SERVICE_NAME_RULE='e2e::${service.metadata.name}' \
- --set oap.envoy.als.enabled=true \
- --set oap.replicas=1 \
- --set ui.image.repository=ghcr.io/apache/skywalking/ui \
- --set ui.image.tag=$SW_UI_COMMIT \
- --set oap.image.tag=$SW_OAP_COMMIT \
- --set
oap.image.repository=ghcr.io/apache/skywalking/oap \
- --set oap.storageType=elasticsearch \
- --set oap.startupProbe.failureThreshold=60 \
- --set oap.startupProbe.tcpSocket.port=12800 \
- --set oap.startupProbe.initialDelaySeconds=120 \
- --set oap.startupProbe.periodSeconds=10 \
- --set satellite.enabled=true \
- --set
satellite.image.repository=apache/skywalking-satellite \
- --set satellite.image.tag=vlatest \
- --set
satellite.env.SATELLITE_GRPC_ACCEPT_LIMIT_CPU_UTILIZATION=100 \
- -f ../../test/e2e/case/istio/values.yaml
+ helm -n istio-system install skywalking \
+ oci://ghcr.io/apache/skywalking-helm/skywalking-helm \
+ --version "0.0.0-${SW_KUBERNETES_COMMIT_SHA}" \
+ --set fullnameOverride=skywalking \
+ --set eckOperator.enabled=false \
+ --set oap.env.SW_ENVOY_METRIC_ALS_HTTP_ANALYSIS=$ALS_ANALYZER \
Review Comment:
The PR description is framed as CVE/dependency and base-image upgrades, but
this file also changes the Istio E2E setup (adds ECK operator installation and
switches SkyWalking install to the OCI Helm chart). Please reflect this
additional E2E scope in the PR description (or split it out) to keep the
security-fix PR narrowly scoped and easier to review/rollback.
##########
plugins/client/grpc/static_clients_test.go:
##########
@@ -75,15 +75,27 @@ func TestStaticServer(t *testing.T) {
// init client
c := initClient(ports, t)
- // wait all channel being connected (connect by async)
- time.Sleep(time.Second * 1)
-
- // send request
+ // send requests and retry until all receivers have received data,
+ // since grpc.NewClient connects lazily and connections may become
READY at different times.
jvmClient :=
agent.NewJVMMetricReportServiceClient(c.GetConnectedClient().(*grpc.ClientConn))
- for inx := 0; inx < serverCount*3; inx++ { // the lb connection will be
connected with async, so we need to send more request
- if _, err := jvmClient.Collect(context.Background(),
&agent.JVMMetricCollection{}); err != nil {
- t.Errorf("send request error: %v", err)
+ deadline := time.Now().Add(30 * time.Second)
+ for time.Now().Before(deadline) {
+ for inx := 0; inx < serverCount; inx++ {
+ if _, err := jvmClient.Collect(context.Background(),
&agent.JVMMetricCollection{}); err != nil {
Review Comment:
The retry loop is bounded by `deadline`, but each RPC uses
`context.Background()` so a single `Collect` call can block indefinitely (e.g.,
during connection establishment) and exceed the intended 30s bound. Use a
per-RPC context with timeout/deadline (or reuse a context tied to `deadline`)
so the test runtime is actually bounded and failures return promptly.
```suggestion
ctx, cancel := context.WithDeadline(context.Background(), deadline)
defer cancel()
for time.Now().Before(deadline) {
for inx := 0; inx < serverCount; inx++ {
if _, err := jvmClient.Collect(ctx,
&agent.JVMMetricCollection{}); err != nil {
```
##########
test/e2e/case/istio/als/e2e.yaml:
##########
@@ -42,37 +42,40 @@ setup:
kubectl label namespace default istio-injection=enabled
- name: Install helm
command: bash test/e2e/base/scripts/prepare/setup-e2e-shell/install.sh
helm
+ - name: Install ECK operator
+ command: |
+ helm pull oci://ghcr.io/apache/skywalking-helm/skywalking-helm \
+ --version "0.0.0-${SW_KUBERNETES_COMMIT_SHA}" --untar
+ helm dep up skywalking-helm
+ helm -n istio-system install eck-operator
skywalking-helm/charts/eck-operator-*.tgz \
+ --create-namespace
+ kubectl -n istio-system rollout status --watch --timeout=120s
statefulset/elastic-operator
- name: Install SkyWalking
command: |
- rm -rf skywalking-kubernetes && git clone
https://github.com/apache/skywalking-kubernetes.git
- cd skywalking-kubernetes
- git reset --hard $SW_KUBERNETES_COMMIT_SHA
- cd chart
- mkdir -p skywalking/files/conf.d/oap/ && cp
../../test/e2e/case/istio/metadata-service-mapping.yaml
skywalking/files/conf.d/oap/metadata-service-mapping.yaml
- helm dep up skywalking
- helm -n istio-system install skywalking skywalking \
- --set fullnameOverride=skywalking \
- --set elasticsearch.replicas=1 \
- --set elasticsearch.minimumMasterNodes=1 \
- --set
oap.env.SW_ENVOY_METRIC_ALS_HTTP_ANALYSIS=$ALS_ANALYZER \
- --set
oap.env.SW_ENVOY_METRIC_ALS_TCP_ANALYSIS=$ALS_ANALYZER \
- --set
oap.env.K8S_SERVICE_NAME_RULE='e2e::${service.metadata.name}' \
- --set oap.envoy.als.enabled=true \
- --set oap.replicas=1 \
- --set ui.image.repository=ghcr.io/apache/skywalking/ui \
- --set ui.image.tag=$SW_UI_COMMIT \
- --set oap.image.tag=$SW_OAP_COMMIT \
- --set
oap.image.repository=ghcr.io/apache/skywalking/oap \
- --set oap.storageType=elasticsearch \
- --set oap.startupProbe.failureThreshold=60 \
- --set oap.startupProbe.tcpSocket.port=12800 \
- --set oap.startupProbe.initialDelaySeconds=120 \
- --set oap.startupProbe.periodSeconds=10 \
- --set satellite.enabled=true \
- --set
satellite.image.repository=apache/skywalking-satellite \
- --set satellite.image.tag=vlatest \
- --set
satellite.env.SATELLITE_GRPC_ACCEPT_LIMIT_CPU_UTILIZATION=100 \
- -f ../../test/e2e/case/istio/values.yaml
+ helm -n istio-system install skywalking \
+ oci://ghcr.io/apache/skywalking-helm/skywalking-helm \
+ --version "0.0.0-${SW_KUBERNETES_COMMIT_SHA}" \
+ --set fullnameOverride=skywalking \
+ --set eckOperator.enabled=false \
+ --set oap.env.SW_ENVOY_METRIC_ALS_HTTP_ANALYSIS=$ALS_ANALYZER \
Review Comment:
The PR description focuses on CVE/dependency and base-image upgrades, but
these changes introduce a new E2E flow (switching SkyWalking install to the OCI
Helm chart and adding an explicit ECK operator install). Please update the PR
description to mention these E2E changes (or split them into a separate PR) so
reviewers/maintainers understand the additional scope and risk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
