-
Notifications
You must be signed in to change notification settings - Fork 216
Update aks sample script with Azure NFS share for storage #2726
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
Conversation
Changes to be committed: modified: kubernetes/samples/scripts/common/utility.sh new file: kubernetes/samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/azure-csi-storageaccount-template.yaml deleted: kubernetes/samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/azure-file-pv-template.yaml modified: kubernetes/samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/azure-file-pvc-template.yaml modified: kubernetes/samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/create-domain-on-aks-inputs.yaml modified: kubernetes/samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/create-domain-on-aks.sh modified: kubernetes/samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/validate.sh Signed-off-by: galiacheng <haixia.cheng@microsoft.com>
|
@galiacheng Please assign reviewers for this PR... |
|
Hello @robertpatrick I cannot select any reviewer, could you assign the pr to @rjeberhard and @mriccell, thank you! |
tbarnes-us
left a comment
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 only have minor comments. They don't need to block the pull, but you may want to follow up on them.
| attempts=$((attempts + 1)) | ||
| sleep 1 | ||
| pvc_state=`kubectl get pvc $1 -o jsonpath='{.status.phase}'` | ||
| done |
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.
just a couple of minor comments:
-
You can check "SECONDS" checks instead of using "attempt count" math to implement timeouts in shell, since SECONDS contains the time since the script started. ("local end_secs=$(SECONDS + 10)" and "[ $SECONDS -le $end_secs]")
-
attempts should be declared local
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.
Cool, I love the SECONDS method, it will be more accurate.
| nameAvailable=$(echo "$ret" | grep "nameAvailable" | grep "false") | ||
| if [ -n "$nameAvailable" ]; then | ||
| echo $ret | ||
| fail "Storage account ${aksClusterName} is unavaliable." |
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.
Minor: unavaliable --> unavailable
| --name ${azureStorageShareName} \ | ||
| --enabled-protocol NFS \ | ||
| --root-squash NoRootSquash \ | ||
| --quota 100 |
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.
Minor: If you want to aid with tracing/debugging throughout, maybe consider pattern like:
local cmd="az storage share-rm create ... --quota 100"
echo "Running cmd '$cmd'"
$cmd
Since az is used so frequently, you could make a helper function that does this:
runCmd () {
echo "about to run '$*'"
$*
}
runCmd ls /tmp/*.log ~/*.sh
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.
This is a very useful idea to deal with the commands in a same pattern.
local cmd="..." get the output message, while we want to keep the progress message during the az command is running, I will keep the script as it's.
|
|
||
| function configureStorageAccountNetwork { | ||
| # get the resource group name of the AKS managed resources | ||
| aksMCRGName=$(az aks show --name $aksClusterName --resource-group $azureResourceGroupName -o tsv --query "nodeResourceGroup") |
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.
minor: define all local variables as "local"
| attempts=0 | ||
| svcState="running" | ||
| while [ ! "$svcState" == "completed" ] && [ ! $attempts -eq 30 ]; do | ||
| attempts=0 |
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.
minor: local
582830c to
6139be2
Compare
Signed-off-by: galiacheng <haixia.cheng@microsoft.com> Changes to be committed: modified: samples/scripts/common/utility.sh modified: samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/create-domain-on-aks.sh
Signed-off-by: galiacheng <haixia.cheng@microsoft.com> Changes to be committed: modified: samples/scripts/common/utility.sh
6139be2 to
6fbc52f
Compare
Update aks sample script with Azure NFS share for storage
Update aks sample script with Azure NFS share for storage
This pr includes script changes to v3.3 for Azure NFS share in the domain on a PV sample of AKS.
Workflow to test the script: https://github.com/galiacheng/weblogic-kubernetes-operator/runs/4916604379?check_suite_focus=true
A live doc for review: https://zealous-dune-0dac9090f.1.azurestaticapps.net/samples/azure-kubernetes-service/domain-on-pv
Changes to be committed:
modified: kubernetes/samples/scripts/common/utility.sh
new file: kubernetes/samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/azure-csi-storageaccount-template.yaml
deleted: kubernetes/samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/azure-file-pv-template.yaml
modified: kubernetes/samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/azure-file-pvc-template.yaml
modified: kubernetes/samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/create-domain-on-aks-inputs.yaml
modified: kubernetes/samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/create-domain-on-aks.sh
modified: kubernetes/samples/scripts/create-weblogic-domain-on-azure-kubernetes-service/validate.sh
Signed-off-by: galiacheng haixia.cheng@microsoft.com