kubevirt / containerized-data-importer

Data Import Service for kubernetes, designed with kubevirt in mind.
Apache License 2.0
428 stars 269 forks source link

Fix test failures at port forwarding stop on s390x #3533

Open Davo911 opened 6 days ago

Davo911 commented 6 days ago

What this PR does / why we need it: Several critical tests are failing on s390x during, what appears to be, a cleanup step:

STEP: Stop port forwarding @ 11/06/24 11:18:39.461
  [FAILED] in [AfterEach] - /root/go/src/kubevirt.io/containerized-data-importer/tests/upload_test.go:104 @ 11/06/24 11:18:39.462
  << Timeline

  [FAILED] Expected success, but got an error:
      <*exec.ExitError | 0xc00066ea40>:
      exit status 1
      {
          ProcessState: {
              pid: 1480951,
              status: 256,
              rusage: {
                  Utime: {Sec: 0, Usec: 200995},
                  Stime: {Sec: 0, Usec: 157220},
                  Maxrss: 62104,
                  Ixrss: 0,
                  Idrss: 0,
                  Isrss: 0,
                  Minflt: 2760,
                  Majflt: 0,
                  Nswap: 0,
                  Inblock: 0,
                  Oublock: 0,
                  Msgsnd: 0,
                  Msgrcv: 0,
                  Nsignals: 0,
                  Nvcsw: 9775,
                  Nivcsw: 259,
              },
          },
          Stderr: nil,
      }
  In [AfterEach] at: /root/go/src/kubevirt.io/containerized-data-importer/tests/upload_test.go:104 @ 11/06/24 11:18:39.462

By trying to terminate the process in a more graceful way, it should be less likely to run into this in the future.

NONE
kubevirt-bot commented 6 days ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign aglitke for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kubevirt/containerized-data-importer/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
kubevirt-bot commented 6 days ago

Hi @Davo911. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
Davo911 commented 6 days ago

Still a Draft, because even though this brings down the failing critical tests from 30 to 12, some of the 12 remaining still seem to fail at this Step.

akalenyu commented 6 days ago

I wonder why you even end up under the port-forward branch? We set up a NodePort and configure the CDI config's uploadProxyURL accordingly https://github.com/kubevirt/containerized-data-importer/blob/e0683b40b02c6ba9519af901ec0c55a1fbd8512b/cluster-sync/sync.sh#L98-L102 The port forward bit was just left around as a fallback but that isn't used really since OpenShift has a route and kvci has the NodePort setup.

Davo911 commented 6 days ago

Hey @akalenyu thanks for looking at this! For my local setup I'm using KUBEVIRT_PROVIDER=external, in that case this configuration step is skipped.

https://github.com/kubevirt/containerized-data-importer/blob/e0683b40b02c6ba9519af901ec0c55a1fbd8512b/cluster-sync/sync.sh#L278-L283

akalenyu commented 6 days ago

Hey @akalenyu thanks for looking at this! For my local setup I'm using KUBEVIRT_PROVIDER=external, in that case this configuration step is skipped.

https://github.com/kubevirt/containerized-data-importer/blob/e0683b40b02c6ba9519af901ec0c55a1fbd8512b/cluster-sync/sync.sh#L278-L283

I see, and I guess that cluster is not on OpenShift? In that case, try setting up an ingress https://github.com/kubevirt/containerized-data-importer/blob/main/doc/exposing-upload-proxy.md Once that is set up on your cluster, and the CDI config has an updated uploadProxy, you won't need any code changes and you will magically avoid the port forward setup branch.

Davo911 commented 6 days ago

I'll look into that or switch to openshift for local development.

Should we include that into https://github.com/kubevirt/containerized-data-importer/blob/448fe9d566964f0fbff4fa2f4f0f4904e5331838/hack/README.md or https://github.com/kubevirt/containerized-data-importer/blob/448fe9d566964f0fbff4fa2f4f0f4904e5331838/cluster-sync/sync.sh by any chance?

akalenyu commented 6 days ago

To me it's a moving part so not really. Maybe drop the port forward setup bit and just fail if the uploadproxyurl is not set?