DarumaDocker
commented
1 year ago Flows review
Open juntao opened 1 year ago
DarumaDocker
commented
1 year ago Flows review
juntao
commented
1 year ago Hello, I am a serverless review bot on flows.network. Here are my reviews of changed source code files in this PR.
Overall, the code snippet appears to be well-written and easy to understand. However, there are a few potential improvements and issues.
Inconsistency in environment variable usage for WASMEDGE_VERSION and CRUN_VERSION:
$WASMEDGE_VERSION and $CRUN_VERSION, instead of $wasmedge and $crun.Consider using labels for if conditions in jobs:
$wasmedge="latest", consider labeling each case with descriptive names to make it easier to follow when modifying the workflow later on. For example, if: ${{ matrix.case_name == 'latest' }}.Ensure proper input validation for logLevel input:
logLevel input from the workflow_dispatch trigger, but it is not used in the script. This parameter should be validated and passed to a proper step if it is needed.Potential optimizations in the installation steps:
containerd and crun are executed depending on the value of matrix.latest. By compacting these steps into one and using conditional statements within the step itself, the code could be made cleaner and easier to maintain.Error stream redirection:
2>&1 to other shell commands as well, ensuring all error output is captured in the log files consistently.Use clearer names for environment variables:
Protect against malicious commit messages or branch names:
github.head_ref variable for a concurrency group label, be aware that this can be controllable by users who push a branch or create a pull request. Consider sanitizing or filtering this value to avoid potential issues that may arise from malicious input.Order of the matrix.include values:
matrix.include section as in the matrix section would improve the code readability.The key changes in the given patch are:
Changed the paths-ignore options:
push and pull_request triggers now have paths-ignore set to ignore changes in README.md.Update crun version in the matrix options:
crun in the matrix have been replaced with a single version - 1.8.4.Updated the actions/checkout version:
actions/checkout GitHub Action has been updated from v2 to v3.Overall, the code snippet appears to be well-organized and readable. However, there are a few potential issues or areas for improvement:
Use of sudo with apt-get: It might be better to use sudo with the -E flag to preserve the environment variables when running apt-get. This is relevant when using the ACCEPT_EULA=Y variable to accept licenses during installation. Modify the apt-get lines like so:
- name: Install apt-get packages
run: |
sudo -E ACCEPT_EULA=Y apt-get update
sudo -E ACCEPT_EULA=Y apt-get upgrade
sudo -E ACCEPT_EULA=Y apt-get install git wgetDuplicate execution of simple_wasi_application.sh: The script simple_wasi_application.sh is executed twice with different conditions, but they could be combined into a single step. You can set the WASMEDGE_VERSION and CRUN_VERSION environment variables based on the matrix values, and then execute the script only once. For example:
- name: Set up environment variables
run: |
echo "WASMEDGE_VERSION=${{ matrix.wasmedge }}" >> $GITHUB_ENV
echo "CRUN_VERSION=${{ matrix.crun }}" >> $GITHUB_ENV
- name: Install containerd and crun
run: |
bash containerd/install.sh --wasmedge=$WASMEDGE_VERSION --crun=$CRUN_VERSION
- name: Run demo in containerd
env:
TAG: ${{ matrix.tag }}
VARIANT: ${{ matrix.variant }}
run: |
bash containerd/simple_wasi_application.sh --tag=$TAG --variant=$VARIANT > dump.log 2>&1Include error checking for commands: There is no error checking for the installation of packages, containerd, crun, or other steps in the workflow. Consider adding error checking by using set -e or set -o errexit at the beginning of the shell script, or checking the exit status of commands right after they are executed, for example:
apt-get install git wget || exit 1Change actions/checkout@v3 to actions/checkout@v2: As of now, there's no such version as actions/checkout@v3. You must change this action reference to actions/checkout@v2 instead.
Keep a single cron schedule: You might want to have a single cron schedule for the whole workflow. In the current setup, the schedule is for a single job. However, you may want the same behavior for all other jobs as well. To do this, move the schedule property under the on field like below:
on:
workflow_dispatch:
...
push:
...
pull_request:
...
schedule:
- cron: "0 0 */1 * *"Addressing these issues should help improve the reliability and maintenance of your GitHub Actions workflow.
This patch includes the following key changes:
paths-ignore field has been added to both the push and pull_request triggers. This means the workflow won't be triggered for changes only affecting the README.md file.crun matrix option has been updated from multiple version numbers ["1.6", "1.7", "1.7.1", "1.7.2"] to a single version number ["1.8.4"]. This means the workflow will only test this specific crun version.actions/checkout action version has been updated from v2 to v3.Overall, the code snippet looks fine. However, there are a few recommendations I would like to make for potential improvements:
For this block of code:
sudo ACCEPT_EULA=Y apt-get update
sudo ACCEPT_EULA=Y apt-get upgrade
sudo ACCEPT_EULA=Y apt-get install git wgetIt is better to consolidate the three commands into a single apt-get command for optimization:
sudo ACCEPT_EULA=Y apt-get update && sudo ACCEPT_EULA=Y apt-get upgrade -y && sudo ACCEPT_EULA=Y apt-get install -y git wgetFor better readability and maintainability, consider breaking the long single-line commands into multiple lines using YAML's multiline syntax:
run: |
bash crio/http_server/http_server_application.sh \
--tag=$TAG \
--config=$CONFIG > dump.log 2>&1and
if grep -q "echo: name=WasmEdge" dump.log; then
echo -e "Execution Success!"
else
echo -e "Execution Fail! Please check the above log for details"
exit 1
fiThat's it! Other than these recommendations, the code snippet looks good.
The key changes in the patch are as follows:
The push and pull_request triggers now have paths-ignore configurations added:
paths-ignore:
- '**/README.md'This will ignore the README.md file changes, meaning the workflow will not be triggered for changes only related to the README.md file.
The crun matrix configuration changed:
From: ["1.6", "1.7", "1.7.1", "1.7.2"]
To: ["1.8.4"]
This simplifies the test matrix to only include crun version 1.8.4 instead of previous multiple versions.
The exclude block has been removed from the matrix configuration. The exclude block was excluding specific crun and wasmedge version combinations. This is no longer necessary since the crun matrix configuration is simplified.
The actions/checkout action version updated:
From: v2
To: v3
This updates the GitHub Actions checkout action version to the latest release (v3) for potential bug fixes and improvements.
Overall, the source code snippet is well-structured and easy to understand, but there are a few potential issues and improvements that can be made.
Potential problem when using "latest" for dependencies: When using "latest" for wasmedge or crun, there might be unexpected changes or incompatibilities that cause the build to fail. It is generally better to use specific versions to avoid instability in the pipeline.
Mixing single and double quotes for strings: In the script, single quotes (' ') and double quotes (" ") are used interchangeably for string values. It would be better to stick to one type (preferably single quotes) for consistency, unless there is a specific need to use the other type.
Hardcoded string in the check result section: The string "Printed from wasi: This is from a main function" is hardcoded in the check result section. It would be better to define the expected output as a variable to make it easier to update when necessary.
Duplication of paths-ignore: Both "push" and "pull_request" events have paths-ignore specified. To enhance maintainability, you may consider using a YAML anchor and reference, or move the shared attributes to a separate customized event filter.
Use an environment variable for 0 0 */1 * * in schedule: To improve maintainability and readability, consider declaring the cron expression 0 0 */1 * * as an environment variable with a descriptive name.
Missing '-y' flag for apt-get commands: When running apt-get commands such as apt-get update, apt-get upgrade, and apt-get install, it is recommended to add the -y flag to automatically confirm the installation. Thus, the commands should look like sudo ACCEPT_EULA=Y apt-get update -y, sudo ACCEPT_EULA=Y apt-get upgrade -y, and sudo ACCEPT_EULA=Y apt-get install git wget -y.
Here is the revised version of the source code snippet with the mentioned improvements:
name: CRI-O test
concurrency:
group: crio-${{ github.head_ref }}
cancel-in-progress: true
on:
workflow_dispatch:
inputs:
logLevel:
description: 'Log level'
required: true
default: 'info'
push:
branches: [ main ]
paths-ignore: &ignore-paths
- '**/README.md'
pull_request:
branches: [ main ]
paths-ignore: *ignore-paths
schedule:
- cron: ${{ env.SCHEDULE_INTERVAL }}
env:
SCHEDULE_INTERVAL: '0 0 */1 * *'
SUCCESS_MESSAGE: 'Printed from wasi: This is from a main function'
jobs:
crio:
strategy:
fail-fast: false
matrix:
crun: ['1.8.4']
wasmedge: ['0.11.0', '0.11.1', '0.11.2']
config: ['container_wasi.json']
tag: ['latest']
latest: [false]
include:
- name: 'WasmEdge latest release, Crun master'
latest: true
wasmedge: 'latest'
crun: 'latest'
tag: 'latest'
config: 'container_wasi.json'
runs-on: ubuntu-20.04
name: WasmEdge:${{ matrix.wasmedge }},crun:${{ matrix.crun }}
steps:
- uses: actions/checkout@v3
with:
fetch-depth: 0
- name: Install apt-get packages
run: |
sudo ACCEPT_EULA=Y apt-get update -y
sudo ACCEPT_EULA=Y apt-get upgrade -y
sudo ACCEPT_EULA=Y apt-get install git wget -y
- name: Install CRI-O and crun (specified version)
if: ${{ ! matrix.latest }}
env:
WASMEDGE_VERSION: ${{ matrix.wasmedge }}
CRUN_VERSION: ${{ matrix.crun }}
run: |
bash crio/install.sh --wasmedge=$WASMEDGE_VERSION --crun=$CRUN_VERSION
- name: Install CRI-O and crun (latest version)
if: ${{ matrix.latest }}
run: |
bash crio/install.sh
- name: Run demo in CRI-O
env:
TAG: ${{ matrix.tag }}
CONFIG: ${{ matrix.config }}
run: |
bash crio/simple_wasi_application.sh --tag=$TAG --config=$CONFIG > dump.log 2>&1
- name: Display crun and wasmedge version
run: |
crun --version
wasmedge --version
- name: Dump the log of execution
run: |
cat dump.log
- name: Check the result
run: |
if grep -q "${{ env.SUCCESS_MESSAGE }}" dump.log; then
echo -e "Execution Success!"
else
echo -e "Execution Fail! Please check the above log for details"
exit 1
fiThe key changes in this patch are:
Added paths-ignore to push and pull_request events, instructing the workflow to ignore changes in the 'README.md' file.
Updated the crun version matrix to only include 1.8.4, removing 1.6, 1.7, 1.7.1, and 1.7.2. This means that the tests will now be run against Crun version 1.8.4 only.
Removed the exclude clause that specified combinations of crun and wasmedge versions to be excluded due to incompatibilities. With the updated version matrix for Crun, these exclusions are no longer necessary.
Updated the actions/checkout action version from v2 to v3.
The source code snippet appears to be a GitHub Actions workflow YAML file designed for testing a container management application using WasmEdge and Crun with Kubernetes. Here are a few suggestions for improvements:
Make sure to use consistent and proper indentation for better readability. In the matrix field, include delimiter should be aligned with its other siblings.
The following lines might be unnecessary if you want to trigger the CI when any changes in README.md are pushed or during pull requests.
paths-ignore:
- '**/README.md'Update the environment variable names to uppercase for better readability and convention:
if: ${{ ! matrix.LATEST }}
env:
WASMEDGE_VERSION: ${{ matrix.wasmedge }}
CRUN_VERSION: ${{ matrix.crun }}The following step introduces an arbitrary 1200s (20-minute) delay in your workflow:
- name: Sleep for 1200s
run: sleep 1200s
shell: bashConsider using more reliable and efficient ways to wait for resources to be ready for testing, such as Kubernetes readiness probes or using Kubernetes wait commands.
When dumping log files, it might be helpful to provide a clearer message in the console by adding echo statements before displaying the logs.
For example:
- name: Dump the log of k8s setup
run: |
echo "====== K8S LOGS ======="
cat k8s.log
- name: Dump the log of execution
run: |
echo "====== EXECUTION LOGS ======="
cat dump.logInstead of just echoing "Execution Fail," consider providing specific feedback about what might have caused the failure.
Add relevant comments to the code to provide better understanding and maintainability.
The key changes in the patch are as follows:
Removed paths-ignore for both push and pull_request events:
This change will now trigger the workflow when there are changes in the README.md file for both push and pull_request events.
Updated Crun versions in the matrix:
The Crun versions have been updated in the matrix to only test with "1.8.4" instead of "1.6", "1.7", "1.7.1", and "1.7.2". The previously excluded Crun versions are now removed from the list.
Removed exclude field from the matrix configuration:
The exclude field that prevented specific combinations of Crun and WasmEdge versions from being tested has been removed since the Crun version is updated to "1.8.4".
Updated the version of actions/checkout action:
The version of the actions/checkout action has been updated from "v2" to "v3". This ensures the use of the latest version of the action for checking out the repository.
ACCEPT_EULA=Y is only required for certain software packages that require accepting an End User License Agreement. It is unlikely that git and wget require this. You may remove ACCEPT_EULA=Y from the following code block:- name: Install apt-get packages
run: |
sudo apt-get update
sudo apt-get upgrade
sudo apt-get install git wgetmatrix.include only once within the strategy section:matrix:
crun: ["1.8.4"]
wasmedge: ["0.11.0", "0.11.1", "0.11.2"]
tag: ["latest"]
latest: [false]
variant: ["compat-smart"]
include:
- name: "WasmEdge latest release, Crun master"
latest: true
wasmedge: "latest"
crun: "latest"
tag: "latest"
variant: "compat-smart"actions/checkout@v3 where the latest version is actually v2.4.2. Change the version to actions/checkout@v2 for reliability:- uses: actions/checkout@v2
with:
fetch-depth: 0k8s.log and dump.log contain important data, consider uploading these logs as artifacts when a step fails so that you can easily download and analyze them. Add the following code segments after the corresponding Dump the log ... steps:- name: Upload k8s log if setup fails
if: ${{ failure() }}
uses: actions/upload-artifact@v2
with:
name: k8s-log
path: k8s.log
- name: Upload dump log if execution fails
if: ${{ failure() }}
uses: actions/upload-artifact@v2
with:
name: dump-log
path: dump.logThese suggestions should improve the reliability and maintainability of your workflow.
The key changes in this patch are:
paths-ignore for both push and pull_request events to ignore changes in README.md files:push:
branches: [ main ]
paths-ignore:
- '**/README.md'
pull_request:
branches: [ main ]
paths-ignore:
- '**/README.md'Updated the crun version in the matrix from multiple versions ["1.6", "1.7", "1.7.1", "1.7.2"] to a single version ["1.8.4"]. This change reduces the number of test matrix combinations.
Removed the exclude section, as it is no longer necessary with the updated crun version:
exclude:
- ...actions/checkout from v2 to v3. However, as mentioned in my previous response, the latest stable version is v2. I recommend keeping the version as v2 instead of the non-existent v3.Overall, the snippet appears mostly correct. However, there are a few potential issues that may be improved upon:
schedule, push, and pull_request triggers:It is recommended to keep consistency between the triggers if possible. Instead of having individual push and pull_request sections with repeated conditions, you can combine them as follows:
on:
workflow_dispatch:
...
push:
branches: [ main ]
paths-ignore:
- '**/README.md'
pull_request:
branches: [ main ]
paths-ignore:
- '**/README.md'
schedule:
- cron: "0 0 */1 * *"Some of the installation and setup steps may fail without stopping the workflow execution (install.sh scripts, k8s.log, and others). It is better to handle errors and stop the execution immediately when a failure occurs. Consider using bash -e or set -e in the script execution to stop on the first encountered error.
sleep in the workflow:Using a fixed sleep time of 1200s seems too arbitrary and may result in wasted time if the setup completes earlier. Instead, consider monitoring the actual status of the Kubernetes cluster, and proceed after it becomes ready using polling, timeouts, or wait conditions.
Using a fixed version number for the software used in the matrix (like "1.8.4" for crun) may not be ideal in the long run. When a new version is released, the workflow will need to be updated manually. You could consider automating the version-fetching task and providing an option to run with the latest stable version without having to modify the workflow file manually.
In some places, environment variable names are in uppercase (e.g., WASMEDGE_VERSION, CRUN_VERSION), and in others, they are in lowercase (e.g., logLevel). It would be good to use a consistent naming convention.
The key changes in the provided patch are:
push and pull_request triggers have been updated to include the paths-ignore condition, which was previously missing in the push trigger: push:
branches: [ main ]
paths-ignore:
- '**/README.md'
pull_request:
branches: [ main ]
paths-ignore:
- '**/README.md'crun version matrix has been simplified by removing the exclude section and reducing the number of versions from four to one: matrix:
- crun: ["1.6", "1.7", "1.7.1", "1.7.2"]
+ crun: ["1.8.4"]
wasmedge: ["0.11.0", "0.11.1", "0.11.2"]actions/checkout step has been updated from version 2 to version 3: steps:
- - uses: actions/checkout@v2
+ - uses: actions/checkout@v3The source code snippet is a GitHub Actions workflow for testing Kubernetes with CRI-O and WasmEdge. Here are a few potential issues and recommendations:
Redundant fetch-depth value in actions/checkout step:
The fetch-depth value currently set as 0 is redundant because the default value is already 1, which means it will fetch only the latest commit.
You can remove the whole with section if the shallow clone is enough:
- uses: actions/checkout@v3sudo ACCEPT_EULA=Y apt-get upgrade might cause unintended side effects:
Upgrading all packages in the OS can potentially introduce compatibility issues or conflicts between dependencies. It is recommended to only upgrade required packages or use a fixed version of the OS or dependencies instead.
It is also advised to run apt-get upgrade non-interactively to avoid blocking the workflow execution waiting for user input:
sudo ACCEPT_EULA=Y apt-get upgrade -yNot specifying the path attribute in paths-ignore:
The paths-ignore list is currently filtering any change to files named README.md, but this could occur in any directory in the repository, which may not be the intended behavior. To clarify the intention, specify the path of the file (e.g. /README.md).
cancel-in-progress might cause unnecessary job cancellations:For jobs with longer execution times, the cancel-in-progress flag can lead to multiple overlapping jobs being executed repeatedly if the workflow is dispatched before the previous job finishes. In this case, consider increasing the cron interval for the scheduled workflow or allowing parallel execution by removing the cancel-in-progress flag.
The Sleep for 1200s step seems to be waiting for Kubernetes and CRI-O to install and start properly, but a more efficient way would be to implement a health check that polls the status of the services and continues as soon as they are ready, instead of waiting for a fixed amount of time. This would shorten the overall run time and keep the logs more concise.
The success message "Printed from wasi: This is from a main function" is hardcoded in the Check the result step. If the success message changes in the future, the step will fail. It would be better to store the expected outcome in a variable or a file that can be updated more easily.
The current script ends with either a success or fail message. It would be helpful to handle errors more specifically for easier debugging. For example, you could:
The key changes in this patch are:
Added paths-ignore conditions to both push and pull_request triggers. This means that the workflow will not be triggered if the changes made involve only files named README.md.
Updated the crun version in the matrix from multiple versions (1.6, 1.7, 1.7.1, and 1.7.2) to only one version (1.8.4).
Removed the exclude section which contained combinations of specific crun and wasmedge versions. This is no longer necessary since the crun version was updated to 1.8.4.
Updated the actions/checkout action from version 2 to version 3.
I have reviewed the code snippet, and found some potential issues that might be worth addressing:
The code snippet does not have a name for the k8s_kwasm_server job. While this does not affect functionality, it can be confusing when viewing the GitHub Actions log. It's recommended to add a name for better readability:
k8s_kwasm_server:
name: Kubernetes WasmEdge HTTP Server Deployment
runs-on: ubuntu-latestThe code snippet does not specify any timeout limit for the k8s_kwasm_server job. It's recommended to add a timeout value to prevent a job from running indefinitely in case of any issues:
k8s_kwasm_server:
name: Kubernetes WasmEdge HTTP Server Deployment
runs-on: ubuntu-latest
timeout-minutes: 15The shell commands in the run step do not have any error handling or exit traps in case any command fails. This can lead to difficulty in debugging if any command fails. It is advised to add error handling or use the set command to make sure the job fails immediately if any command fails:
- run: |
set -e
helm repo add kwasm http://kwasm.sh/kwasm-operator/
helm repo update
...There are two sleep commands with 30 and 10 seconds sleep time, which may not be consistent for different environments. It would be better to use an active waiting method to check for the target service status (e.g., using a until or timeout command), or at least set sleep values as environment variables for easier tweaking:
- run: |
set -e
...
kubectl apply -f kwasm/http-server/http_server_application.yaml
until kubectl rollout status deployment/your_deployment_name; do sleep 5; done
kubectl port-forward service/demo-service 8080 &
sleep 10
curl -d "name=WasmEdge" -X POST http://localhost:8080
echoWith these changes, the code snippet would be more robust and easier to handle in different environments.
The given patch includes a YAML configuration for a GitHub Actions workflow that deploys a Kubernetes WasmEdge HTTP server. Key changes are as follows:
Establishes workflow concurrency, allowing only one run of the workflow at a time, and canceling any in-progress runs when a new run is triggered.
Triggers the workflow on several events, such as manual dispatch, push to the main branch (ignoring README.md changes), pull requests to the main branch (ignoring README.md changes), and a scheduled daily run.
Provides a single job, k8s_kwasm_server, running on the ubuntu-latest environment that contains the following steps:
actions/checkout@v3.helm/kind-action@v1.4.0.kwasm Helm repository, and update the Helm repositories.kwasm-operator using the Helm.kwasm.sh/kwasm-node=true.kwasm/http-server/http_server_application.yaml.kubectl port-forward to forward port 8080.The source code snippet appears to define a GitHub Action workflow for Kubernetes k8s kwasm test. Here are a few suggestions:
name attribute to each step:
It is recommended to add a comment or name attribute for each step to clearly indicate its purpose. This will help other developers understand the purpose of each step when they review or update the workflow.steps:
- name: Checkout repository
uses: actions/checkout@v3
with:
fetch-depth: 0
- name: Set up KinD (Kubernetes in Docker)
uses: helm/kind-action@v1.4.0
- name: Install and run Kwasm on Kubernetes
run: |
...Use a more readable cron syntax:
Currently, the workflow is scheduled to run at "0 0 /1 ", which may be confusing. You can use a more readable alternative like "0 0 " if this is supposed to run daily at midnight.
Handling inputs.logLevel:
Although logLevel is set as required input in the workflow_dispatch event, it is not used in the actual steps of the job. If this input is necessary, add a step to utilize or pass the logLevel value to the commands or scripts where it's required.
Output validation and error handling:
As the workflow installs a helm chart, annotates a node, and applies a YAML configuration file, it's crucial to validate the output of each command. Any errors should result in a failed job to avoid misinterpretation of the job results. You can use set -e at the beginning of the run block to make the script exit if any command fails.
- name: Install and run Kwasm on Kubernetes
run: |
set -e
helm repo add kwasm http://kwasm.sh/kwasm-operator/
...With these suggestions, your code should be more maintainable and reliable. Consider adding optional features such as caching to speed up the build process if necessary, and make sure to apply any specific project requirements to the workflow.
The patch defines a new GitHub Action workflow named "k8s kwasm test". Key changes and features include:
workflow_dispatch with a required input of logLevel, push and pull_request specifically for the main branch (ignoring changes in README.md), and schedule with a cron expression to run periodically.k8s_kwasm is defined with multiple steps.
kwasm.sh/kwasm-node label.Overall, the code snippet appears to be a Kubernetes deployment configuration for a demo-app along with a RuntimeClass and a Service. It seems functional at first glance. However, there are a few recommendations and questions to be addressed:
Make sure the crun handler is actually installed and configured on your cluster nodes. If it's not available or improperly configured, you may run into runtime issues.
Consider adding resource requests and limits to the container to manage its resource consumption. For example:
resources:
requests:
cpu: 100m
memory: 128Mi
limits:
cpu: 200m
memory: 256MiThis can help prevent potential resource hogging and over-allocation by setting a boundary for your application's resource usage.
In the Service, the type is set to LoadBalancer. If you are deploying this on a cloud provider, this might create an external load balancer resource. If you do not intend for the service to be exposed externally, you might want to consider changing the type to ClusterIP. Alternatively, if you are looking for a temporary external exposure for testing purposes, you can use the type NodePort.
If you want to add an exposed URL for your service, you could use Ingress configuration along with the service to expose a specific route (e.g., example.com/demo). This improves the usability of the app by providing a user-friendly URL for accessing the service.
Depending on the application, consider adding any relevant readiness and liveness probes to ensure that the application is up and healthy. These probes are important for Kubernetes to understand when the application is ready and when to restart containers if the application stops responding.
Make sure your organization's policies and best practices are being followed in areas like container image management, versioning, and access control.
In conclusion, the snippet appears functional, but depending on your requirements and the application, you may want to consider some of the above recommendations.
This patch introduces changes to Kubernetes configuration, including creating a RuntimeClass object called "crun", a Deployment object called "demo-deployment," and a Service object called "demo-service." The key changes are as follows:
A RuntimeClass named "crun" is created, with its "handler" set to "crun" as well.
A Deployment configuration named "demo-deployment" is added with the following specifications:
A Service configuration named "demo-service" is created with the following attributes:
Here are some potential issues that I found in this code snippet:
To fix this, you can update the resources section under the containers field like this:
resources:
requests:
cpu: 100m
memory: 100Mi
limits:
cpu: 500m
memory: 500MiFor example, instead of using wasmedge/example-wasi:latest, you could use wasmedge/example-wasi:1.0.0.
handler value in the RuntimeClass kind is correct: The handler should point to the correct runtime to be used, such as runc or another one that you have installed on your cluster. Make sure that the handler value of crun is correct and intentional.Apart from these issues, the code snippet looks fine. Note that the provided snippet is not a full Kubernetes configuration and may require additional context to ensure that it works correctly in your environment. Do ensure that all desired configurations are defined as needed.
This patch introduces a Kubernetes configuration with the following key components:
A RuntimeClass named crun with a handler set to crun. This configuration defines the runtime to be used for running containers in the Kubernetes cluster.
A Job named wasm-demo with the following specifications:
module.wasm.image/variant set to compat-smart.wasmedge/example-wasi:latest./wasi_example_main.wasm with an argument 50000000.Never, which means the job won't be restarted on failure.crun runtime, as indicated by the runtimeClassName field.backoffLimit of 1, which means the job will be retried only once after the initial failure.Please note that there are a few potential issues with this patch, as mentioned in my previous response. You may want to consider addressing those issues when implementing the configuration in a production environment.
cc https://github.com/second-state/wasmedge-containers-examples/pull/29
juntao
commented
1 year ago flows review
juntao
commented
1 year ago Hello, I am a serverless review bot on flows.network. Here are my reviews of changed source code files in this PR.
Overall, the YAML script for the GitHub Actions workflow looks pretty decent. However, there are a few suggestions that could improve its quality:
The ACCEPT_EULA=Y environment variable is set for each command that uses apt-get, which could be redundant. Instead, set it in the environment section of the containerd_server job:
jobs:
containerd_server:
runs-on: ubuntu-20.04
name: WasmEdge:${{ matrix.wasmedge }},crun:${{ matrix.crun }}
environment:
ACCEPT_EULA: YThen, you can simply run apt-get commands without ACCEPT_EULA=Y:
- name: Install apt-get packages
run: |
sudo apt-get update
sudo apt-get upgrade
sudo apt-get install git wgetIt's advisable to use the shellcheck action to check the shell scripts (containerd/install.sh and containerd/http_server/http_server_application.sh) for potential issues, as it can detect common mistakes and help improve script quality. Add the shellcheck action to your workflow like this:
- name: ShellCheck
uses: ludeeus/action-shellcheck@master
with:
severity: warning
followSourced: trueIf possible, add comments to the workflow steps to provide context and more information about the purpose of each step.
Instead of manually displaying the execution log using cat and checking for success using grep, you may consider using the run key to direct the output to the console for visibility and using exit code checks for success or failure.
It is better to use the official action for checking out the code in your workflows (actions/checkout@v3 should be actions/checkout@v2). However, if you have a specific reason to use v3, make sure that it is an available version.
Apart from these suggestions, the code looks good, and it's clear what it is trying to achieve.
The key changes in the patch are:
paths-ignore is added to both the push and pull_request triggers. This means that the workflow will not be triggered if the only changes made in a push or pull request are in README.md files. paths-ignore:
- '**/README.md'crun matrix version choices are changed from ["1.6", "1.7", "1.7.1", "1.7.2"] to just ["1.8.4"]. This reduces the number of combinations tested and focuses the testing on a specific version of crun. crun: ["1.8.4"]actions/checkout action version is updated from v2 to v3. - uses: actions/checkout@v3The given code is a GitHub Actions workflow for testing Containerd with different combinations of Crun and WasmEdge versions. Below, I provide some suggestions and potential problems in the code:
In the on section, single quotes are used, but in other sections, double quotes are used. While this is not a problem for YAML, having a consistent quote style is generally recommended for better readability.
The logLevel input provided in the workflow_dispatch event is not used within the job steps. To make use of this, you should pass it as an environmental variable or an argument to the script or command you want to use it within.
containerd/install.sh:In the steps for installing containerd and crun, the matrix.latest condition is checked. If true, it installs the latest version without any specified version passed. When matrix.latest is false, the script is run with the specified WASMEDGE_VERSION and CRUN_VERSION. However, if the default behavior of install.sh is to install the latest versions when no arguments are passed, the condition for 'matrix.latest' is not required, just run the script without passing arguments.
-e:echo -e is used in the last step to display success or failure messages. The -e flag is used to interpret backslash escapes usually. Since you don't have any backslash escapes in the message strings, the -e flag is unnecessary.
It would be good practice to add error handling mechanisms in the script execution steps, like using set -e or set -o pipefail to make sure the actions fail if any sub-command fails.
Based on these observations, you can make changes to the given code to improve it further.
This patch introduces the following key changes:
Added paths-ignore to both push and pull_request triggers: With this change, the workflow will not be triggered when only the README.md file is updated. This can help in reducing the unnecessary runs for documentation changes.
Updated Crun versions in the matrix: The Crun versions have been updated to only include "1.8.4". This change reduces the number of combinations in the testing matrix, which can save resources and testing time.
Updated the actions/checkout action version: The action's version has been updated from v2 to v3. While both versions work, it is always a good practice to keep up-to-date with the latest version, as it may introduce bug fixes, new features, or optimizations.
Here are some potential issues and improvements in the presented workflow file:
paths-ignore configuration:The paths-ignore configuration under push and pull_request events is the same. You can simplify the workflow configuration by using a single push and pull_request event without specifying branches and still achieve the same result.
on:
workflow_dispatch:
inputs:
logLevel:
description: 'Log level'
required: true
default: 'info'
push:
paths-ignore:
- '**/README.md'
pull_request:
paths-ignore:
- '**/README.md'
schedule:
- cron: "0 0 */1 * *"actions/checkout@v3:actions/checkout@v3 doesn't currently exist. The latest version of actions/checkout is v2. Please replace the following line:
- uses: actions/checkout@v3with:
- uses: actions/checkout@v2jobs.crio_server:The name property is missing for jobs.crio_server. To add it, use the following:
jobs:
crio_server:
name: CRI-O HTTP server testaccept_eula:It is not necessary to set ACCEPT_EULA=Y each time; you can set this as an environment variable for the job. You can define the environment variable once at the job level, and it will be available for all steps in the job, like so:
jobs:
crio_server:
...
env:
ACCEPT_EULA: Y
steps:
...Then, you can remove the redundant ACCEPT_EULA=Y from each run command that requires apt-get.
> instead of -i flag with grep:Please use the -i flag with grep command to make the search case-insensitive, as follows:
if grep -iq "echo: name=WasmEdge" dump.log; thenThis ensures that the search will still match even if there's a change in the case of the text being searched.
Otherwise, the provided GitHub Actions workflow appears to be correctly set up, and I see no other major issues or concerns with the code.
The key changes in the provided patch are:
Modification of the crun versions in the matrix of the jobs.crio_server job. The patch removes multiple versions of crun ("1.6", "1.7", "1.7.1", "1.7.2") and focuses on testing only version "1.8.4".
Removal of the exclude field in the matrix configuration. Previously, some tests were excluded because specific combinations of crun and wasmedge versions would fail due to apparmor settings. Since only one crun version is now being tested, these exclusions are no longer necessary.
Update of the actions/checkout version from v2 to v3. This change is not recommended, as there is no version v3 currently available for actions/checkout. The latest version is still v2.
Overall, the code appears to be formatted well and follows a good structure. However, some potential improvements and issues have been identified below.
actions/checkout@v3 in the steps should be actions/checkout@v2. As of now, there is no version 3 for the GitHub Checkout action.Replace:
- uses: actions/checkout@v3With:
- uses: actions/checkout@v2Install apt-get packages section, the ACCEPT_EULA=Y might not be required for the packages being installed in this step. The ACCEPT_EULA=Y is typically used when installing packages like Microsoft SQL Server tools and libraries that require accepting the End User License Agreement (EULA) during installation.You might consider removing ACCEPT_EULA=Y from the run commands as follows:
Replace:
run: |
sudo ACCEPT_EULA=Y apt-get update
sudo ACCEPT_EULA=Y apt-get upgrade
sudo ACCEPT_EULA=Y apt-get install git wgetWith:
run: |
sudo apt-get update
sudo apt-get upgrade
sudo apt-get install git wgetjobs section to prevent your test environment from hanging indefinitely on a specific job. This can help save resources and clean up the environment in case of unexpected issues during the tests:Add the following line under the crio job:
timeout-minutes: <number_of_minutes>This is an optional improvement, and you can choose an appropriate number of minutes as per your test requirements.
Other than these minor improvements, the code is well-structured, and no serious issues were discovered during the review. Please make the suggested changes, and the code should work as expected.
This patch introduces the following key changes to the source code:
Adds paths-ignore for both push and pull_request triggers, effectively ignoring any changes made to README.md files.
paths-ignore:
- '**/README.md'Updates the crun matrix values by removing multiple versions ("1.6", "1.7", "1.7.1", "1.7.2") and replacing them with a single version "1.8.4".
crun: ["1.8.4"]Removes the entire 'exclude' block from the matrix, which previously excluded specific combinations of crun and wasmedge versions due to an issue with apparmor settings causing failures in wasm custom handlers.
Changes the version of the GitHub Checkout action from v2 to v3 in the steps section. Note: This change should be reverted as mentioned in the previous review since there is no version 3 for the GitHub Checkout action.
Replace:
- uses: actions/checkout@v3With:
- uses: actions/checkout@v2Overall the code appears well-organized, and the job structure follows standard practices for GitHub Actions. However, there are a few potential issues and improvements that can be made.
It's recommended to use single quotes for string values in YAML files for consistency.
description: 'Log level'
default: 'info'Should be changed to:
description: "Log level"
default: "info"sudo for apt-get commands:The sudo ACCEPT_EULA=Y prefix for the apt-get commands in the "Install apt-get packages" step is unnecessary since GitHub Actions runners run as root by default. You can remove the sudo and ACCEPT_EULA=Y parts, like this:
- name: Install apt-get packages
run: |
apt-get update
apt-get upgrade
apt-get install git wgetFor the "Installing and starting k8s" step, you are redirecting both stdout and stderr to k8s.log. But if any error occurs during this step, the step won't fail, and the error details will only appear after the sleep command in a separate step. A better approach would be to run the script without redirecting output, letting the step fail immediately if there's an error:
- name: Installing and starting k8s
run: |
bash kubernetes_containerd/install.shYou could similarly remove the output redirection in the "Run WasmEdge in k8s" step. This way, the error logs will be shown right in the workflow logs for easier debugging.
The cancel-in-progress: true option under concurrency might lead to cases where a legitimate new job is cancelled because it started at the same time as another. Consider removing this option or make sure this is the desired behavior.
Instead of using matrix variables for WASMEDGE_VERSION and CRUN_VERSION, you can define them as environment variables inside the.env section of the "Install containerd and crun (specified version)" step, like this:
- name: Install containerd and crun (specified version)
if: ${{ ! matrix.latest }}
env:
WASMEDGE_VERSION: ${{ matrix.wasmedge }}
CRUN_VERSION: ${{ matrix.crun }}
run: |
bash containerd/install.sh --wasmedge=$WASMEDGE_VERSION --crun=$CRUN_VERSIONThis will make the code more readable and less error-prone.
This is not related to the code itself but a general practice. Make sure to use proper commit messages when submitting the code, so it's clear what changes were made in each commit. Use understandable and meaningful messages that briefly explain the changes. Avoid using vague messages like "Update" or "Fix bugs".
This patch includes the following key changes:
Removed the paths-ignore option from the push and pull_request events, so this workflow is no longer skipped when there are changes only to README.md.
Updated the crun version matrix to only include version 1.8.4, removing previous versions (1.6, 1.7, 1.7.1, and 1.7.2).
Removed the exclude section, which previously excluded certain combinations of crun and wasmedge versions due to a specific issue with AppArmor.
Updated the GitHub Actions checkout action to use version 3 instead of version 2.
Overall, the YAML configuration for the GitHub Actions workflow seems to be well-written. However, I would like to point out a few suggestions and small issues that I noticed:
Redundant paths-ignore:
The paths-ignore in both sections push and pull_request are set to ignore the README.md file.
paths-ignore:
- '**/README.md'If this workflow should not run when only the README.md file is changed, it's fine. But, if you want the workflow to run on any changes, you can remove this part.
Use longer sleep time: In the following step, the sleep time is set to 1200 seconds (20 minutes) which might be too long. You may want to review the need for such a long sleep duration and adjust it accordingly.
- name: Sleep for 1200s
run: sleep 1200s
shell: bashConsider using a create-release action: If the workflow is responsible for creating new releases, consider using a GitHub action like actions/create-release. This can help you automate the versioning and release creation process.
Error messages consistency: In the "Check the result" step, the success message has an exclamation mark, while the error message does not. This is a minor inconsistency that can be fixed.
echo -e "Execution Success!"
echo -e "Execution Fail! Please check the above log for details"Remove duplicate bash usage:
For most of the shell steps in the workflow, the usage of bash is redundant as it is the default shell for the GitHub runners. Here are the redundant bash usages:
run: |
bash containerd/install.sh --wasmedge=$WASMEDGE_VERSION --crun=$CRUN_VERSION
run: |
bash containerd/install.sh
run: |
bash kubernetes_containerd/install.sh > k8s.log 2>&1
run: |
bash kubernetes_containerd/simple_wasi_application.sh --tag=$TAG --variant=$VARIANT > dump.log 2>&1Removing the unnecessary bash will make the workflow cleaner and easier to maintain.
Mask sensitive data (optional):
Make sure there is no sensitive data being displayed in any of the logs. For example, if you have any authentication tokens or passwords being used in your installation scripts, make sure to mask these values by using the ::add-mask:: notation in the GitHub Actions.
Always use the most recent version of actions: In the following step, you are using version 3 of actions/checkout:
- uses: actions/checkout@v3At the time of writing, the most recent version of actions/checkout is v2. Make sure always to reference the latest tag available.
Overall, the presented workflow seems very interesting, and I didn't find any critical issues with it. If you address the suggestions mentioned above, it will improve the quality of your GitHub Actions workflow. As always, it is a good practice to thoroughly test your workflows with various inputs and scenarios to ensure it behaves as expected.
The key changes in this patch are:
Added paths-ignore to both push and pull_request events:
paths-ignore:
- '**/README.md'This change makes the workflow ignore any changes to the README.md file and not run when the README.md file is the only file modified.
Updated crun versions in the test matrix. Removed version "1.6", "1.7", "1.7.1", "1.7.2" of crun and added version "1.8.4":
- crun: ["1.6", "1.7", "1.7.1", "1.7.2"]
+ crun: ["1.8.4"]Removed the exclude section that was excluding certain combinations of crun and wasmedge versions:
- exclude:
- ... (list of crun and wasmedge version exclusions)Changed the version of the actions/checkout action from v2 to v3:
- - uses: actions/checkout@v2
+ - uses: actions/checkout@v3These changes have added file path ignoring for the README.md file, updated the crun version used in the test matrix, removed the exclude section for crun and wasmedge versions, and updated the actions/checkout action version.
Below are the potential issues observed in the source code:
In these lines:
env:
WASMEDGE_VERSION: ${{ matrix.wasmedge }}
CRUN_VERSION: ${{ matrix.crun }}It's important to ensure that these variables do not contain sensitive information. If they do, it is advised to use GitHub Encrypted Secrets to store these values securely.
The job has a step that deliberately sleeps for 1200 seconds (20 minutes):
- name: Sleep for 1200s
run: sleep 1200s
shell: bashThis could lead to longer job running times if not done correctly. Consider adding the timeout-minutes attribute to your job definition to avoid potential issues with job timeouts:
jobs:
k8s_crio_server:
timeout-minutes: 120During the installation and starting of k8s, the output is redirected to a log file and not shown in real-time:
- name: Installing and starting k8s
run: |
bash kubernetes_crio/install.sh > k8s.log 2>&1This can make debugging difficult during the execution. Instead, consider using the tee command to log the output while also displaying it in real-time:
- name: Installing and starting k8s
run: |
bash kubernetes_crio/install.sh 2>&1 | tee k8s.logThe workflow job has a step that sleeps for 1200 seconds:
- name: Sleep for 1200s
run: sleep 1200s
shell: bashThis might be necessary in some cases, but if possible, consider reducing the waiting time or use other mechanisms, such as checking if a condition is met or a service has started.
The scheduled cron job uses a hardcoded value:
schedule:
- cron: "0 0 */1 * *"Consider parameterizing the cron schedule as an input variable, so it can be easily adjusted without modifying the code.
Some steps look like they can be combined, such as Dump the log of k8s setup and Installing and starting k8s. Consider combining the steps to make the workflow more readable and to avoid potential errors due to separated steps.
The key changes in the provided patch are as follows:
paths-ignore for both push and pull_request triggers to ignore changes to the README.md:push:
branches: [ main ]
paths-ignore:
- '**/README.md'
pull_request:
branches: [ main ]
paths-ignore:
- '**/README.md'crun matrix to use only version "1.8.4", removing "1.6", "1.7", "1.7.1", and "1.7.2":matrix:
crun: ["1.8.4"]exclude section that contained specific crun versions and corresponding wasmedge versions. The specified crun versions (1.7.1 and 1.7.2) introduced a new AppArmor setting, which caused failures in the custom Wasm handler:exclude:
...
- crun: "1.7.1"
wasmedge: "0.11.0"
...
- crun: "1.7.2"
wasmedge: "0.11.2"
...actions/checkout action from version 2 to version 3:steps:
- uses: actions/checkout@v3
with:
fetch-depth: 0Here's a general code review for the provided GitHub Actions workflow:
The workflow is using a matrix strategy for version combinations of wasmedge and crun, and also includes a job with the latest versions. This is a good practice for test coverage.
The jobs have a clear naming convention for their matrix values, which makes it easy to identify the test scenarios.
The workflow is using paths-ignore for README.md in the push and pull_request triggers, which prevents unnecessary CI runs.
There are separate install steps for specific and latest versions of CRI-O and crun. Steps could be combined into a single one using different inputs for the install script. However, the separate steps improve readability.
The Installing and starting k8s step does not provide any output during the execution, as logs are redirected to a file k8s.log. If it encounters any error, we won't be able to monitor it until the next step, Dump the log of k8s setup.
A long sleep of 1200 seconds is hardcoded in the workflow, which may slow down the CI. If this sleep is necessary, it would be better to have a conditional or looping mechanism checking the desired state instead of a fixed sleep time.
Separate steps for logging and checking the result allow for better visibility of errors with logs dumped before the check step.
Recommendations:
Install CRI-O and crun (specified version) and Install CRI-O and crun (latest version) steps into a single one with different input variables for the install script, if possible.Overall, the workflow code looks well-structured and easy to read. The steps and annotations are clear and follow a logical sequence.
The key changes in the patch are:
Added paths-ignore for README.md in the push trigger. This change prevents unnecessary CI runs when only the README.md file is modified in a push.
Updated the crun version matrix from ["1.6", "1.7", "1.7.1", "1.7.2"] to ["1.8.4"]. The patch removes the previous crun versions and tests only with the new crun version 1.8.4.
Removed the exclude section, which excluded specific combinations of crun and wasmedge versions that would fail the wasm custom handler. This is no longer needed since the crun version matrix has been updated to include only version 1.8.4.
Updated the actions/checkout action version from v2 to v3. This update provides the latest features and improvements of the checkout action.
actions/checkout@v3 does not exist, as the latest version is v2. Replace it with the correct one:- uses: actions/checkout@v2k8s_kwasm_server job: env:
LOG_LEVEL: ${{ github.event.inputs.logLevel }}After this, you can modify your application code or container configuration to read the log level from the environment variable.
- run: |
result=$(curl -w "%{http_code}" -d "name=WasmEdge" -X POST http://localhost:8080)
echo $result
if [[ $result != *"200"* ]]; then
echo "Error: Failed Step"
exit 1
fiThis will print the result of the curl command along with the HTTP status code. If the status code is not 200 (OK), the pipeline will fail and notify of the error.
kubectl port-forward process and kill that process at the end of the job to prevent it from lingering:- run: |
kubectl port-forward service/demo-service 8080 &
port_forward_pid=$!
sleep 10
# ... the rest of your commands ...
kill $port_forward_pidWith these suggested improvements, the updated YAML script would look like:
name: k8s kwasm http server
concurrency:
group: k8s-kwasm-server-${{ github.head_ref }}
cancel-in-progress: true
on:
workflow_dispatch:
inputs:
logLevel:
description: 'Log level'
required: true
default: 'info'
push:
branches: [ main ]
paths-ignore:
- '**/README.md'
pull_request:
branches: [ main ]
paths-ignore:
- '**/README.md'
schedule:
- cron: "0 0 */1 * *"
jobs:
k8s_kwasm_server:
runs-on: ubuntu-latest
env:
LOG_LEVEL: ${{ github.event.inputs.logLevel }}
steps:
- uses: actions/checkout@v2
with:
fetch-depth: 0
- uses: helm/kind-action@v1.4.0
- run: |
helm repo add kwasm http://kwasm.sh/kwasm-operator/
helm repo update
helm install -n kwasm --create-namespace kwasm kwasm/kwasm-operator
kubectl annotate node --all kwasm.sh/kwasm-node=true
kubectl apply -f kwasm/http-server/http_server_application.yaml
sleep 30
kubectl port-forward service/demo-service 8080 &
port_forward_pid=$!
sleep 10
result=$(curl -w "%{http_code}" -d "name=WasmEdge" -X POST http://localhost:8080)
echo $result
if [[ $result != *"200"* ]]; then
echo "Error: Failed Step"
exit 1
fi
kill $port_forward_pid
echoThis patch introduces a GitHub Actions workflow called "k8s kwasm http server". Key changes and steps in the workflow are:
Workflow triggers:
The workflow has one job named "k8s_kwasm_server" running on the latest Ubuntu runner with the following steps:
actions/checkout@v3. (note: this should be changed to actions/checkout@v2 as v3 does not exist)helm/kind-action@v1.4.0.However, I recommend implementing the improvements suggested in my earlier response to make the workflow more robust and efficient.
Overall, this GitHub Actions workflow is well-structured and follows best practices. However, there are a few potential issues and improvements to consider:
Fetch depth:
In the actions/checkout@v3 action, you've specified fetch-depth: 0, which fetches all history for all branches and tags. This can result in a longer checkout time. If you don't need the entire history, you can change the fetch depth to a lower value e.g., fetch-depth: 1, which will only fetch the latest commit.
Helm chart versioning:
You are using the latest version of the Helm chart by not specifying a chart version: helm install -n kwasm --create-namespace kwasm kwasm/kwasm-operator. It is recommended to pin the chart version to ensure your deployments are consistent and predictable. To pin the chart version, you can add --version=<chart_version>.
Use specific Kubernetes node for Jobs:
Instead of adding an annotation to all nodes, it is a good idea to target specific nodes if you want the job to run on only one of them. To accomplish this, add a .spec.template.spec.nodeName field to the Job in kwasm/simple_wasi_application.yaml specifying the target node name where you want the job to run.
Example:
spec:
template:
metadata:
name: wasm-demo
spec:
nodeName: my-targeted-node
...Timeout for sleep command:
Instead of using a fixed sleep duration of 30 seconds, you can use kubectl wait to block until the Job has finished successfully or failed:
Example:
kubectl wait --for=condition=complete --timeout=120s job/wasm-demoThis command will wait for the Job to complete up to 120 seconds (adjust the timeout duration as needed). If the Job does not complete within the specified time, the step will fail and the workflow will stop.
Error handling:
Consider using the -o yaml flag for various shell commands like helm install, kubectl apply, and kubectl logs to capture output as YAML, and then parse or process the output more easily. This will make error handling more robust if an error occurs when running any of these commands.
The patch outlines a GitHub Actions workflow for a Kubernetes (k8s) WebAssembly (Wasm) test. Key changes and configurations in the workflow are:
k8s-kwasm-${{ github.head_ref }}) and cancel in-progress runs if a new push or pull request is created on the same branch.main branch, pull requests to the main branch, and a schedule running every day at midnight.README.md files are ignored from triggering the workflow.k8s_kwasm) that runs on the latest Ubuntu version.actions/checkout@v3 with fetch depth 0 (checkout all branches and tags).helm/kind-action@v1.4.0.kwasm Helm repository, update the repository list, and install the kwasm-operator Helm chart.kwasm.sh/kwasm-node=true to enable them for Wasm deployment.kwasm/simple_wasi_application.yaml.wasm-demo job using kubectl logs.Overall, the code looks good with a few recommendations for improvement. Here's a review of the key areas of the provided YAML file:
Consistency in API versions:
Consider using consistent API versions throughout your manifest file. While it might not cause an issue in most cases, it's better to use a consistent API version for organizational and readability purposes.
Change:
apiVersion: node.k8s.io/v1To:
apiVersion: node.k8s.io/v1beta1Add resources limits and requests:
While not strictly mandatory, it's a best practice to include resource limits and requests for your containers to prevent resource over-allocation and ensure stable performance.
Add this block under the demo-container definition:
resources:
limits:
cpu: "1"
memory: 256Mi
requests:
cpu: 100m
memory: 128MiAdd liveness and readiness probes:
To ensure that your container is healthy and ready to accept traffic, it's recommended to add liveness and readiness probes.
Add this block under the demo-container definition:
livenessProbe:
httpGet:
path: /healthz
port: 1234
readinessProbe:
httpGet:
path: /ready
port: 1234Make sure to replace the /healthz and /ready URIs with the correct endpoints for your application.
With these changes, a more optimized YAML configuration would look like:
apiVersion: node.k8s.io/v1beta1
kind: RuntimeClass
metadata:
name: crun
handler: crun
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: demo-deployment
spec:
replicas: 2
selector:
matchLabels:
app: demo-app
template:
metadata:
labels:
app: demo-app
annotations:
module.wasm.image/variant: compat-smart
spec:
containers:
- name: demo-container
image: wasmedge/example-wasi-http:latest
ports:
- containerPort: 1234
resources:
limits:
cpu: "1"
memory: 256Mi
requests:
cpu: 100m
memory: 128Mi
livenessProbe:
httpGet:
path: /healthz
port: 1234
readinessProbe:
httpGet:
path: /ready
port: 1234
runtimeClassName: crun
---
apiVersion: v1
kind: Service
metadata:
name: demo-service
spec:
type: LoadBalancer
selector:
app: demo-app
ports:
- protocol: TCP
port: 8080
targetPort: 1234The patch contains a Kubernetes configuration file, which sets up a RuntimeClass, a Deployment, and a Service. Key changes in the patch include:
RuntimeClass named "crun" with the handler set to "crun".Deployment named "demo-deployment" with the following specifications:
Service named "demo-service" with the following specifications:
Incomplete or missing comments: Although this source code is in YAML format and used for Kubernetes configuration, it's still a good practice to add comments to explain the purpose of each section and configuration options. This will help in understanding the configuration and make future modifications easier.
Hardcoded values: In the command option, there are hardcoded values like "50000000". These values should ideally be passed through variables, using ConfigMaps or Secrets, or at least explained in the comments.
Image tag: It's better to use specific image tags rather than using "latest" since "latest" can introduce potential breaking changes unintentionally. Consider using a specific version tag for the image like "wasmedge/example-wasi:v1.0.0" instead of "wasmedge/example-wasi:latest".
Resources Limits and Requests: Resources like CPU and memory are not specified in the container specifications. It is a good practice to set resource limits and requests to avoid unexpected resource consumption and help in better scheduling of Pods by the Kubernetes Scheduler.
Use of restartPolicy: Never: This configuration specifies that the container should not be restarted even in the case of failure. Depending on the use-case, you might want to either allow Kubernetes to restart the container in case of failure or control it using backoff limits.
Other than these points, the source code appears to be well-formatted YAML and uses a consistent indentation of 2 spaces.
This patch introduces the following key changes:
RuntimeClass with the name "crun" and handler "crun". This allows specifying a custom runtime for running containers in the Kubernetes cluster.Job called "wasm-demo" with the following specifications:
backoffLimit of the job to 1, which means that Kubernetes will retry the job once after an initial failure.Additionally, the metadata section includes an annotation for the Wasm image variant ("compat-smart") and placeholder timestamps for creation.
cc https://github.com/second-state/wasmedge-containers-examples/pull/29
Hello, I am a serverless review bot on flows.network. Here are my reviews of changed source code files in this PR.
.github/workflows/containerd-server.yml
Upon reviewing the code snippet, I identified a few issues:
actions/checkout@v3, but there is no stable versionv3available. Update this action version tov2. Change the step like this:Aside from those issues, the overall logic seems to be working as intended. However, keep in mind that this review doesn't cover the actual shell scripts (
install.shandhttp_server_application.sh). These files should be reviewed separately for proper error handling, security, and performance issues.Here is a summary of the key changes in the patch:
paths-ignorefor bothpushandpull_requesttriggers, ignoring changes in anyREADME.mdfile. This prevents the workflow from running when only the README has been updated.crunversion matrix to only include version "1.8.4", removing previous versions "1.6", "1.7", "1.7.1", and "1.7.2".actions/checkoutfromv2tov3. However, as mentioned in my previous response, there is no stablev3version available. You should revert this change and keep usingactions/checkout@v2..github/workflows/containerd.yml
Here is a summary of the key changes in the patch:
paths-ignorefor bothpushandpull_requesttriggers, ignoring changes in anyREADME.mdfile. This prevents the workflow from running when only the README has been updated.crunversion matrix to only include version "1.8.4", removing previous versions "1.6", "1.7", "1.7.1", and "1.7.2".actions/checkoutfromv2tov3. However, as mentioned in my previous analysis, there is no stablev3version available. You should revert this change and keep usingactions/checkout@v2..github/workflows/crio-server.yml
Here is a summary of the key changes in the patch:
paths-ignorefor bothpushandpull_requesttriggers, ignoring changes in anyREADME.mdfile. This prevents the workflow from running when only the README has been updated.crunversion matrix to only include version "1.8.4", removing previous versions "1.6", "1.7", "1.7.1", and "1.7.2".excludesection that was preventing certain combinations ofcrunandwasmedgeversions from running due to apparmor settings.actions/checkoutfromv2tov3. However, as mentioned in my previous analyses, there is no stablev3version available. You should revert this change and keep usingactions/checkout@v2..github/workflows/crio.yml
Here is a summary of the key changes in the patch:
paths-ignorefor bothpushandpull_requesttriggers, ignoring changes in anyREADME.mdfile. This prevents the workflow from running when only the README has been updated.crunversion matrix to only include version "1.8.4", removing previous versions "1.6", "1.7", "1.7.1", and "1.7.2".excludesection that was preventing certain combinations ofcrunandwasmedgeversions from running due to apparmor settings.actions/checkoutfromv2tov3. However, as mentioned in my previous analyses, there is no stablev3version available. You should revert this change and keep usingactions/checkout@v2..github/workflows/kubernetes-containerd-server.yml
Overall, the provided code snippet seems to be a GitHub Actions workflow, which is used for building, testing, and deploying code in a CI/CD environment. Here are a few potential issues and suggestions:
actions/checkout@v3in theusessection does not exist. At the time of writing this, the latest version of the action isv2. Instead, you should useactions/checkout@v2.envvariableACCEPT_EULAis not required for updates, upgrades or installations withapt-get. You can remove those lines:Ensure that the
containerd/install.shandkubernetes_containerd/install.shscripts are in your repository, as they are being called during the workflow.For better readability and maintainability, consider splitting the combined steps that dump logs and check results in the end, into separate steps:
actions/cache@v2in your workflow to cache dependencies. Please refer to the documentation for more details.Other than the above issues, the provided code snippet seems to be formatted well and follows good practices.
This patch introduces several key changes to the original GitHub Actions workflow:
For both
pushandpull_requestevents,paths-ignorehas been added to avoid triggering the workflow when only theREADME.mdfile is modified.The
crunmatrix has been simplified by removing multiple versions and keeping only1.8.4. Theexcludesection has been removed as it is no longer necessary.The
actions/checkoutversion has been updated fromv2tov3. However, it's important to note that, at the time of writing this, the latest version ofactions/checkoutisv2. So, you should useactions/checkout@v2instead ofactions/checkout@v3.Overall, these changes help streamline the workflow configuration and avoid unnecessary triggers.
.github/workflows/kubernetes-containerd.yml
The code snippet provided appears to be a YAML configuration file for a GitHub Actions workflow.
Overall, the workflow is well-structured, and I didn't find any major issues. However, there are a few minor improvements that can be made:
The workflow name should be enclosed in single or double quotes:
Instead of hard-coding the sleep duration in the 'Sleep for 1200s' step, it's better to use an environment variable or input parameter. This makes the workflow more flexible and maintainable:
Apart from these minor changes, the code looks good for a workflow targeting Kubernetes, containerd, and WasmEdge testing. Note that since the code snippet is not complete, it's important to validate the entire YAML file for correctness and adherence to GitHub Actions workflow syntax.
This patch introduces the following key changes to the GitHub Actions workflow configuration:
Added paths-ignore configuration to both the push and pull_request triggers. This will ignore changes to the README.md file and not trigger the workflow for those changes.
Updated the crun version matrix, removing versions "1.6", "1.7", "1.7.1", and "1.7.2", and adding version "1.8.4".
Removed the entire exclude section for the matrix, which previously excluded certain combinations of crun and wasmedge versions that may cause wasm custom handler failures.
Updated the action
actions/checkoutfrom version 2 to version 3..github/workflows/kubernetes-crio-server.yml
This patch introduces the following key changes to the GitHub Actions workflow configuration:
Added paths-ignore configuration to both the push and pull_request triggers. This will ignore changes to the README.md file and not trigger the workflow for those changes.
Updated the crun version matrix, removing versions "1.6", "1.7", "1.7.1", and "1.7.2", and adding version "1.8.4".
Removed the entire exclude section for the matrix, which previously excluded certain combinations of crun and wasmedge versions that may cause wasm custom handler failures.
Added a config key to the matrix with the value "k8s-http_server.yaml". This line didn't exist in the previous patch.
Updated the action
actions/checkoutfrom version 2 to version 3..github/workflows/kubernetes-crio.yml
This patch introduces the following key changes to the GitHub Actions workflow configuration:
Added paths-ignore configuration to both the push and pull_request triggers. This will ignore changes to the README.md file and not trigger the workflow for those changes.
Updated the crun version matrix, removing versions "1.6", "1.7", "1.7.1", and "1.7.2", and adding version "1.8.4".
Removed the entire exclude section for the matrix, which previously excluded certain combinations of crun and wasmedge versions that may cause wasm custom handler failures.
Updated the action
actions/checkoutfrom version 2 to version 3..github/workflows/kwasm-server.yml
This patch introduces a new GitHub Actions workflow named "k8s kwasm http server". The key changes and additions in this configuration are:
The workflow is configured with concurrency settings, which allows only one instance of the workflow to run at a time for a given branch reference.
There are triggers for workflow_dispatch, push, pull_request, and a scheduled cron-based trigger, with paths-ignore logic set to avoid running the workflow when only the README.md file is updated.
The workflow contains a single job named "k8s_kwasm_server" that runs on the latest version of Ubuntu.
The job uses the "actions/checkout@v3" action with a fetch-depth of 0 and the "helm/kind-action@v1.4.0" action.
The job installs the kwasm operator in a Kubernetes cluster using Helm, annotates the node, and applies the http_server_application.yaml configuration.
The job waits for 30 seconds, sets up port-forwarding, and waits for another 10 seconds.
Finally, the job runs a curl command with a POST request to the HTTP server, sending the "name=WasmEdge" data.
.github/workflows/kwasm.yml
This patch introduces a new GitHub Actions workflow called "k8s kwasm test". Key changes and features include:
workflow_dispatchevent with alogLevelinput,pushandpull_requestevents for themainbranch (ignoringREADME.mdchanges), and a scheduled cron job that runs once per day.ubuntu-latestenvironment and consists of several steps:actions/checkout@v3action with afetch-depthof 0.helm/kind-action@v1.4.0action.helm repo add kwasm http://kwasm.sh/kwasm-operator/andhelm repo update.helm install -n kwasm --create-namespace kwasm kwasm/kwasm-operator.kubectl annotate node --all kwasm.sh/kwasm-node=true.kubectl apply -f kwasm/simple_wasi_application.yaml.sleep 30) to allow for resource allocation.kubectl logs job/wasm-demo.kwasm/http-server/http_server_application.yaml
The code snippet appears to be a Kubernetes YAML configuration for a deployment and a service using a WebAssembly application. While there are no apparent syntax errors, there are some details worth pointing out:
Runtime Class: The
RuntimeClassnamedcrunmight not be available on the Kubernetes cluster. Before applying this configuration, make sure the cluster has thecrunruntime class configured. If the class is not available, the Kubernetes cluster might fail to create the pod or use a different runtime handler.Container Image: The container image
wasmedge/example-wasi-http:latestmay not always be the most stable release. It is advisable to use a specific version, such aswasmedge/example-wasi-http:<version-number>, to ensure long-term stability and minimize potential issues.LoadBalancer Service: The service is configured to use a LoadBalancer; this can lead to undesired results if applied to a local development environment, and it might not even work. Deployment in a cluster with a cloud provider that supports the LoadBalancer type should not face any issues.
Resource Limits: There is no specification of resource limits or requests in the container. It is good practice to specify the resources (CPU and memory) required and limits for the container to prevent excessive resource consumption.
Liveness and Readiness Probes: The container configuration lacks both liveness and readiness probes. Adding these probes can help ensure that the container is ready to accept requests and is functioning correctly.
Overall, the YAML configuration seems functional, but addressing the mentioned details can ensure better stability and compatibility.
The patch introduces a Kubernetes YAML configuration with the following key components:
RuntimeClass: A custom
RuntimeClassnamedcrunis defined with the handler specified ascrun.Deployment: A
Deploymentnameddemo-deploymentis defined with the following attributes:app: demo-app.demo-containerusing thewasmedge/example-wasi-http:latestimage, listening on port 1234.crunRuntimeClass.Service: A
Servicenameddemo-serviceis defined with the following attributes:LoadBalancertype.app: demo-app.kwasm/simple_wasi_application.yaml
Here are some potential issues in this Kubernetes configuration:
The
creationTimestamp: nullfields undermetadatafor bothJoband itstemplateare not necessary and can be removed. Kubernetes will automatically populate this field when the object is created.The
resourcesfield under thewasm-democontainer is empty. This could result in suboptimal resource allocation for the container. It is recommended to specify resource limits and requests for better resource management, like this:.wasmfile with an argument50000000. If this value needs to be changed or updated frequently, it's a good idea to use ConfigMap or Secrets to store this value and reference it in the Kubernetes configuration.Other than these issues, the configuration appears to be correctly set up. The snippet is only a small part of a bigger configuration, so be sure to check other parts for any potential problems.
This patch consists of a Kubernetes configuration that defines two resources: a
RuntimeClasswith the name "crun" and aJobwith the name "wasm-demo".Key changes made in this patch:
RuntimeClassnamed "crun" with a handler "crun".wasmedge/example-wasi:latestwith the name "wasm-demo"./wasi_example_main.wasmwith an argument50000000.backoffLimitfor the Job is set to 1.module.wasm.image/variant: compat-smartfor the pod template.cc https://github.com/second-state/wasmedge-containers-examples/pull/29