Bug 1872080: Avoid a bash "unbound variable" error by danwinship · Pull Request #192 · openshift/cluster-dns-operator

danwinship · 2020-08-28T17:02:41Z

Seen in https://openshift-gce-devel.appspot.com/build/origin-ci-test/logs/periodic-ci-openshift-ovn-kubernetes-master-e2e-metal-ipi-dualstack-periodic/1299134739436802048/. There is some larger failure mode here which is not yet diagnosed, but on the way to figuring that out I saw that the current error-handling code in the dns-node-resolver pod is not working, resulting in the container crashing with:

2020-08-28T01:57:14.539502724Z /bin/bash: line 38: svc_ips: unbound variable

rather than sleeping and looping

danwinship · 2020-08-29T16:12:08Z

lol wut

2020-08-28T17:49:11.033552409Z /bin/bash: line 51: cmp: command not found
2020-08-28T17:50:11.058408167Z /bin/bash: line 51: cmp: command not found
2020-08-28T17:51:11.080578023Z /bin/bash: line 51: cmp: command not found
2020-08-28T17:52:11.111326141Z /bin/bash: line 51: cmp: command not found
2020-08-28T17:53:11.127450483Z /bin/bash: line 51: cmp: command not found
2020-08-28T17:54:11.142883458Z /bin/bash: line 51: cmp: command not found
2020-08-28T17:55:11.157156676Z /bin/bash: line 51: cmp: command not found
...

was the check broken before and always false? Or maybe this is caused by the RHEL 8 migration?

Under "set -u", if svc_ips is declared-but-empty, then "${#svc_ips[@]}" results in an "unbound variable" error (which, under "set -e" causes the script to exit with an error). However, "${svc_ips[@]}" expands to the empty string, as expected.

danwinship · 2020-08-29T18:49:42Z

So yeah, the RHCOS 8 images appear to not have cmp (or diff; same package, diffutils), while the RHCOS 7 images did. Possible RHCOS bug? (@jupierce? @alvaroaleman?)

You could do something like:

if [[ "$(cat ${TEMP_FILE} | sha256sum)" != "$(cat ${HOSTS_FILE} | sha256sum)" ]]; then
  cp -f "${TEMP_FILE}" "${HOSTS_FILE}"
fi

