[on-prem] Bug 2089775: Fix regexp in keepalived script chk_default_ingress.sh #3156
Conversation
This change fixes issue #3155
|
Hi @nvsmirnov. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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
bot
added
the
needs-ok-to-test
kikisdeliveryservice
changed the title
|
As noted on the issue, this will need a bugzilla. |
|
Opened bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2089775 |
|
Thanks @nvsmirnov ! |
kikisdeliveryservice
changed the title
openshift-ci
bot
added
bugzilla/severity-unspecified
|
@nvsmirnov: This pull request references Bugzilla bug 2089775, which is invalid:
Comment In response to this:
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. |
kikisdeliveryservice
added
the
do-not-merge/hold
| @@ -3,4 +3,4 @@ path: "/etc/kubernetes/static-pod-resources/keepalived/scripts/chk_default_ingre | |||
| contents: | |||
| inline: | | |||
| #!/bin/bash | |||
| /host/bin/oc --kubeconfig /var/lib/kubelet/kubeconfig get ep -n openshift-ingress router-internal-default -o yaml | grep 'ip:' | grep -q {{`{{.NonVirtualIP}}`}} | |||
| /host/bin/oc --kubeconfig /var/lib/kubelet/kubeconfig get ep -n openshift-ingress router-internal-default -o yaml | grep 'ip:' | grep -q '{{`{{.NonVirtualIP}}`}}$' | |||
There was a problem hiding this comment.
Can we combine the two greps here so we have correct boundaries at both the start and end of the IP? I accidentally pushed a duplicate fix in #3164 but since you proposed this first I'd prefer to merge it under your PR.
There was a problem hiding this comment.
Yes, of course. Strictly saying, the form with grep still not exact (because dot is special character)
It is better process with jq, I'll figure out soon how it is better to do this with jq filters :-)
There was a problem hiding this comment.
@cybertron, I've changed PR so it now uses jq instead of grep, and this should work more precise.
There was a problem hiding this comment.
...the only problem is I can't check here if this template will render correctly to destination file.
What I did is checked on real OKD node that this works:
oc --kubeconfig /var/lib/kubelet/kubeconfig get ep -n openshift-ingress router-internal-default -o json | jq -e '.subsets[].addresses | map( select (.ip == "IP_ADDRESS") )[0]'; echo $?
If IP_ADDRESS is in oc output, it sets exit status 0, and non-zero if it is not in output.
|
/ok-to-test Thanks, I will also pull this down locally so I can poke around the environment after deployment and make sure everything looks to be doing what it's supposed to. |
openshift-ci
bot
added
ok-to-test
|
/test e2e-metal-ipi |
|
/bugzilla refresh |
openshift-ci
bot
added
bugzilla/severity-high
|
@kikisdeliveryservice: This pull request references Bugzilla bug 2089775, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (vvoronko@redhat.com), skipping review request. In response to this:
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. |
There was a problem hiding this comment.
Unfortunately it looks like jq is not available in the keepalived image, which may explain why we didn't use it originally.
Grep should be safe to use in this case if we choose the regex correctly. If a . matched a number incorrectly, then the next . in the IP would not match. Since we know the value here is an IP, we don't have to worry about matching something like 1a1b1c1 to 1.1.1.1 since that would be invalid for the field in the first place.
|
@cybertron, oh yes. I changed back to one grep, thank you. |
|
/retest |
|
/lgtm Thanks! |
|
/test e2e-metal-ipi Should probably make sure that still works, but this is the version I tested locally so I'm not worried about it. |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cybertron, mandre, nvsmirnov, sinnykumari 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/hold cancel We have lgtm from multiple platforms and all of the tests are passing. We should be good. |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@nvsmirnov: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@nvsmirnov: All pull requests linked via external trackers have merged: Bugzilla bug 2089775 has been moved to the MODIFIED state. In response to this:
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. |
Fixes: #3155
- What I did
Changed regexp in template. Really I didn't even try to build MCO, just created machineconfig override to test if it works :-)
- How to verify it
Apply fix on cluster and see if destination file contains correct grep command, and keepalived keeps ingress VIP on same node that runs ingress pod.
Ideally one can check it on cluster that has nodes with IP as described in issue #3155.
- Description for the changelog
Fixed keepalived check for ingress VIP