Bug 1886572: Calculate keepalived priority for ingress

[yboaron]

Ingress VIP should be set only on a node that runs an instance of the default ingress controller pod.
In current code, in case extra ingress-controllers are created the ingress VIP might be wrongly set on a node that doesn't run
an instance of the default ingress controller.

This PR calculates the priority for keepalived ingress VIP depending on the presence of the router pod in the node by monitoring the content of router-internal-default endpoints resource.

[yboaron]

/retitle Bug 1886572: Calculate keepalived priority for ingress

[openshift-ci]

@yboaron: This pull request references Bugzilla bug 1886572, which is valid. The bug has been moved to the POST state. 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.8.0) matches configured target release for branch (4.8.0)
• bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

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:

Bug 1886572: Calculate keepalived priority for ingress

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.

[cybertron]

Is there any chance we can do this in a check script instead of the monitor? Or do we not have the ability to run oc commands from inside the keepalived container? It would just be nice to keep all of the priority bits in the same place so we're not having to look at both the keepalived and monitor logs when trying to figure out what happened with the priority if/when a VIP ends up somewhere it shouldn't.

Although maybe on reload keepalived would log the new priority anyway? Mostly I want to avoid making keepalived harder to debug than it already is.

[yboaron]

Is there any chance we can do this in a check script instead of the monitor? Or do we not have the ability to run oc commands from inside the keepalived container? It would just be nice to keep all of the priority bits in the same place so we're not having to look at both the keepalived and monitor logs when trying to figure out what happened with the priority if/when a VIP ends up somewhere it shouldn't.

Although maybe on reload keepalived would log the new priority anyway? Mostly I want to avoid making keepalived harder to debug than it already is.

Agree that having check_script for this purpose is ideal, we need to be able :

1. As you mentioned, to run oc command from the keepalived container
2. Retrieve node's IP address

I'll try to mount host's oc binary and check that

[yboaron]

/retest

[cybertron]

After some discussion elsewhere, we decided to proceed with this solution since it's the one we have implemented and code freeze is coming up.

However, I have concerns about the error handling and we need to fix the fmt errors before it can go in.

[cybertron]

I don't think we want to default to 40 for the error case. If we can't determine whether the node has the default ingress we should give it the lower priority so we don't take the VIP from a node that is known to have the right ingress.

This applies to all of the other error cases below too.

Also, can we log the error? Otherwise all we know is that we got a priority of 20, but that could mean we didn't have the ingress or it could mean something went wrong here.

[yboaron]

I set the priority to 40 in case of error because I didn't want to change current behavior (where priority set to 40 by default) though setting the priority to 20 in error case makes sense.

I'll change it to 20.

[yboaron]

/retest

[yboaron]

/test e2e-metal-ipi

[openshift-ci]

@yboaron: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-metal-ipi	65f8866	link	/test e2e-metal-ipi

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.

[cybertron]

/lgtm

The metal-ipi job failure appears unrelated and the ipv6 job passed so this should be fine.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cybertron, yboaron

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 [cybertron,yboaron]

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

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-ci]

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

• openshift/baremetal-runtimecfg#141

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

• openshift/machine-config-operator#2595 is open

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with /bugzilla refresh.

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

In response to this:

Bug 1886572: Calculate keepalived priority for ingress

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.