(The cat+pipe is needed because otherwise sha256sum includes the filename in the output which isn't what we want.)

alvaroaleman · 2020-08-29T18:52:48Z

Sorry, I know nothing about how rhcos 7 and 8 differ. @jupierce and @yselkowitz are the best candidates for questions around that.

yselkowitz · 2020-08-30T05:07:59Z

In what container is the script which calls cmp actually run (asking because some operators provide a script but run it in a different container)?

danwinship · 2020-08-30T14:07:04Z

oh, duh, I'm mixing up RHCOS and UBI... I was thinking we couldn't install extra RPMs by hand but obviously we can

danwinship · 2020-08-31T12:29:05Z

In what container is the script which calls cmp actually run (asking because some operators provide a script but run it in a different container)?

Looks like "openshift/origin-cli:v4.0" or whatever that gets magically turned into.

yselkowitz · 2020-08-31T12:45:38Z

@jupierce should diffutils be explicitly added to base for this?

yselkowitz

If the script is run in cli, then installing cmp in this operator isn't going to help.

yselkowitz · 2020-08-30T17:09:17Z

 FROM registry.svc.ci.openshift.org/ocp/4.6:base
+
+# dns-node-resolver needs "cmp"
+RUN dnf install -y diffutils && dnf clean all


I'd recommend yum instead of dnf.

yselkowitz · 2020-08-31T13:58:02Z

openshift/images#35 should fix this via inheritance.

danwinship · 2020-08-31T14:14:19Z

If the script is run in cli, then installing cmp in this operator isn't going to help.

yeah, I hadn't realized it was using a different image when I pushed that commit

danwinship · 2020-08-31T14:14:30Z

(I later un-pushed that commit)

danwinship · 2020-09-02T16:53:35Z

/retest

yselkowitz · 2020-09-03T04:16:38Z

/lgtm
/retest

yselkowitz · 2020-09-03T04:17:23Z

/retitle Bug 1872080: Avoid a bash "unbound variable" error

openshift-ci-robot · 2020-09-03T04:17:43Z

@danwinship: This pull request references Bugzilla bug 1872080, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1872080: Avoid a bash "unbound variable" error

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.

yselkowitz · 2020-09-03T11:56:57Z

/assign @ironcladlou

sgreene570 · 2020-09-03T14:09:25Z

Looks like theres also the possibility of an unbound variable error here:

cluster-dns-operator/assets/dns/daemonset.yaml

Line 120 in 5c7efff

if [[ "$?" -eq 0 && "${#ips[@]}" -ne 0 ]]; then

svc_ips is derived from ips.
@danwinship mind addressing that statement in this PR as well?

sgreene570 · 2020-09-03T14:14:02Z

Ah, nevermind, after further reading, the ips variable shouldn't ever be unbound.

cluster-dns-operator/assets/dns/daemonset.yaml

Lines 117 to 119 in 5c7efff

    
                         for i in ${!cmds[*]} 
        
                         do 
        
                           ips=($(eval "${cmds[i]}"))

/approve

openshift-ci-robot · 2020-09-03T14:14:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, sgreene570, yselkowitz

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:

~~OWNERS~~ [sgreene570]

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

crawford · 2020-09-03T14:55:40Z

/retest

yselkowitz · 2020-09-03T15:59:16Z

/hold
This might not be enough after all, testing now.

sgreene570 · 2020-09-03T16:00:52Z

/hold
This might not be enough after all, testing now.

Could you elaborate on whats going wrong?

yselkowitz · 2020-09-03T16:07:34Z

/hold cancel
Never mind, problem is unrelated.

danwinship · 2020-09-03T16:09:42Z

The logs are still showing /bin/bash: line 51: cmp: command not found (eg https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-dns-operator/192/pull-ci-openshift-cluster-dns-operator-master-e2e-aws/1300410258996334592/artifacts/e2e-aws/gather-extra/pods/openshift-dns_dns-default-2xq66_dns-node-resolver.log) but that's technically a separate problem from the one being fixed in this PR...

Miciah · 2020-09-03T16:10:29Z

            # We will not update /etc/hosts when there is coredns service outage or api unavailability
            # Stale entries could exist in /etc/hosts if the service is deleted
-            if [[ "${#svc_ips[@]}" -ne 0 ]]; then
+            if [[ -n "${svc_ips[*]}" ]]; then


Why is this necessary? This works for me on RHEL76:

$ echo $BASH_VERSION 4.2.46(2)-release $ set -u $ declare -A svc_ips $ [[ "${#svc_ips[@]}" -ne 0 ]] $ echo $? 1 $

Do we need to report a BZ against RHEL8's Bash?

much newer bash, probably expected

I filed BZ#1875566, "Bash says a declared but empty array is unbound".

The behavior in 4.2 was a bug. It was fixed incorrectly in 4.3 (making both ${#svc_ips[@]} and ${svc_ips[@]} an error) and then fully fixed in 4.4 (making the former an error but the latter not). This has to do with POSIX compliance or something.

This PR seems to have fixed clusters using the RHEL 8 base, but broke clusters using the prior RHEL 7 base:
eg

sh-4.2# echo $BASH_VERSION 4.2.46(2)-release sh-4.2# set -u sh-4.2# declare -A svc_ips sh-4.2# [[-n "${svc_ips[*]}"]] sh: svc_ips[*]: unbound variable

Would [[ -n "${svc_ips[*]-}" ]] work on both Bash 4.2 and 4.4?

Would [[ -n "${svc_ips[*]-}" ]] work on both Bash 4.2 and 4.4?

I think so... what exactly does the - change?

${parameter-default} means to substitute the value of the variable parameter if it is set, or the literal value default (which in our case is the empty string) if parameter is not set.

Ah, I had tried changing the original ${#svc_ips[@]} to ${#svc_ips[@]:-0}, which does not work.

#193 changes the test to [[ -n "${svc_ips[*]-}" ]].

yselkowitz · 2020-09-03T16:11:21Z

missing cmp would be indicative of a stale cache, problem has already been fixed in 4.6:base

sgreene570 · 2020-09-03T17:39:13Z

cmp binary present in most recent run: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-dns-operator/192/pull-ci-openshift-cluster-dns-operator-master-e2e-aws/1301524653289771008

Failures look unrelated.

/retest

openshift-ci-robot · 2020-09-03T19:50:19Z

@danwinship: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/origin#25442 is open

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state.

Bugzilla bug 1872080 has not been moved to the MODIFIED state.

Details

In response to this:

Bug 1872080: Avoid a bash "unbound variable" error

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.

openshift-ci-robot requested review from danehans and ironcladlou August 28, 2020 17:02

danwinship force-pushed the unbound-svc-ips branch from 0a887e1 to a411700 Compare August 28, 2020 17:16

Avoid a bash "unbound variable" error

5c7efff

Under "set -u", if svc_ips is declared-but-empty, then "${#svc_ips[@]}" results in an "unbound variable" error (which, under "set -e" causes the script to exit with an error). However, "${svc_ips[@]}" expands to the empty string, as expected.

danwinship force-pushed the unbound-svc-ips branch 2 times, most recently from 4530ed8 to 5c7efff Compare August 31, 2020 12:28

yselkowitz suggested changes Aug 31, 2020

View reviewed changes

openshift-ci-robot assigned yselkowitz Sep 3, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2020

openshift-ci-robot changed the title ~~Avoid a bash "unbound variable" error~~ Bug 1872080: Avoid a bash "unbound variable" error Sep 3, 2020

openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Sep 3, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 3, 2020

openshift-ci-robot assigned ironcladlou Sep 3, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2020

Miciah reviewed Sep 3, 2020

View reviewed changes

openshift-merge-robot merged commit 6b4360e into openshift:master Sep 3, 2020

Miciah mentioned this pull request Sep 8, 2020

Bug 1876912: Fix "unbound variable" error on Bash 4.2 #193

Merged

openshift-ci-robot mentioned this pull request Sep 14, 2020

Bug 1872080: Rename image references for RHEL 8 openshift/cluster-resource-override-admission-operator#36

Merged

Conversation

danwinship commented Aug 28, 2020

Uh oh!

danwinship commented Aug 29, 2020

Uh oh!

danwinship commented Aug 29, 2020

Uh oh!

alvaroaleman commented Aug 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yselkowitz commented Aug 30, 2020

Uh oh!

danwinship commented Aug 30, 2020

Uh oh!

danwinship commented Aug 31, 2020

Uh oh!

yselkowitz commented Aug 31, 2020

Uh oh!

yselkowitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yselkowitz commented Aug 31, 2020

Uh oh!

danwinship commented Aug 31, 2020

Uh oh!

danwinship commented Aug 31, 2020

Uh oh!

danwinship commented Sep 2, 2020

Uh oh!

yselkowitz commented Sep 3, 2020

Uh oh!

yselkowitz commented Sep 3, 2020

Uh oh!

openshift-ci-robot commented Sep 3, 2020

Uh oh!

yselkowitz commented Sep 3, 2020

Uh oh!

sgreene570 commented Sep 3, 2020

Uh oh!

sgreene570 commented Sep 3, 2020

Uh oh!

openshift-ci-robot commented Sep 3, 2020

Uh oh!

crawford commented Sep 3, 2020

Uh oh!

yselkowitz commented Sep 3, 2020

Uh oh!

sgreene570 commented Sep 3, 2020

Uh oh!

yselkowitz commented Sep 3, 2020

Uh oh!

danwinship commented Sep 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgreene570 Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yselkowitz commented Sep 3, 2020

Uh oh!

sgreene570 commented Sep 3, 2020

alvaroaleman commented Aug 29, 2020 •

edited

Loading

sgreene570 Sep 8, 2020 •

edited

Loading