Fix how we wait on router becoming available

[ramr]

as the cluster ingress operator uses daemonsets (not supported by oc wait).
this could be an alternative to PR #2001

@ironcladlou @smarterclayton PTAL thx

[wking]

I'm not clear on how -w plays with MAX_RETRIES and the sleep 60 at the tail of the loop. But a max of 10 infinitely-blocking calls seems like something that's going to hang for too long sometimes ;). Maybe something like:

i=0
MAX_RETRIES=10
TARGET="$(date +%s)"
TARGET="$((TARGET + 60))"
until oc --request-timeout=55s oc rollout status "${ROUTER_DEPLOYMENT}" -n "${ROUTER_NAMESPACE}" -w || [ $i -eq $MAX_RETRIES ]; do
  i=$((i+1))
  ...
  REMAINING="$((TARGET - NOW))"
  sleep "${REMAINING}"
  TARGET="$((TARGET + 60))"
done

[ramr]

lol - yeah that's true but figured its going to fail anyway beyond this point if we don't get back the rollout info (assume there is a timeout above this in the caller) but ... fair point. I created this PR as an alternative - am not certain its needed as @smarterclayton was ok with the other alternative in #2001 - putting this one on hold.

[wking]

... but figured its going to fail anyway beyond this point if we don't get back the rollout info...

Then we can fail after the loop if [[ $i -eq $MAX_RETRIES ]] (or move the MAX_RETRIES comparison inside the loop and exit 1 if it turns true)? Or just drop MAX_RETRIES and set the sleep to 1 second to recover from network hiccups and rely on whatever wrapping timeout to reap the container (assuming that exists)?

[ramr]

I think having a few retries might be good to do anyway to handle any other connectivity/spurious issues. I'll fix this up as per your original comments with a timeout. Thanks.

[ramr]

/hold

[ramr]

/hold cancel

[ramr]

/retest

[ramr]

@wking PTAL Thx. Made the changes - I put in a 2m wait on the oc rollout command.
Test snippet log: https://gist.github.com/ramr/05fe97fe0e0d4a7929330dc178f33db0

[wking]

I think we should drop the MAX_RETRIES check here. It's originally from fe24e8b (#1773), but:

$ i=0
$ MAX_RETRIES=3
$ until false || [ $i -eq $MAX_RETRIES ]; do i=$((i+1)); echo "$i"; [ $i -eq $MAX_RETRIES ] && echo "timeout" && break; done
1
2
3
timeout
$ i=0
$ until false; do i=$((i+1)); echo "$i"; [ $i -eq $MAX_RETRIES ] && echo "timeout" && break; done
$ until false; do i=$((i+1)); echo "$i"; [ $i -eq $MAX_RETRIES ] && echo "timeout" && break; done
1
2
3
timeout

You never hit the case where the MAX_RETRIES condition in this line is true, because you always hit the exit 1 timeout inside the loop body first.

CC @abhinavdahiya

[ramr]

will drop it - it is redundant.

[wking]

60 seconds seems like a long break for a network or spurious issue. On the other hand, if we only sleep 1, we'd exit 1 here if the network was out for only 10 seconds. So I'd still rather have logic that said "this is the wall time we're willing to wait, we don't care how many network connections you need to cover that window". If you don't like my earlier proposal, how about:

TARGET="$(date -d '10 minutes' +%s)"
NOW="$(date +%s)"
while [[ "${NOW}" -lt "${TARGET}" ]]; do
  REMAINING="$((TARGET - NOW))"
  if oc --request-timeout="${REMAINING}s" oc rollout status "${ROUTER_DEPLOYMENT}" -n "${ROUTER_NAMESPACE}" -w; then
    break
  fi
  NOW="$(date +%s)"
done
[[ "${NOW}" -ge "${TARGET}" ]] && echo "timeout waiting for ${ROUTER_NAMESPACE}/${ROUTER_DEPLOYMENT} to be available" && exit 1

or:

TARGET="$(date -d '10 minutes' +%s)"
NOW="$(date +%s)"
REMAINING="$((TARGET - NOW))"
until oc --request-timeout="${REMAINING}s" oc rollout status "${ROUTER_DEPLOYMENT}" -n "${ROUTER_NAMESPACE}" -w; do
  NOW="$(date +%s)"
  REMAINING="$((TARGET - NOW))"
  if [[ "${REMAINING}" -le 0 ]]; then
    echo "timeout waiting for ${ROUTER_NAMESPACE}/${ROUTER_DEPLOYMENT} to be available"
    exit 1
  fi
done

And all of these are untested sketches, if we hit on phrasing you like let me know and I can test it more thoroughly ;).

[ramr]

Nah, the original one is fine. I'll use that one (and test it) and update this PR. Thanks.

[ramr]

@wking made the requested change PTAL Thx

• collapsed a couple of lines from the wait expiry checks you had suggested
• removed the redundant condition check on the until line
• increased the wait time to 15 mins (took a while for the cluster to come up, so figured might as well bump that limit up a wee bit).
• Tested just the snippet via a script albeit with slightly different values for the timeouts to simulate getting into the loop ... https://gist.github.com/ramr/6b585f5b579b7df9ebe034bf896c7832

[wking]

increased the wait time to 15 mins...

So ideally this would be:

oc --request-timeout=900s rollout status "${ROUTER_DEPLOYMENT}" -n "${ROUTER_NAMESPACE}" -w

But connections break down, so you want to retry. And if the connection breaks after 5 minutes, you want the next call to have --request-timeout=600s. Or maybe you're concerned about flooding requests, so you want to sleep for two seconds and run the next call with --request-timeout=598s. My suggestions here have that sliding timeout.

The constant-timeout, max-retries approach has to balance efficiency and responsiveness (favoring long timeouts and therefore low max-retries) against resiliency (favoring high max-retries and therefore low request tineouts). I want both ;).

But really, this seems like a generic problem that really wants an oc/kubectl-level fix, so you could:

oc --request-timeout=900s --reconnect rollout status "${ROUTER_DEPLOYMENT}" -n "${ROUTER_NAMESPACE}" -w

Without all the wrapping shell business. I'm happy pushing for that, and landing any rollout approach in the meantime. Are you comfortable with 7991fd3?

[wking]

Ah, I hear you're off today, so I'll just:

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ramr, wking

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• ci-operator/templates/openshift/installer/OWNERS [wking]

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

[openshift-ci-robot]

@ramr: Updated the prow-job-cluster-launch-installer-e2e configmap using the following files:

• key cluster-launch-installer-e2e.yaml using file ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml

Details

In response to this:

as the cluster ingress operator uses daemonsets (not supported by oc wait).
this could be an alternative to PR #2001

@ironcladlou @smarterclayton PTAL thx

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ironcladlou]

Just to correct the record: I mistook today for Friday, so Ram should in fact be around... I'm sure he'll be pleased to see this merged when he gets in 😁