Add retry for pv creation commands #457

praveenkumar · 2021-07-28T07:16:18Z

This patch will fix following error from CI

./openshift-clients/linux/oc create -f -
error: error when creating "STDIN": Post "https://api.crc.testing:6443/api/v1/persistentvolumes?fieldManager=kubectl-create": dial tcp 192.168.126.11:6443: connect: connection refused

cfergeau · 2021-07-28T16:46:04Z

This patch will fix following error from CI

Is this happening with 4.8 or 4.9?

praveenkumar · 2021-07-29T03:39:47Z

Is this happening with 4.8 or 4.9?

@cfergeau This is from 4.9 side.

praveenkumar · 2021-07-29T05:09:12Z

/retest

praveenkumar · 2021-07-30T03:45:47Z

/test e2e-snc

praveenkumar · 2021-08-02T03:21:41Z

/test e2e-snc

cfergeau · 2021-08-02T16:17:32Z

Not a big fan that some temporary files need to be added, the script is nicer before these changes :(

cfergeau · 2021-08-03T13:18:12Z

snc-library.sh

@@ -170,19 +170,20 @@ function create_pvs() {

    for pvname in $(seq -f "pv%04g" 1 ${count}); do
        if ! ${OC} get pv "${pvname}" &> /dev/null; then
-            generate_pv "${pvdir}/${pvname}" "${pvname}" | ${OC} create -f -
+            generate_pv "${pvdir}/${pvname}" "${pvname}" > tmp_pv.yaml 
+            retry ${OC} create -f tmp_pv.yaml


I think this could work and avoid the need for the temporary file?

retry ${OC} create -f - <<EOF $(generate_pv "${pvdir}/${pvname}" "${pvname}") EOF

cfergeau · 2021-08-03T13:20:44Z

snc-library.sh

        else
            echo "persistentvolume ${pvname} already exists"
        fi
    done

    # Apply registry pvc to bound with pv0001
-    ${OC} apply -f registry_pvc.yaml
+    retry ${OC} apply -f registry_pvc.yaml


Wondering if it's possible to do something like

OC="retry ${OC}"

so that all calls to oc go through a retry. Alternatively, this could be a run_oc function which calls retry ${OC} .... Might be cleaner.

cfergeau · 2021-08-03T13:38:48Z

snc.sh

@@ -191,7 +191,8 @@ retry ${OC} scale --replicas=1 ingresscontroller/default -n openshift-ingress-op
 retry ${OC} patch config.imageregistry.operator.openshift.io/cluster --patch '{"spec":{"defaultRoute":true}}' --type=merge

 # Add a tip in the login page
-retry ${OC} get secrets -n openshift-authentication v4-0-config-system-ocp-branding-template -o json | ${JQ} -r '.data["login.html"]'  | base64 -d > login.html
+retry ${OC} get secrets -n openshift-authentication v4-0-config-system-ocp-branding-template -o json > tmp_secret_template.json
+${JQ} -r '.data["login.html"]' tmp_secret_template.json | base64 -d > login.html


Maybe something like this:

diff --git a/snc.sh b/snc.sh index 4ffe9ad..d3dfaef 100755 --- a/snc.sh +++ b/snc.sh @@ -191,7 +191,7 @@ retry ${OC} scale --replicas=1 ingresscontroller/default -n openshift-ingress-op retry ${OC} patch config.imageregistry.operator.openshift.io/cluster --patch '{"spec":{"defaultRoute":true}}' --type=merge # Add a tip in the login page -retry ${OC} get secrets -n openshift-authentication v4-0-config-system-ocp-branding-template -o json | ${JQ} -r '.data["login.html"]' | base64 -d > login.html +retry_xx /dev/null ${OC} get secrets -n openshift-authentication v4-0-config-system-ocp-branding-template -o json | ${JQ} -r '.data["login.html"]' | base64 -d > login.html ${PATCH} login.html < login.html.patch retry ${OC} create secret generic login-template --from-file=login.html -n openshift-config diff --git a/tools.sh b/tools.sh index 288bd74..2cad5c2 100755 --- a/tools.sh +++ b/tools.sh @@ -88,7 +88,9 @@ if ! which ${PATCH}; then sudo yum -y install /usr/bin/patch fi -function retry { +function retry_xx { + local output=$1 + shift local retries=10 local count=0 until "$@"; do @@ -106,6 +108,10 @@ function retry { return 0 } +function retry { + retry /dev/stdout $@ +} + function get_vm_prefix { local crc_vm_name=$1 # This random_string is created by installer and added to each resource type,

could work.

Alternatively, have you tried something like this:

branding_template=$(retry ${OC} get secrets -n openshift-authentication v4-0-config-system-ocp-branding-template -o json) ${JQ} ...

Actually I'm not sure I understand why the pipe won't work when the redirect does :-/

Actually I'm not sure I understand why the pipe won't work when the redirect does :-/

Pipe and redirect both works but in case of pipe next command execute even with the first command failure but in case of redirect it complete the retry. Following bash script will do retry for 4 times and if you replace redirect with | echo "foobar" it just do once and exit.

#!/bin/bash set -exuo pipefail export LC_ALL=C.UTF-8 export LANG=C.UTF-8 function retry { local retries=4 local count=0 until "$@"; do exit=$? wait=$((2 ** $count)) count=$(($count + 1)) if [ $count -lt $retries ]; then echo "Retry $count/$retries exited $exit, retrying in $wait seconds..." sleep $wait else echo "Retry $count/$retries exited $exit, no more retries left." return $exit fi done return 0 } retry false > tmp_file

cfergeau · 2021-08-04T08:36:21Z

Ah, so the temporary file will also contain Retry ..., retrying in ... seconds... together with the json data, and we are lucky that jq accepts to parse this? Would be better to be able to cleanly separate the logs from retry from the actual json data?

Give more resources to the cluster, we run the testsuite on should hopefully prevent some disruptions/timeouts because of low memory conditions.

This would avoid to have any retry data on a file if we redirect a command output to it and command failed for 1-2 times. ``` retry false > temp_file ```

praveenkumar · 2021-08-05T07:23:21Z

/retest

praveenkumar · 2021-08-05T15:06:53Z

/retest

cfergeau · 2021-08-05T15:59:20Z

snc.sh

@@ -191,7 +191,8 @@ retry ${OC} scale --replicas=1 ingresscontroller/default -n openshift-ingress-op
 retry ${OC} patch config.imageregistry.operator.openshift.io/cluster --patch '{"spec":{"defaultRoute":true}}' --type=merge

 # Add a tip in the login page
-retry ${OC} get secrets -n openshift-authentication v4-0-config-system-ocp-branding-template -o json | ${JQ} -r '.data["login.html"]'  | base64 -d > login.html
+retry ${OC} get secrets -n openshift-authentication v4-0-config-system-ocp-branding-template -o json > tmp_secret_template.json


Is this commit needed now that retry output goes to stderr? I think pipe only picks up stdout and not stderr?

@cfergeau yes this is still required because even the output is stderr but retry <cmd1> | <cmd2> still going to execute the cmd2 on first failure, what we want is cmd2 only execute if cmd1 successful.

#!/bin/bash set -exuo pipefail export LC_ALL=C.UTF-8 export LANG=C.UTF-8 function retry { local retries=4 local count=0 until "$@"; do exit=$? wait=$((2 ** $count)) count=$(($count + 1)) if [ $count -lt $retries ]; then echo "Retry $count/$retries exited $exit, retrying in $wait seconds..." 1>&2 sleep $wait else echo "Retry $count/$retries exited $exit, no more retries left." 1>&2 return $exit fi done return 0 } retry false | echo "Hello" > foo.txt

What seems to matter is whether <cmd1> outputs something on stdout on failures, or if the only time something is printed to stdout is on success

#!/bin/bash set -exuo pipefail export LC_ALL=C.UTF-8 export LANG=C.UTF-8 function retry { local retries=4 local count=0 until "$@"; do exit=$? wait=$((2 ** $count)) count=$(($count + 1)) if [ $count -lt $retries ]; then echo "fail">/dev/null else echo '{"foo": 0}' return 0 fi done return 0 } retry false | jq .

works fine, jq is called on the last iteration, but if I remove the >/dev/null and print to stdout in the error cases, then it fails.
Maybe what I suggested before would do the trick:

branding_template=$(retry ${OC} get secrets -n openshift-authentication v4-0-config-system-ocp-branding-template -o json) ${JQ} ...

@cfergeau so with latest patch I tried looks like it is failing on the CI since it have input stream once ?

++ cat + pv_template='apiVersion: v1 kind: PersistentVolume metadata: name: pv0001 labels: volume: pv0001 spec: capacity: storage: 100Gi accessModes: - ReadWriteOnce - ReadWriteMany - ReadOnlyMany hostPath: path: /mnt/pv-data/pv0001 persistentVolumeReclaimPolicy: Recycle' + retry ./openshift-clients/linux/oc create -f /dev/fd/63 + local retries=10 ++ echo 'apiVersion: v1 kind: PersistentVolume metadata: name: pv0001 labels: volume: pv0001 spec: capacity: storage: 100Gi accessModes: - ReadWriteOnce - ReadWriteMany - ReadOnlyMany hostPath: path: /mnt/pv-data/pv0001 persistentVolumeReclaimPolicy: Recycle' + local count=0 + ./openshift-clients/linux/oc create -f /dev/fd/63 error: error when creating "/dev/fd/63": Post "https://api.crc.testing:6443/api/v1/persistentvolumes?fieldManager=kubectl-create": dial tcp 192.168.126.11:6443: connect: connection refused + exit=1 + wait=1 + count=1 + '[' 1 -lt 10 ']' + echo 'Retry 1/10 exited 1, retrying in 1 seconds...' Retry 1/10 exited 1, retrying in 1 seconds... + sleep 1 + ./openshift-clients/linux/oc create -f /dev/fd/63 error: no objects passed to create + exit=1 + wait=2 + count=2 + '[' 2 -lt 10 ']' + echo 'Retry 2/10 exited 1, retrying in 2 seconds...' Retry 2/10 exited 1, retrying in 2 seconds... + sleep 2 + ./openshift-clients/linux/oc create -f /dev/fd/63 error: no objects passed to create

cfergeau · 2021-08-05T15:59:36Z

.gitignore

@@ -9,3 +9,4 @@ yq
 openshift-clients/
 podman-remote/
 .sw[a-p]
+tmp_pv.yaml


I'd still prefer to avoid temporary files as much as possible :)

@cfergeau yes but I don't see options here :(

praveenkumar · 2021-08-06T08:21:41Z

/retest

praveenkumar · 2021-08-09T05:07:52Z

/retest

praveenkumar · 2021-08-10T04:25:07Z

/retest

praveenkumar · 2021-08-11T02:52:35Z

/retest

cfergeau

I'd add some explanation to the commit log for Add cluster-cloud-controller-manager-operator to cvo override list
If we need to revisit that decision in 1 year, will be useful to remember why we decided it was not needed in the past.

cfergeau · 2021-08-11T12:34:30Z

snc-library.sh

@@ -170,19 +170,20 @@ function create_pvs() {

    for pvname in $(seq -f "pv%04g" 1 ${count}); do
        if ! ${OC} get pv "${pvname}" &> /dev/null; then
-            generate_pv "${pvdir}/${pvname}" "${pvname}" | ${OC} create -f -
+	    pv_template=$(generate_pv "${pvdir}/${pvname}" "${pvname}")
+            retry ${OC} create -f <(echo ${pv_template})


indentation mismatch it seems, one line with tabs, the other with spaces?

openshift-ci · 2021-08-11T12:35:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, praveenkumar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfergeau,praveenkumar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2021-08-11T13:29:51Z

New changes are detected. LGTM label has been removed.

cfergeau · 2021-08-11T19:16:54Z

snc.sh

@@ -191,7 +191,8 @@ retry ${OC} scale --replicas=1 ingresscontroller/default -n openshift-ingress-op
 retry ${OC} patch config.imageregistry.operator.openshift.io/cluster --patch '{"spec":{"defaultRoute":true}}' --type=merge

 # Add a tip in the login page
-retry ${OC} get secrets -n openshift-authentication v4-0-config-system-ocp-branding-template -o json | ${JQ} -r '.data["login.html"]'  | base64 -d > login.html
+secret_template=$(retry ${OC} get secrets -n openshift-authentication v4-0-config-system-ocp-branding-template -o json)
+${JQ} -r '.data["login.html"]' <(echo "${secret_template}") | base64 -d > login.html


This can be echo ${secret_template} | ${JQ} .... Not familiar at all with the <(echo ...) syntax even if I was the one to bring it up, so don't know if this is behaving as expected.

cfergeau · 2021-08-11T19:26:11Z

snc-library.sh

@@ -170,19 +170,20 @@ function create_pvs() {

    for pvname in $(seq -f "pv%04g" 1 ${count}); do
        if ! ${OC} get pv "${pvname}" &> /dev/null; then
-            generate_pv "${pvdir}/${pvname}" "${pvname}" | ${OC} create -f -
+            pv_template=$(generate_pv "${pvdir}/${pvname}" "${pvname}")
+            retry ${OC} create -f <(echo "${pv_template}")


I think anything involving < | won't work as expected with retry, as the first try will consume the data which was produced by echo, and the next tries will have nothing to read from.

diff --git a/tools.sh b/tools.sh index 288bd74..d22426a 100755 --- a/tools.sh +++ b/tools.sh @@ -91,7 +91,13 @@ fi function retry { local retries=10 local count=0 - until "$@"; do + if [ -t 0 ]; then + # no pipe + input="" + else + # pipe, save stdin + input="$(cat)" + fi + + until echo "$input" | "$@"; do exit=$? wait=$((2 ** $count)) count=$(($count + 1))

might help, but I expect there will be other corner cases I'm not familiar with. An intermediate function may help (one which runs echo ${pv_template} | ${OC} create -f so that we can then run retry intermediate_function ${pv_template})

@cfergeau in that case I will go with intermediate file and remove it after retry happen. Don't want to spend more time around it with hit an trial method. I can create a follow up issue for improvement around it if you like and spend sometime.

Created #460

This patch will fix following error from CI ``` ./openshift-clients/linux/oc create -f - error: error when creating "STDIN": Post "https://api.crc.testing:6443/api/v1/persistentvolumes?fieldManager=kubectl-create": dial tcp 192.168.126.11:6443: connect: connection refused ```

This patch will fix following CI failure. ``` + retry ./openshift-clients/linux/oc get secrets -n openshift-authentication v4-0-config-system-ocp-branding-template -o json + local retries=10 + local count=0 + jq -r '.data["login.html"]' + ./openshift-clients/linux/oc get secrets -n openshift-authentication v4-0-config-system-ocp-branding-template -o json + base64 -d The connection to the server api.crc.testing:6443 was refused - did you specify the right host or port? + exit=1 + wait=1 + count=1 + '[' 1 -lt 10 ']' + echo 'Retry 1/10 exited 1, retrying in 1 seconds...' + sleep 1 parse error: Invalid numeric literal at line 1, column 6 ```

On the CI these tests are failing every single time. - https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/code-ready_snc/457/pull-ci-code-ready-snc-master-e2e-snc/1420281953436831744

openshift-ci bot requested review from cfergeau and gbraad July 28, 2021 07:16

openshift-ci bot added the approved label Jul 28, 2021

praveenkumar force-pushed the add_retry branch from 2a47764 to f77bd17 Compare July 28, 2021 11:14

praveenkumar force-pushed the add_retry branch from f77bd17 to 76d5f4c Compare July 29, 2021 03:44

cfergeau mentioned this pull request Aug 2, 2021

Run OpenShift reliability/minimal test suite #456

Open

cfergeau reviewed Aug 3, 2021

View reviewed changes

cfergeau and others added 2 commits August 5, 2021 11:39

Use a beefier crc instance

58276ce

Give more resources to the cluster, we run the testsuite on should hopefully prevent some disruptions/timeouts because of low memory conditions.

Send retry messages to stderr instead stdout

4fe9f58

This would avoid to have any retry data on a file if we redirect a command output to it and command failed for 1-2 times. ``` retry false > temp_file ```

praveenkumar force-pushed the add_retry branch from 76d5f4c to ee5d3d7 Compare August 5, 2021 06:16

cfergeau reviewed Aug 5, 2021

View reviewed changes

praveenkumar force-pushed the add_retry branch from d75e265 to 0426e8e Compare August 10, 2021 12:21

praveenkumar force-pushed the add_retry branch from 0426e8e to 258e50f Compare August 11, 2021 11:08

cfergeau approved these changes Aug 11, 2021

View reviewed changes

openshift-ci bot assigned cfergeau Aug 11, 2021

openshift-ci bot added the lgtm label Aug 11, 2021

praveenkumar force-pushed the add_retry branch from 258e50f to adf6d95 Compare August 11, 2021 13:29

openshift-ci bot removed the lgtm label Aug 11, 2021

cfergeau reviewed Aug 11, 2021

View reviewed changes

praveenkumar added 4 commits August 12, 2021 10:47

Add more k8s conformance test in the ignore list

d9a5137

On the CI these tests are failing every single time. - https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/code-ready_snc/457/pull-ci-code-ready-snc-master-e2e-snc/1420281953436831744

Add cluster-cloud-controller-manager-operator to cvo override list

25449e3

praveenkumar force-pushed the add_retry branch from adf6d95 to 25449e3 Compare August 12, 2021 05:22

praveenkumar mentioned this pull request Aug 16, 2021

refactor to remove the temp file during retry #460

Open

praveenkumar merged commit de127b4 into crc-org:master Aug 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add retry for pv creation commands #457

Add retry for pv creation commands #457

praveenkumar commented Jul 28, 2021

cfergeau commented Jul 28, 2021

praveenkumar commented Jul 29, 2021

praveenkumar commented Jul 29, 2021

praveenkumar commented Jul 30, 2021

praveenkumar commented Aug 2, 2021

cfergeau commented Aug 2, 2021

cfergeau Aug 3, 2021 •

edited

Loading

cfergeau Aug 3, 2021

cfergeau Aug 3, 2021 •

edited

Loading

praveenkumar Aug 4, 2021 •

edited

Loading

cfergeau commented Aug 4, 2021

praveenkumar commented Aug 5, 2021

praveenkumar commented Aug 5, 2021

cfergeau Aug 5, 2021

praveenkumar Aug 9, 2021

cfergeau Aug 10, 2021

praveenkumar Aug 11, 2021

cfergeau Aug 5, 2021

praveenkumar Aug 9, 2021

praveenkumar commented Aug 6, 2021

praveenkumar commented Aug 9, 2021

praveenkumar commented Aug 10, 2021

praveenkumar commented Aug 11, 2021

cfergeau left a comment

cfergeau Aug 11, 2021

openshift-ci bot commented Aug 11, 2021

openshift-ci bot commented Aug 11, 2021

cfergeau Aug 11, 2021

cfergeau Aug 11, 2021

praveenkumar Aug 12, 2021

praveenkumar Aug 16, 2021

Add retry for pv creation commands #457

Add retry for pv creation commands #457

Conversation

praveenkumar commented Jul 28, 2021

cfergeau commented Jul 28, 2021

praveenkumar commented Jul 29, 2021

praveenkumar commented Jul 29, 2021

praveenkumar commented Jul 30, 2021

praveenkumar commented Aug 2, 2021

cfergeau commented Aug 2, 2021

cfergeau Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfergeau Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

praveenkumar Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

cfergeau commented Aug 4, 2021

praveenkumar commented Aug 5, 2021

praveenkumar commented Aug 5, 2021

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

praveenkumar commented Aug 6, 2021

praveenkumar commented Aug 9, 2021

praveenkumar commented Aug 10, 2021

praveenkumar commented Aug 11, 2021

cfergeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 11, 2021

openshift-ci bot commented Aug 11, 2021

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

cfergeau Aug 3, 2021 •

edited

Loading

cfergeau Aug 3, 2021 •

edited

Loading

praveenkumar Aug 4, 2021 •

edited

Loading