-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: deploy and functest make tasks #196
Conversation
Hi @nestoracunablanco. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
b4aa937
to
2c87c07
Compare
scripts/deploy-kubevirt-and-cdi.sh
Outdated
kubectl patch -n ${KUBEVIRT_NAMESPACE} kubevirt kubevirt --type='json' -p '[{ | ||
"op": "add", | ||
"path": "/spec/configuration/developerConfiguration/featureGates/-", | ||
"value": "DataVolumes", | ||
},{ | ||
"op": "add", | ||
"path": "/spec/configuration/developerConfiguration/featureGates/-", | ||
"value": "CPUManager", | ||
},{ | ||
"op": "add", | ||
"path": "/spec/configuration/developerConfiguration/featureGates/-", | ||
"value": "KubevirtSeccompProfile", | ||
},{ | ||
"op": "add", | ||
"path": "/spec/configuration/developerConfiguration/featureGates/-", | ||
"value": "NUMA", | ||
},{ | ||
"op": "replace", | ||
"path": "/spec/configuration/seccompConfiguration", | ||
"value": { | ||
"virtualMachineInstanceProfile": { | ||
"customProfile": { | ||
"localhostProfile": "kubevirt/kubevirt.json" | ||
} | ||
} | ||
}, | ||
}]' |
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.
None of them should be needed anymore.
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.
To ensure that the function tests run successfully, both NUMA and GPU are required. However, I have removed the others.
scripts/deploy-kubevirt-and-cdi.sh
Outdated
#!/bin/bash | ||
set -e | ||
|
||
KUBEVIRT_NIGHTLY=$(curl -L https://storage.googleapis.com/kubevirt-prow/devel/nightly/release/kubevirt/kubevirt/latest) |
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.
Can you change it to KUBEVIRT_VERSION
and make it use the latest stable version by default? This will help pinning KUBEVIRT_VERSION
to a specific value in release branches.
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.
Done
scripts/deploy-kubevirt-and-cdi.sh
Outdated
KUBEVIRT_NAMESPACE=${1:-kubevirt} | ||
|
||
kubectl apply -f - <<EOF | ||
apiVersion: v1 | ||
kind: Namespace | ||
metadata: | ||
name: ${KUBEVIRT_NAMESPACE} | ||
EOF |
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 sure this is needed.
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.
Deleted
scripts/deploy-kubevirt-and-cdi.sh
Outdated
kubectl apply -f - <<EOF | ||
apiVersion: v1 | ||
kind: Namespace | ||
metadata: | ||
name: ${CDI_NAMESPACE} | ||
EOF |
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.
Same, I don't think this is needed.
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.
Deleted
2c87c07
to
00cab67
Compare
/ok-to-test |
Makefile
Outdated
|
||
.PHONY: functest | ||
functest: | ||
cd tests && kubectl config view --flatten > /tmp/kubeconfig && KUBECONFIG=/tmp/kubeconfig go test -v -timeout 0 ./functests/... -ginkgo.v -ginkgo.randomize-all $(FUNCTEST_EXTRA_ARGS) |
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.
What does writing to /tmp/kubeconfig
first help with?
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.
Hi @0xFelix, thank you for your comment.
If I understood the code correctly, in order for the function tests to work effectively, the variable KUBECONFIG must point to a kubectl config file. Given this information, I have thought of a solution: dumping the config to a temporary file and then loading it as an environment variable for the tests.
I am open to any ideas, comments, suggestions, etc. How would you approach solving this?
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.
If you don't specify KUBECONFIG
it should pick up the default ~/.kube/config
that in this case is also used by kubectl
. Did it not work without KUBECONFIG
?
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.
For example, in my case, when using minikube, it does not write the configuration to ~/.kube/config
. Therefore, I came up with the idea of dumping the config to an extra file and changing KUBECONFIG
only within the context of the function 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.
Where is kubectl config view --flatten
looking at in this case? IIUC without KUBECONFIG
it will look at ~/.kube/config
?
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.
Solved in a more elegant way. @0xFelix you can take a look.
scripts/deploy-kubevirt-and-cdi.sh
Outdated
|
||
|
||
# Deploying kubevirt | ||
KUBEVIRT_VERSION=$(latest_version "kubevirt") |
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.
Can you just use https://storage.googleapis.com/kubevirt-prow/devel/release/kubevirt/kubevirt/stable.txt
as we do elsewhere?
KUBEVIRT_VERSION=$(curl -L https://storage.googleapis.com/kubevirt-prow/devel/release/kubevirt/kubevirt/stable.txt) |
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.
Done.
|
||
|
||
# Deploying CDI | ||
CDI_VERSION=$(latest_version "containerized-data-importer") |
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.
I don't think we have a version of stable.txt
available for CDI so this is fine.
ed4831b
to
6634e73
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.
Looks good to me, thanks!
/lgtm
6634e73
to
55e5516
Compare
Hey @0xFelix, I've made some updates to the code as the tests were failing. Could you please take another look? |
Makefile
Outdated
@@ -41,6 +44,14 @@ lint: generate | |||
generate: kustomize yq | |||
scripts/generate.sh | |||
|
|||
.PHONY: deploy | |||
deploy: generate lint |
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.
nit: generate
is a dependency of lint
, so it's redundant.
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.
Done.
This tasks are helpful when working wieh an external Kubernetes cluster. The 'deploy' task installs the bundles and their dependencies, while the 'functest' task runs the function tests against the cluster. Signed-off-by: Nestor Acuna-Blanco <nestor.acuna@ibm.com>
55e5516
to
23b19c8
Compare
/retest |
@nestoracunablanco: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
/test 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.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lyarwood 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 |
This tasks are helpful when working wieh an external Kubernetes cluster.
The 'deploy' task installs the bundels and their dependencies, whilw the 'functest' task runs the function tests against the cluster.
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):No issue writen
Special notes for your reviewer:
The file
deploy-kubevirt-and-cdi
is very similar to the one in the ssp-operator repository. The plan is to update it in the other repository if the one there gets merged.Release note: