kubernetes: clean up temporary provisioning scripts on the management server#13592
Conversation
… server The CKS action workers copy provisioning scripts (deploy-cloudstack-secret, deploy-provider, deploy-csi-driver, delete-pv-reclaimpolicy-delete, autoscale-kube-cluster and upgrade-kubernetes.sh) to the cluster nodes via retrieveScriptFiles() + copyScripts()/copyScriptFile(). Each script is written to a local temp file via File.createTempFile(), but that local copy is never deleted after it has been SCP'd to the node. As a result every cluster deploy, provider/CSI deploy, autoscale enable, PV cleanup and Kubernetes upgrade leaks one *.sh file per script into the management server's temp directory, growing without bound over the cluster lifetime. On a long-running management server this accumulates to hundreds of stale scripts in /tmp. This adds cleanupScriptFiles()/deleteScriptFileQuietly() to the base action worker and invokes it in a finally block around every copy site (deployProvider, deployCsiDriver, autoscaleCluster, deletePVsWithReclaimPolicyDelete and the upgrade worker), so the local temp copies are removed once they have been transferred to the node. The upgrade worker additionally removes its upgrade-kubernetes.sh temp file after the node loop completes. No functional change to the provisioning itself: the scripts are still written, copied and executed exactly as before; only the leftover local copies are deleted. Covered by new unit tests for cleanupScriptFiles()/deleteScriptFileQuietly() (temp files deleted, null/missing files handled without throwing).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13592 +/- ##
=========================================
Coverage 19.47% 19.47%
- Complexity 19351 19355 +4
=========================================
Files 6296 6296
Lines 568368 568394 +26
Branches 69622 69622
=========================================
+ Hits 110673 110717 +44
+ Misses 445618 445597 -21
- Partials 12077 12080 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18588 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
| } | ||
|
|
||
| @Override | ||
| protected void cleanupScriptFiles() { |
There was a problem hiding this comment.
Doesn't this delete upgradeScriptFile too early? deployProvider() runs per node in the loop and calls cleanupScriptFiles() in its own finally, and since this is an UpgradeWorker, that hits this override and deletes upgradeScriptFile before runInstallScriptOnVM() (next line) SCPs it. Could this break the upgrade the first time a node needs the provider re-deployed?
| deleteScriptFileQuietly(autoscaleScriptFile); | ||
| } | ||
|
|
||
| protected void deleteScriptFileQuietly(File file) { |
There was a problem hiding this comment.
Should the field be set to null after deleting? Right now it still points to a file that no longer exists. If it's used again before retrieveScriptFiles() runs, it could silently try to work with a deleted file.
| logMessage(Level.INFO, "Provider files missing. Adding them now", null); | ||
| retrieveScriptFiles(); | ||
| copyScripts(publicIpAddress, sshPort); | ||
| try { |
There was a problem hiding this comment.
This same try { copyScripts(...) } finally { cleanupScriptFiles(); } block shows up 3 times (here, deployCsiDriver(), and autoscaleCluster()). Could this be a small shared helper, like copyScriptsWithCleanup(nodeAddress, sshPort), to avoid repeating it?
| * once they have been copied to the cluster node(s). Without this, every deploy/autoscale/PV-cleanup | ||
| * action leaks a *.sh file in the management server's tmp directory. | ||
| */ | ||
| protected void cleanupScriptFiles() { |
There was a problem hiding this comment.
Nit: could this loop over a list of the 5 fields instead of calling deleteScriptFileQuietly 5 times by hand? Would be one line instead of five, and less to update if a new script field gets added later.
|
|
||
| @Test | ||
| public void testCleanupScriptFilesDeletesAllTempFiles() throws Exception { | ||
| actionWorker.deploySecretsScriptFile = File.createTempFile("deploy-cloudstack-secret", ".sh"); |
There was a problem hiding this comment.
Nit: these strings duplicate actionWorker.deploySecretsScriptFilename and friends. Could reuse those constants instead?
|
[SF] Trillian test result (tid-16587)
|
5 participants
Description
The CKS action workers copy provisioning scripts to the cluster nodes via
retrieveScriptFiles()+copyScripts()/copyScriptFile(). Each script iswritten to a local temp file with
File.createTempFile(filename, ".sh")inretrieveScriptFile(), but that local copy on the management server is neverdeleted after it has been SCP'd to the node.
As a result, every cluster deploy, provider/CSI deploy, autoscale enable, PV
cleanup and Kubernetes upgrade leaks one
*.shfile per script into themanagement server's temp directory, growing without bound over the cluster
lifetime:
This PR adds
cleanupScriptFiles()/deleteScriptFileQuietly()to the baseKubernetesClusterActionWorkerand invokes it in afinallyblock aroundevery copy site:
KubernetesClusterActionWorker.deployProvider()KubernetesClusterActionWorker.deployCsiDriver()KubernetesClusterResourceModifierActionWorker.autoscaleCluster()KubernetesClusterResourceModifierActionWorker.deletePVsWithReclaimPolicyDelete()KubernetesClusterUpgradeWorker(overridescleanupScriptFiles()to also removeits
upgrade-kubernetes.shtemp file, cleaned up after the per-node upgrade loopcompletes since the same file is reused across nodes)
There is no functional change to the provisioning itself: the scripts are still
written, copied and executed exactly as before; only the leftover local copies
on the management server are removed once they have been transferred.
Types of changes
Bug Severity
Observed on a CloudStack 4.22.1 management server running CKS clusters; the code
path is unchanged on
main.