Skip to content
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

Add test against cda cluster to validate sfc behavior #201

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bn222
Copy link
Contributor

@bn222 bn222 commented Oct 31, 2024

No description provided.

Copy link

openshift-ci bot commented Oct 31, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bn222

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2024
@bn222
Copy link
Contributor Author

bn222 commented Oct 31, 2024

/test make-e2e-test

@SalDaniele
Copy link
Contributor

Nice!

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2024
@SalDaniele
Copy link
Contributor

/retest ci/prow/make-fast-e2e-test

Copy link

openshift-ci bot commented Oct 31, 2024

@SalDaniele: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test images
  • /test make-e2e-test
  • /test make-fast-e2e-test
  • /test make-fmt-check
  • /test make-generate-check
  • /test make-prow-ci-manifests-check
  • /test make-test
  • /test make-vendor-check

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-openshift-dpu-operator-main-images
  • pull-ci-openshift-dpu-operator-main-make-fast-e2e-test
  • pull-ci-openshift-dpu-operator-main-make-fmt-check
  • pull-ci-openshift-dpu-operator-main-make-generate-check
  • pull-ci-openshift-dpu-operator-main-make-prow-ci-manifests-check
  • pull-ci-openshift-dpu-operator-main-make-test
  • pull-ci-openshift-dpu-operator-main-make-vendor-check

In response to this:

/retest ci/prow/make-fast-e2e-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@SalDaniele
Copy link
Contributor

/test ci/prow/make-fast-e2e-test

Copy link

openshift-ci bot commented Oct 31, 2024

@SalDaniele: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test images
  • /test make-e2e-test
  • /test make-fast-e2e-test
  • /test make-fmt-check
  • /test make-generate-check
  • /test make-prow-ci-manifests-check
  • /test make-test
  • /test make-vendor-check

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-openshift-dpu-operator-main-images
  • pull-ci-openshift-dpu-operator-main-make-fast-e2e-test
  • pull-ci-openshift-dpu-operator-main-make-fmt-check
  • pull-ci-openshift-dpu-operator-main-make-generate-check
  • pull-ci-openshift-dpu-operator-main-make-prow-ci-manifests-check
  • pull-ci-openshift-dpu-operator-main-make-test
  • pull-ci-openshift-dpu-operator-main-make-vendor-check

In response to this:

/test ci/prow/make-fast-e2e-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 4598126 and 2 for PR HEAD c7a6da9 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3dfc39d and 1 for PR HEAD c7a6da9 in total

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2024
e2e-test/e2e_test.go Outdated Show resolved Hide resolved
@thom311
Copy link
Contributor

thom311 commented Nov 4, 2024

Could you add a README (or a comment somewhere) how such a test can run?

Obviously, it depends on the ipu_deploy target. Which turns the request into how to run the ipu_deploy target.

Since the CDA config in hack/cluster-configs/config-dpu.yaml reads the google-spread sheet, it seems clear that it can only work on machines from that spread sheet. If that is not the case, how to run the target on another system? If that is the case, which machines exactly? (also considering that some of those machines are hooked up to CI, it's not clear which can be taken for manual testing -- if any).

Point being: please explain this somewhere (a README would be good).


Also, it does seem to me that all those make e2e-test targets really only work on certain machines and are not usable to an upstream user (unless they patch parts of the code). With that, it seems strange that these are make targets (that look usable to anybody, when they are not). I think these should just be scripts under contrib/rh-ci or similar. Or if not and I just don't understand it, how can a user who is not in our CI lab use those targets? The point here is, why is this all hooked up as make targets, when it could just be scripts (in a clearly named directory). Well, the make targets are already just calling to scripts, so why do they exist at all?

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2024
@bn222
Copy link
Contributor Author

bn222 commented Nov 5, 2024

/override ci/prow/make-fast-e2e-test

Copy link

openshift-ci bot commented Nov 5, 2024

@bn222: Overrode contexts on behalf of bn222: ci/prow/make-fast-e2e-test

In response to this:

/override ci/prow/make-fast-e2e-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci-robot
Copy link

/test remaining-required

@bn222
Copy link
Contributor Author

bn222 commented Nov 20, 2024

/test make-e2e-test

@SalDaniele
Copy link
Contributor

/lgtm

@SalDaniele
Copy link
Contributor

/test make-e2e-test

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2024
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2024
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 0ac5055 in total

3 similar comments
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 0ac5055 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 0ac5055 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 0ac5055 in total

@bn222
Copy link
Contributor Author

bn222 commented Nov 25, 2024

/test make-e2e-test

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 0ac5055 in total

@vrindle
Copy link
Contributor

vrindle commented Nov 25, 2024

/test make-e2e-test

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 0ac5055 in total

4 similar comments
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 0ac5055 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 0ac5055 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 0ac5055 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 0ac5055 in total

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2024
@SalDaniele
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2024
@openshift-ci-robot
Copy link

/test remaining-required

bn222 and others added 2 commits November 26, 2024 21:49
Signed-off-by: Salvatore Daniele <sdaniele@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2024
@SalDaniele
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2024
@openshift-ci-robot
Copy link

/test remaining-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 1725d51 in total

3 similar comments
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 1725d51 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 1725d51 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 99f3e30 and 2 for PR HEAD 1725d51 in total

Copy link

openshift-ci bot commented Nov 27, 2024

@bn222: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/make-e2e-test 1725d51 link true /test make-e2e-test

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

if err != nil {
return nil, nil, err
}
dpuConfig, err := t.createClient("/root/kubeconfig.microshift")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test should run as non-root user. With those paths to the kubeconfig, only root can run them.

I think it should take the files from relativeToAbs("../../kubeconfig.*").

DpuConfigPath: "hack/cluster-configs/config-dpu.yaml",
}
_, dpuConfig, _ := cluster.EnsureExists()
Expect(err).NotTo(HaveOccurred())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs _, dpuConfig, err := ...

if err != nil {
return nil, err
}
return cfg, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.ReadFile() should suffice: https://pkg.go.dev/os#example-ReadFile

@@ -0,0 +1,136 @@
package daemon
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong package.

return &pod
}
}
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems such utilities should be part of testutils package. Also, why does not not use c.Get()?

Eventually(func() bool {
pod := getPod(dpuSideClient, nfName, ns)
if pod != nil {
println("Pod got")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this use println()? Other code seems to use loggers. Also, BeforeSuite() sets up the logger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants