kubevirt / kubesecondarydns

DNS for KubeVirt VirtualMachines secondary interfaces
Apache License 2.0
7 stars 8 forks source link

e2e, Introduce e2e test: resolving a FQDN entry #38

Closed AlonaKaplan closed 1 year ago

AlonaKaplan commented 1 year ago

The test is creating a VMI with one multus and no default interface and verifies that nslookup to the interface fqdn retrieves the correct IP.

The PR also contains -

kubevirt-bot commented 1 year ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

oshoval commented 1 year ago

About nslookup binary I think we should do the following as part of hack/functest.sh

[ "$CI" == "true" ] && dnf install bind-utils (*)

if ! nslookup www.google.com > /dev/null 2>&1; then
  echo error
  exit 1
fi

So on both local and on CI, it will directly fail as first step if the binary doesn't exists the (*) will make it work on CI for now of course, and once we add it to the bootstrap image we can remove it. It might take time until it is merged, and a new image + bump exists, so IMO it is more comfortable to use (*) first and do it in the bootstrap in parallel. We should not install stuff locally as part of the e2e tests, nor require sudo to run the tests. On CI we don't need sudo. bootstrap file images/bootstrap/Dockerfile

oshoval commented 1 year ago

We need to remove push or pull_request from the git actions when you create a PR it runs both, maybe because you are the original repo creator or something (it doesn't happen when PRs are created by other people)

AlonaKaplan commented 1 year ago

[ "$CI" == "true" ] && dnf install bind-utils (*)

if ! nslookup www.google.com > /dev/null 2>&1; then
  echo error
  exit 1
fi

Thanks, updated the PR

oshoval commented 1 year ago

/release-note-none

oshoval commented 1 year ago

Need to show this to Brian please https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubesecondarydns/38/pull-kubesecondarydns-e2e-k8s/1599692018617946112

I think there is a new image with extended timeout will try to update job to v20221119-67aebd9

https://github.com/kubevirt/project-infra/pull/2500

oshoval commented 1 year ago

/test pull-kubesecondarydns-e2e-k8s

i hope btw sudo is not needed, lets see

oshoval commented 1 year ago

Created PR for the bootstrap https://github.com/kubevirt/project-infra/pull/2501

AlonaKaplan commented 1 year ago

/retest

oshoval commented 1 year ago

Please fork the repo for next PRs, it seems you are using the main repo branches https://github.com/kubevirt/kubesecondarydns/branches This is why it runs both pull_request and push git actions.

AlonaKaplan commented 1 year ago

do we still need pkg/controllers/controllers_suite_test.go or it can be deleted ?

We need it. It is for the controller unit tests.

oshoval commented 1 year ago

Please note this test would run also as part of CNAO CNAO deploys KSD and calls make create-nodeport, and then vendor the repo and run the make functest target. so if there is something that related to kubeconfig / namespace etc it should be dynamic (about kubeconfig saw you already took care)

AlonaKaplan commented 1 year ago

I use test namespace. It should be ok.

oshoval commented 1 year ago

Once https://github.com/kubevirt/project-infra/pull/2504 is merged we can remove the dnf install line please

AlonaKaplan commented 1 year ago

Thanks, few more small comments

about PR desc, nodes reducing - maybe worth to either explain why, or at least tell see (specific) commit for more info

The relevant commit message has an explanation. Do you think it is not enough?

oshoval commented 1 year ago

Thanks, few more small comments about PR desc, nodes reducing - maybe worth to either explain why, or at least tell see (specific) commit for more info

The relevant commit message has an explanation. Do you think it is not enough?

It is enough, but the line of the PR desc better to suggest to look on it please Changing the number of nodes in the ci cluster to 1 (it is enough for now). to Changing the number of nodes of the cluster to 1 (see 2nd commit).

You might want to amend the 2nd commit by adding that the reason the NodePort doesn't work on the first node, is that in case there are more than 1 nodes, that node is not a worker and only workers are included as you found. Alternatively also to add this info to the PR desc itself.

AlonaKaplan commented 1 year ago

/approve

kubevirt-bot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlonaKaplan

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: - ~~[OWNERS](https://github.com/kubevirt/kubesecondarydns/blob/main/OWNERS)~~ [AlonaKaplan] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment