bhouse-nexthop opened a new pull request, #14598:
URL: https://github.com/apache/cloudstack/pull/14598
### Description
This PR fixes the download and option handling in three of the scripts CKS
copies to cluster nodes: `deploy-csi-driver`, `deploy-provider` and
`autoscale-kube-cluster`.
Fixes #14597
#### What was wrong
- **`deploy-csi-driver` never fetched `manifest.yaml`.** When the node has
no CSI manifests from the binaries ISO, the script downloads them. Its loop
applied both files and exited after downloading only the first
(`snapshot-crds.yaml`), so the apply of `manifest.yaml` always failed. The
`snapshot-crds.yaml` it left behind then sent every later run down the "files
already present" branch, which failed the same way.
- **HTTP errors were saved and applied as manifests.** All three scripts
used `curl -sSL` without `-f`, so a 404 (a moved or missing URL) was written to
the manifest file and applied. The bad file stayed under `/opt` and was applied
again on the next run without re-downloading. The `if [ $? -ne 0 ]` checks
after `curl` couldn't report anything: the scripts run under `#!/bin/bash -e`,
so a failing `curl` ended the script before the check.
- **`autoscale-kube-cluster --disable` was rejected.** The option was
declared as `-d | --enable)`, so `--disable` failed as an unknown option (`-d`
worked, and is what CloudStack passes). With neither `-e` nor `-d`, the
unquoted `[ $ENABLE == "true" ]` failed with "unary operator expected" and fell
through to deleting the autoscaler.
- **Disabling failed when there was no autoscaler to delete** (for example
after a failed enable): `kubectl delete deployment` returned NotFound under
`bash -e`, so the API call failed and autoscaling could not be turned off from
CloudStack.
#### How it is fixed
- **`deploy-csi-driver`:** downloads every file, then applies them the way
the ISO path does (CRDs, `sleep 5`, manifest). It treats the files as present
only when both `snapshot-crds.yaml` and `manifest.yaml` exist, so a node left
with only `snapshot-crds.yaml` (by the old script, or by a failed download)
downloads again and recovers.
- **All three downloads:** `curl -fsSL` inside the `if`, writing to a
per-run temporary file (`<file>.tmp.$$`, removed by an `EXIT` trap) that is
moved into place only once it is complete. A failed or interrupted download
leaves no file that a later run would apply, and is retried on the next run
with the existing error message.
- **`autoscale-kube-cluster`:** accepts `--disable`, quotes the test, and
exits with an error when neither `-e` nor `-d` is given, instead of deleting
the autoscaler. CloudStack always passes one of them
(`KubernetesClusterResourceModifierActionWorker`), so its calls behave as
before. The delete uses `--ignore-not-found`, so disabling succeeds when the
autoscaler isn't deployed.
Not changed here:
- The autoscaler and provider manifests are still fetched from
`master`/`main`. Pinning them is separate work.
- `scripts/util/create-kubernetes-binaries-iso.sh` has the same `curl`
without `-f` for the manifests it bundles into the ISO. That is a build-time
script with its own cleanup questions, so it is left for a separate change.
- A node that already holds an error page under `/opt` from the old scripts
still applies it; delete the file to recover.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [x] Minor
- [ ] Trivial
### How Has This Been Tested?
- Ran the old and new scripts side by side with `/opt` redirected to a
temporary directory, a stub `curl` (succeeds, or fails a chosen URL with HTTP
404) and a stub `kubectl` (records calls, fails `apply` of a missing file):
| Case | Before | After |
|---|---|---|
| `deploy-csi-driver`, no ISO files, downloads work | downloads
`snapshot-crds.yaml` only; `apply manifest.yaml` fails, exit 1 | downloads
both, applies CRDs then manifest, exit 0 |
| same, `manifest.yaml` returns 404 | same failure as above | error
message, exit 1, no temporary files left; the next run downloads both again |
| only `snapshot-crds.yaml` left from an earlier run | applies it, then
fails on `manifest.yaml` | re-downloads both and applies them |
| `deploy-provider` / `autoscale-kube-cluster -e`, URL returns 404 | error
page saved under `/opt` and applied | error message, exit 1, nothing saved |
| `autoscale-kube-cluster --disable` | "no such option --disable", exit 1
| deletes the autoscaler deployment (`--ignore-not-found`) |
| `autoscale-kube-cluster -d`, `-e -M 3 -m 1` (as CloudStack calls them) |
works | works, same commands |
| `autoscale-kube-cluster -i x` (neither flag) | "unary operator
expected", then deletes the autoscaler | error, exit 1, nothing deleted |
- `bash -n` and `shellcheck` on the three scripts: the new versions are
clean. The old ones had SC2181 (`$?` check), SC2086 (unquoted) and
SC2221/SC2222 (`-d | --enable` shadowed by `-e | --enable`).
- Not tested on a live CKS cluster.
#### How did you try to break this feature and the system with this change?
- **Nodes with the manifests from the ISO:** unchanged.
`k8s-control-node.yml` copies both CSI files together, so the "both files
present" check still takes the ISO path.
- **Nodes left in the broken state by the old script** (only
`snapshot-crds.yaml` present): now re-download and recover instead of failing
forever.
- **Interrupted download** (e.g. the 60 s SSH timeout): the real file is
never half-written, and the `EXIT` trap removes the temporary file (bash runs
it on TERM/HUP too).
- **Two runs at once** (a retry while the first is still running): each
writes its own `.tmp.$$` file, and `mv` replaces the target in one step.
- **Other open PRs:** #13592 (cleaning up these scripts on the management
server) only touches Java files, so it doesn't conflict.
- **CloudStack's own calls** (`-d`; `-i <id> -e -M <max> -m <min>`):
unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
