Update documentation with deploy script and other minor changes#256
Update documentation with deploy script and other minor changes#256SpaceFace02 wants to merge 7 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SpaceFace02 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideUpdates the getting-started documentation to include explicit environment setup, cluster lifecycle steps, KubeVirt installation, and a new one-shot deployment script for quickly standing up a test environment. Flow diagram for one-shot deploy script in getting-started guideflowchart TD
A[Start one-shot deploy script] --> B[Set environment exports
CONTAINER_CLI, RUNTIME
AK_REGISTRATION_ADDR
TRUSTEE_ADDR
REGISTRY]
B --> C[make cluster-down]
C --> D[make cluster-up]
D --> E[make install-kubevirt]
E --> F[make push]
F --> G[make manifests]
G --> H[make install]
H --> I[kubectl -n trusted-execution-clusters get po,svc]
I --> J[sleep 10m]
J --> K[examples/create-ignition-secret.sh
examples/ignition-coreos.json
coreos-ignition-secret]
K --> L[kubectl apply -f examples/vm-coreos-ign.yaml]
L --> M[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The one-shot deploy script currently relies on a hardcoded
sleep 10m; consider replacing this with a readiness check (e.g.,kubectl waitor checking specific pods) so the script fails fast if the cluster or operator never becomes ready. - There are a few small typos in the new content (e.g.,
Kube-virtvsKubeVirt,oparator exports,deplyoed) that should be corrected for clarity and consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The one-shot deploy script currently relies on a hardcoded `sleep 10m`; consider replacing this with a readiness check (e.g., `kubectl wait` or checking specific pods) so the script fails fast if the cluster or operator never becomes ready.
- There are a few small typos in the new content (e.g., `Kube-virt` vs `KubeVirt`, `oparator exports`, `deplyoed`) that should be corrected for clarity and consistency.
## Individual Comments
### Comment 1
<location path="docs/usage/getting-started-guide.md" line_range="91" />
<code_context>
```
This example works with KubeVirt when the KBS is reachable using the pod networking.
+Make sure Kube-virt is also installed before trying to install the operator and testing functionality.
+```console
+make install-kubevirt
</code_context>
<issue_to_address>
**suggestion (typo):** Use the correct project name capitalization (KubeVirt).
Also remove the hyphen so it matches the upstream spelling.
```suggestion
Make sure KubeVirt is also installed before trying to install the operator and testing functionality.
```
</issue_to_address>
### Comment 2
<location path="docs/usage/getting-started-guide.md" line_range="115" />
<code_context>
+Refer to the one-shot deploy script at the end of this README for a quick install once you've understood the process.
+
Further customization of the project can be controlled with the following env variables:
+ NAMESPACE: sets the namespace where the operator will be deplyoed
+ PLATFORM: use during the installation to configure the platform where the operator will be deployed (`kind` or `openshift`)
</code_context>
<issue_to_address>
**issue (typo):** Fix the typo in "deplyoed".
In the NAMESPACE description, update the word to "deployed".
```suggestion
+ NAMESPACE: sets the namespace where the operator will be deployed
```
</issue_to_address>
### Comment 3
<location path="docs/usage/getting-started-guide.md" line_range="176" />
<code_context>
+export CONTAINER_CLI=docker
+export RUNTIME=docker
+
+# oparator exports
+export AK_REGISTRATION_ADDR=attestation-key-register.trusted-execution-clusters.svc.cluster.local
+export TRUSTEE_ADDR=kbs-service.trusted-execution-clusters.svc.cluster.local
</code_context>
<issue_to_address>
**issue (typo):** Fix the misspelling of "operator" in this comment.
Currently spelled "oparator"; please correct it to "operator".
```suggestion
# operator exports
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
@Jakob-Naucke For some reason I cannot add reviewers, unsure whether its a permission issue. Is /cc the correct way? |
|
/cc @Jakob-Naucke |
|
/cc @alicefr |
Jakob-Naucke
left a comment
Jakob-Naucke
left a comment
There was a problem hiding this comment.
Thank you for the improvements! Some small comments.
|
|
||
| Also make sure the container CLI environment variable is configured | ||
| ```console | ||
| export CONTAINER_CLI=docker |
There was a problem hiding this comment.
Building should work fine on Podman -- did you find this to be necessary?
There was a problem hiding this comment.
There is a line in the guide saying:
Kind can be used with docker or podman. Although, we set podman as default, right now there are some networking issues, therefore, the suggested runtime to use is docker at the moment. The runtime can be configured with:
So I went ahead and set default of runtime and container_cli to docker.
If the aforementioned issue is resolved, I'll change it to podman/CRI agnostic.
There was a problem hiding this comment.
Ah yes. CONTAINER_CLI is really just used for building and pushing, but this deserves clarification so let's maybe remove the paragraph but put this in its place:
If you require a non-Podman engine for building and pushing images, you can override it with the `$CONTAINER_CLI` variable.There was a problem hiding this comment.
building and pushing can be either podman or docker, but for the runtime env variable? That has to be docker due to networking issues on podman?
There was a problem hiding this comment.
Exactly, that's what the paragraph just above is about
|
|
||
| Make sure KubeVirt is also installed before trying to install the operator and testing functionality. | ||
| ```console | ||
| make install-kubevirt |
There was a problem hiding this comment.
I think it's good to have it in the one-shot but this install guide could be used for non-KubeVirt too.
|
|
||
| Wait for cluster to be ready: | ||
| ```console | ||
| sleep 10m |
There was a problem hiding this comment.
do we really need to put a sleep in the doc?
|
|
||
| Print cluster status | ||
| ```console | ||
| kubectl -n trusted-execution-clusters get po,svc |
There was a problem hiding this comment.
Something we could add is a Ready Condition in the TEC CR and wait for it. @Jakob-Naucke WDYT?
|
|
||
| In the logs, trustee prints the content of the TPM PCR registers. They need to match with the reference values present in the configmap `trustee-data` under `reference-values.json`. | ||
|
|
||
| ## One shot deploy script |
There was a problem hiding this comment.
Instead of putting a script in the doc, please commit it in the code
There was a problem hiding this comment.
I don't think this script is necessary the steps are described in the guide and depending on what the users want to achieve they might skip certain steps here
| make cluster-down | ||
| make cluster-up | ||
| make install-kubevirt |
There was a problem hiding this comment.
what I don't quite like of this script is that it always tears down the cluster while the cluster installation is necessary only once. You can run multiple times make install if you make some changes. I would simply discard this script
| This example works with KubeVirt when the KBS is reachable using the pod networking. Refer to the one-shot deploy shell script at the end of this guide for attesting a Kubevirt VM with plain vTPM based attestation, i.e (i.e. no real trusted execution environment). | ||
| This example works with KubeVirt when the KBS is reachable using the pod networking. | ||
|
|
||
| Refer to the one-shot deploy shell script at the end of this guide for attesting a Kubevirt VM with plain vTPM based attestation, i.e (i.e. no real trusted execution environment). |
There was a problem hiding this comment.
I would remove the script from the guide and therefore this sentence
3 participants
Added deploy script + minor changes
Summary by Sourcery
Update the getting started guide to document a full one-shot deployment flow and required environment/setup steps for creating and using a kind-based cluster with the operator.
Enhancements:
Documentation: