Improve patchComponent && Add patchCluster scripts #12207

todor-ivanov · 2024-12-13T11:29:22Z

Status

ready

Description

With the current PR we improve the functionality of patchComponent.sh script adding the following functionalities:

The ability to find the correct WMCore codebase destination - regardless if running from pypi or from Source
The ability to recognize the WMCore version currently deployed at the destination
The ability to parse the patch file to find the full list of files prepared for altering
The ability to "zero" the codeBase of all the files which are about to be patched to the state as they are at the WMCore version currently deployed at the destination
The ability to perform a second patch attempt starting from latest version of the files from the list of files to be altered (by fetching them from upstream/master) if the first attempt to patch from the currently deployed version fails. It is a feature quite needed in the case of completely broken history and/or intermediate developments that might have happened between the moment of forking to a new branch for creating the patch and the actually applying and/or when the origin branch on top of which the patch has been created is behind the branch currently deployed at destination.
Automatically detecting any code conflicts (failed patch steps) and reverting the whole code to the baseline it started from if found any
The ability to patch from local tree (through git * commands) without going through officially creating a PR in the upstream repository
The ability to chose if to skip any of the above actions
The ability to manually revert (only zeroing) the whole code base to the version deployed at the destination
The ability to apply multiple patches in a row (preserving their order)

We also introduce the script patchClustetr.sh, which is capable of applying all those step on a Kuberenetes cluster. The granularity could be:

A whole cluster
A Pod
A Service
A Deployment

This way those two scripts become a well coupled pair, which boosts the development and testing/validation process tremendously!!!

Is it backward compatible (if not, which system it affects?)

YES

Related PRs

None

External dependencies / deployment changes

None

vkuznet

In addition to those I provided through the code I don't know how both scripts handle different error situations, e.g. if patch is already applied, if it is partially succeed? Can it be reversed? It would be useful if you'll clarify these questions as well.

vkuznet · 2024-12-16T13:29:59Z

bin/patchCluster.sh

+# NOTE: We do not support patching from file and patching from command line simultaneously
+#       If both provided at the command line patching from command line takes precedence
+
+podCmdActions="wget https://raw.githubusercontent.com/todor-ivanov/WMCoreAux/master/bin/patchComponent.sh -O /data/patchComponent.sh && chmod 755 /data/patchComponent.sh "


Few things here:

you are assuming that wget is installed on a node which may not be the case. Please add appropriate test and failure for wget (and/or curl) commands

I think you should use back quote to get output of command to assign it to a shell variable. please check

I also suggest to separate wget and chmod command to properly capture wget output

Indeed, wget or curl are requirements. I can try to double check if they are present at the container, but this would make the podCmd* commands executions more complicate.... - would require two extra steps from the caller script, so at least one more cycle there.... The idea here is that they should go in as "one-liners" because they would be pushed through the kubectl connection session as a single command with accumulated errors. So the only way to achieve that is through an && logical chain. I'll see what I can do, though, and will write back the results from my tests here.

so:

I think you should use back quote to get output of command to assign it to a shell variable. please check

Actually not. Because we need to delay the execution as much as possible so it happens at the container. A back quote here would push the execution to happen at the caller's shell. So we better first construct the strings and then send it through the session to the containers.

vkuznet · 2024-12-16T13:31:31Z

bin/patchCluster.sh

+           echo "DEBUG: newPipeFlag=$newPipeFlag"
+           }
+
+podCmd="$podCmdActions && sudo /data/patchComponent.sh $podCmdOpts $patchList "


the same comment here, you are implying that sudo exists which may not be the case.

vkuznet · 2024-12-16T13:32:43Z

bin/patchCluster.sh

+           }
+
+podCmd="$podCmdActions && sudo /data/patchComponent.sh $podCmdOpts $patchList "
+restartCmd="/data/manage restart && sleep 1 && /data/manage status"


here you assume that /data/manage exist, and it is related to our current way of installing which may change in a future. Maybe you should define manage somehow, or at least test that it exists

that is all true.... I will try to include this one in the above mentioned tests as well.

vkuznet · 2024-12-16T13:33:17Z

bin/patchCluster.sh

+echo --------------------------------------------------------
+# First try to find any pod from the service name provided and then extend the list in currPods:
+if [[ -n $currService ]]; then
+    servicePods=`kubectl -n $nameSpace  get ep $currService -o=jsonpath='{.subsets[*].addresses[*].ip}' | tr ' ' '\n' | xargs -I % kubectl -n $nameSpace get pods  -o=name --field-selector=status.podIP=%`


please test that kubectl exists in a user PATH.

same answer... Will try it in my next commit

vkuznet · 2024-12-16T13:36:17Z

bin/patchComponent.sh

+        mkdir -p $pythonLibPath/$fileDir
+        echo INFO: orig: https://raw.githubusercontent.com/dmwm/WMCore/$srcBranch/test/python/$file
+        echo INFO: dest: $pythonLibPath/$file
+        curl -f https://raw.githubusercontent.com/dmwm/WMCore/$srcBranch/test/python/$file  -o $pythonLibPath/$file || {


here you use curl while in patchCluster.sh you rely on wget, why both and not using a single tool. I always prefer curl, so my point is that there is no need to use wget if we use curl or vice versa.

I do not remember the exact reason why I used one tool above the the another one here... I'll test with either of them and if possible I'll stick to one of them

Fix broken testFileList creation

todor-ivanov requested review from vkuznet and anpicci December 13, 2024 11:29

vkuznet requested changes Dec 16, 2024

View reviewed changes

Improve patchComponent && Add patchCluster scripts

2d06401

Fix broken testFileList creation

todor-ivanov force-pushed the feature_Improve_patchScripts_fix-11685 branch from 9b2941b to 2d06401 Compare December 17, 2024 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve patchComponent && Add patchCluster scripts #12207

Improve patchComponent && Add patchCluster scripts #12207

todor-ivanov commented Dec 13, 2024 •

edited

Loading

vkuznet left a comment

vkuznet Dec 16, 2024

todor-ivanov Dec 16, 2024 •

edited

Loading

todor-ivanov Dec 16, 2024

vkuznet Dec 16, 2024

vkuznet Dec 16, 2024

todor-ivanov Dec 16, 2024

vkuznet Dec 16, 2024

todor-ivanov Dec 16, 2024

vkuznet Dec 16, 2024

todor-ivanov Dec 16, 2024

Improve patchComponent && Add patchCluster scripts #12207

Are you sure you want to change the base?

Improve patchComponent && Add patchCluster scripts #12207

Conversation

todor-ivanov commented Dec 13, 2024 • edited Loading

Status

Description

Is it backward compatible (if not, which system it affects?)

Related PRs

External dependencies / deployment changes

vkuznet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

todor-ivanov Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

todor-ivanov commented Dec 13, 2024 •

edited

Loading

todor-ivanov Dec 16, 2024 •

edited

Loading