Bug 2018481: OpenStack: Add note about ETP=Local and Octavia #46245

dulek · 2022-06-02T12:18:13Z

Setting .spec.endpointTrafficPolicy=Local disables node's ability to
redirect NodePort traffic to other nodes when no endpoint of that
NodePort Service exists on the target node. This means that LoadBalancer
Services need to be smart and only direct traffic to nodes that host the
endpoint pods.

This is normally achieved by using endpoint healthchecks. This commit
adds a note that it is required to enable "health monitors" support in
OpenStack cloud-provider configuration in order to support ETP=Local
Services.

As OVN Octavia provider in OSP16 does not support health monitors, a
note is added that ETP=Local Services will not function properly with
OVN Octavia provider.

Version(s): 4.11, 4.10

Issue: https://bugzilla.redhat.com/show_bug.cgi?id=2018481

Previews:

openshift-ci · 2022-06-02T12:18:19Z

@dulek: This pull request references Bugzilla bug 2018481, which is invalid:

expected the bug to target the "4.11.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 2018481: OpenStack: Add note about ETP=Local and Octavia

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.

dulek · 2022-06-02T12:25:50Z

/bugzilla refresh

openshift-ci · 2022-06-02T12:25:58Z

@dulek: This pull request references Bugzilla bug 2018481, 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.11.0) matches configured target release for branch (4.11.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @eurijon

In response to this:

/bugzilla refresh

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.

maxwelldb

Noticed this and left some notes. :) @dulek

modules/installation-osp-setting-cloud-provider-options.adoc

maxwelldb · 2022-06-02T13:17:35Z

modules/installation-osp-setting-cloud-provider-options.adoc

 #...
 ----
 <1> This property enables Octavia integration.
 <2> This property sets the Octavia provider that your load balancer uses. It accepts `"ovn"` or `"amphora"` as values. If you choose to use OVN, you must also set `lb-method` to `SOURCE_IP_PORT`.
 <3> This property is required if you want to use multiple external networks with your cluster. The cloud provider creates floating IP addresses on the network that is specified here.
+<4> Setting this property to `True` will make cloud provider create health monitors for the Octavia load balancers. As of {rh-openstack} 16.x this is only supported for Amphora provider.
+<5> This property is required when `create-monitor` is set to `True`. It controls how often endpoints are monitored.
+<6> This property is required when `create-monitor` is set to `True`. It controls timeout of the monitor requests.


Suggested change

<6> This property is required when `create-monitor` is set to `True`. It controls timeout of the monitor requests.

<6> This property is required when `create-monitor` is set to `True`. It sets how long monitoring requests take to time out in seconds.

modules/installation-osp-setting-cloud-provider-options.adoc

maxwelldb · 2022-06-02T13:18:10Z

modules/installation-osp-setting-cloud-provider-options.adoc

+<4> Setting this property to `True` will make cloud provider create health monitors for the Octavia load balancers. As of {rh-openstack} 16.x this is only supported for Amphora provider.
+<5> This property is required when `create-monitor` is set to `True`. It controls how often endpoints are monitored.
+<6> This property is required when `create-monitor` is set to `True`. It controls timeout of the monitor requests.
+<7> This property is required when `create-monitor` is set to `True`. It controls how many successful monitor requests are needed before load balancer member is marked as online.


Suggested change

<7> This property is required when `create-monitor` is set to `True`. It controls how many successful monitor requests are needed before load balancer member is marked as online.

<7> This property is required when `create-monitor` is set to `True`. It controls how many successful monitor requests are needed before a load balancer member is marked as online.

modules/installation-osp-setting-cloud-provider-options.adoc

dulek · 2022-06-02T16:01:36Z

modules/nw-ingress-controller-configuration-parameters.adoc

+
+[NOTE]
+====
+On {rh-openstack-first} platform `LoadBalancerService` `endpointPublishingStrategy` is only supported when cloud provider is configured to create health monitors. Please note that as of {rh-openstack} 16.1 and 16.2 this is only possible with Amphora Octavia provider.


Can you help me understand how can I link to the cloud provider configuration document from here?

Yeah. Which page specifically do you want to link to? There are a number of pages that I think fit that description, so I want to make sure I'm giving the right guidance.

@maxwelldb: Exactly the other one I edited, which explains how to configure the cloud provider with health monitors and why it's important.

Oh, that's just a module. So I guess that module on the regular OpenStack installation page will work, so like to [1].

[1] https://docs.openshift.com/container-platform/4.10/installing/installing_openstack/installing-openstack-installer-custom.html#installation-osp-setting-cloud-provider-options_installing-openstack-installer-custom

An unfortunate limitation of our docs tooling is that you cannot link from one module to another within the same docs set. While we know that the installation pages are pretty reliable, it isn't 'guaranteed' that a module will ever be packaged with another, and so the practice is forbidden. 😬

We can get around this by either quoting the title of the section we mean--it's on the user to locate it, then--or by adding a note to the assembly that contains the module that we want to link from. Totally a judgment call, though we don't really like keeping too much content in assemblies when possible.

dulek · 2022-06-02T16:03:05Z

Thank you for the review! I've attempted to address your comments and added a note about the issue on the Ingress controller page (we've noticed the issue while deploying Ingresses, so I guess it's worth mentioning it there too). Please take another look.

mandre

The modifications make sense to me. Thanks @dulek for taking care of it.

modules/installation-osp-setting-cloud-provider-options.adoc

dulek · 2022-06-03T15:54:28Z

Alright, updated according to the suggestions and added a tip on where to find more information about OpenStack cloud provider configuration on the Ingress page note.

dulek · 2022-07-20T14:46:12Z

@maxwelldb: Hi, any chance to merge this one?

modules/nw-ingress-controller-configuration-parameters.adoc

modules/installation-osp-setting-cloud-provider-options.adoc

maxwelldb · 2022-07-20T15:17:04Z

@dulek Not outside of the realm of possibility!

This needs a QE ack, and then it can be peer reviewed by the docs team.

dulek · 2022-07-20T16:09:51Z

@eurijon, can you take a look if this addition makes sense? This is related to the BZ in the PR title.

bergerhoffer · 2022-08-10T13:03:34Z

The enterprise-4.12 label has been added to this PR.

This is because your PR targets the main branch and is labeled for enterprise-4.11. And any PR going into main must also target the latest version branch (enterprise-4.12).

If the update in your PR does NOT apply to version 4.12 onward, please re-target this PR to go directly into the appropriate version branch or branches (enterprise-4.x) instead of main.

Setting `.spec.endpointTrafficPolicy=Local` disables node's ability to redirect NodePort traffic to other nodes when no endpoint of that NodePort Service exists on the target node. This means that LoadBalancer Services need to be smart and only direct traffic to nodes that host the endpoint pods. This is normally achieved by using endpoint healthchecks. This commit adds a note that it is required to enable "health monitors" support in OpenStack cloud-provider configuration in order to support `ETP=Local` Services. As OVN Octavia provider in OSP16 does not support health monitors, a note is added that `ETP=Local` Services will not function properly with OVN Octavia provider. Also for ingresses with `endpointPublishingStrategy=LoadBalancerService` the created Service will have `ETP=Local` configured. This commit also adds a note to the ingress operator docs that informs that in OpenStack additional configuration of the cloud provider is required to support such Services.

dulek · 2022-09-26T14:53:01Z

@bergerhoffer: It's squashed now.

maxwelldb · 2022-09-26T15:48:01Z

@maxwelldb Does this need another peer review other than yours, or is it ready to request a merge?

@bergerhoffer Yeah, I figured it made sense to have someone who was not me look, as I provided some suggestions that were accepted.

bergerhoffer

This LGTM!

@dulek Are you ready for this PR to be merged? If so, can you please add the merge-review-needed label, like below, and then I (or someone else) will merge?

/label merge-review-needed

bergerhoffer · 2022-09-26T16:24:28Z

Well I wasn't expecting it to label it when it was in a code block like that... 😄

/remove-label merge-review-needed

dulek · 2022-09-27T07:50:09Z

Yes, let's merge this, it's been almost 4 months since it's up.

/label merge-review-needed

openshift-ci · 2022-09-27T12:46:36Z

@dulek: All pull requests linked via external trackers have merged:

openshift/openshift-docs#46245

Bugzilla bug 2018481 has been moved to the MODIFIED state.

In response to this:

Bug 2018481: OpenStack: Add note about ETP=Local and Octavia

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.

bergerhoffer · 2022-09-27T12:46:37Z

/cherrypick enterprise-4.12

bergerhoffer · 2022-09-27T12:46:41Z

/cherrypick enterprise-4.11

bergerhoffer · 2022-09-27T12:46:46Z

/cherrypick enterprise-4.10

openshift-cherrypick-robot · 2022-09-27T12:48:10Z

@bergerhoffer: new pull request created: #50905

In response to this:

/cherrypick enterprise-4.12

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-cherrypick-robot · 2022-09-27T12:48:13Z

@bergerhoffer: new pull request created: #50906

In response to this:

/cherrypick enterprise-4.11

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-cherrypick-robot · 2022-09-27T12:48:13Z

@bergerhoffer: #46245 failed to apply on top of branch "enterprise-4.10":

Applying: OpenStack: Add note about ETP=Local and Octavia
Using index info to reconstruct a base tree...
M	modules/nw-ingress-controller-configuration-parameters.adoc
Falling back to patching base and 3-way merge...
Auto-merging modules/nw-ingress-controller-configuration-parameters.adoc
CONFLICT (content): Merge conflict in modules/nw-ingress-controller-configuration-parameters.adoc
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 OpenStack: Add note about ETP=Local and Octavia
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick enterprise-4.10

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.

bergerhoffer · 2022-09-27T12:58:34Z

Thanks @dulek! This is merged to 4.11 and 4.12. For 4.10 there was a conflict (due to the note above it not being there in 4.10).

Can you PTAL at this manual cherry pick PR to make sure it looks okay? #50907

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jun 2, 2022

openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 2, 2022

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jun 2, 2022

openshift-ci bot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jun 2, 2022

openshift-ci bot requested a review from eurijon June 2, 2022 12:26

maxwelldb added branch/enterprise-4.10 branch/enterprise-4.11 labels Jun 2, 2022

maxwelldb added this to the Next Release milestone Jun 2, 2022

maxwelldb assigned dulek Jun 2, 2022

maxwelldb reviewed Jun 2, 2022

View reviewed changes

dulek force-pushed the openstack-etp-local branch from f98f663 to b52b65f Compare June 2, 2022 16:00

dulek commented Jun 2, 2022

View reviewed changes

mandre reviewed Jun 3, 2022

View reviewed changes

modules/installation-osp-setting-cloud-provider-options.adoc Outdated Show resolved Hide resolved

dulek force-pushed the openstack-etp-local branch from b52b65f to 479cca0 Compare June 3, 2022 15:48

maxwelldb reviewed Jul 20, 2022

View reviewed changes

modules/nw-ingress-controller-configuration-parameters.adoc Outdated Show resolved Hide resolved

maxwelldb reviewed Jul 20, 2022

View reviewed changes

modules/installation-osp-setting-cloud-provider-options.adoc Outdated Show resolved Hide resolved

maxwelldb added the peer-review-needed Signifies that the peer review team needs to review this PR label Jul 20, 2022

dulek force-pushed the openstack-etp-local branch from 479cca0 to 6e42374 Compare July 20, 2022 16:06

bergerhoffer added the branch/enterprise-4.12 label Aug 10, 2022

kalexand-rh removed this from the Next Release milestone Aug 19, 2022

dulek force-pushed the openstack-etp-local branch from f9ad0dd to 9080985 Compare September 26, 2022 14:52

bergerhoffer added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Sep 26, 2022

bergerhoffer reviewed Sep 26, 2022

View reviewed changes

openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Sep 26, 2022

openshift-ci bot removed the merge-review-needed Signifies that the merge review team needs to review this PR label Sep 26, 2022

bergerhoffer added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR peer-review-needed Signifies that the peer review team needs to review this PR labels Sep 26, 2022

openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Sep 27, 2022

bergerhoffer added the merge-review-in-progress Signifies that the merge review team is reviewing this PR label Sep 27, 2022

bergerhoffer merged commit 06cdc73 into openshift:main Sep 27, 2022

openshift-cherrypick-robot mentioned this pull request Sep 27, 2022

[enterprise-4.12] Bug 2018481: OpenStack: Add note about ETP=Local and Octavia #50905

Merged

openshift-cherrypick-robot mentioned this pull request Sep 27, 2022

[enterprise-4.11] Bug 2018481: OpenStack: Add note about ETP=Local and Octavia #50906

Merged

bergerhoffer mentioned this pull request Sep 27, 2022

[enterprise-4.10] OpenStack: Add note about ETP=Local and Octavia #50907

Merged

1 task

bergerhoffer removed merge-review-in-progress Signifies that the merge review team is reviewing this PR merge-review-needed Signifies that the merge review team needs to review this PR labels Sep 27, 2022

maxwelldb mentioned this pull request Feb 16, 2023

OpenStack: Fix endpointTrafficPolicy mistake #55989

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2018481: OpenStack: Add note about ETP=Local and Octavia #46245

Bug 2018481: OpenStack: Add note about ETP=Local and Octavia #46245

dulek commented Jun 2, 2022 •

edited by maxwelldb

openshift-ci bot commented Jun 2, 2022

dulek commented Jun 2, 2022

openshift-ci bot commented Jun 2, 2022

maxwelldb left a comment

maxwelldb Jun 2, 2022

maxwelldb Jun 2, 2022

dulek Jun 2, 2022

maxwelldb Jun 2, 2022

dulek Jun 3, 2022

dulek Jun 3, 2022

maxwelldb Jun 3, 2022

dulek commented Jun 2, 2022

mandre left a comment

dulek commented Jun 3, 2022

dulek commented Jul 20, 2022

maxwelldb commented Jul 20, 2022

dulek commented Jul 20, 2022

bergerhoffer commented Aug 10, 2022

dulek commented Sep 26, 2022

maxwelldb commented Sep 26, 2022

bergerhoffer left a comment

bergerhoffer commented Sep 26, 2022 •

edited

dulek commented Sep 27, 2022

openshift-ci bot commented Sep 27, 2022

bergerhoffer commented Sep 27, 2022

bergerhoffer commented Sep 27, 2022

bergerhoffer commented Sep 27, 2022

openshift-cherrypick-robot commented Sep 27, 2022

openshift-cherrypick-robot commented Sep 27, 2022

openshift-cherrypick-robot commented Sep 27, 2022

bergerhoffer commented Sep 27, 2022

	<6> This property is required when `create-monitor` is set to `True`. It controls timeout of the monitor requests.
	<6> This property is required when `create-monitor` is set to `True`. It sets how long monitoring requests take to time out in seconds.

	<7> This property is required when `create-monitor` is set to `True`. It controls how many successful monitor requests are needed before load balancer member is marked as online.
	<7> This property is required when `create-monitor` is set to `True`. It controls how many successful monitor requests are needed before a load balancer member is marked as online.

Bug 2018481: OpenStack: Add note about ETP=Local and Octavia #46245

Bug 2018481: OpenStack: Add note about ETP=Local and Octavia #46245

Conversation

dulek commented Jun 2, 2022 • edited by maxwelldb

openshift-ci bot commented Jun 2, 2022

dulek commented Jun 2, 2022

openshift-ci bot commented Jun 2, 2022

maxwelldb left a comment

Choose a reason for hiding this comment

maxwelldb Jun 2, 2022

Choose a reason for hiding this comment

maxwelldb Jun 2, 2022

Choose a reason for hiding this comment

dulek Jun 2, 2022

Choose a reason for hiding this comment

maxwelldb Jun 2, 2022

Choose a reason for hiding this comment

dulek Jun 3, 2022

Choose a reason for hiding this comment

dulek Jun 3, 2022

Choose a reason for hiding this comment

maxwelldb Jun 3, 2022

Choose a reason for hiding this comment

dulek commented Jun 2, 2022

mandre left a comment

Choose a reason for hiding this comment

dulek commented Jun 3, 2022

dulek commented Jul 20, 2022

maxwelldb commented Jul 20, 2022

dulek commented Jul 20, 2022

bergerhoffer commented Aug 10, 2022

dulek commented Sep 26, 2022

maxwelldb commented Sep 26, 2022

bergerhoffer left a comment

Choose a reason for hiding this comment

bergerhoffer commented Sep 26, 2022 • edited

dulek commented Sep 27, 2022

openshift-ci bot commented Sep 27, 2022

bergerhoffer commented Sep 27, 2022

bergerhoffer commented Sep 27, 2022

bergerhoffer commented Sep 27, 2022

openshift-cherrypick-robot commented Sep 27, 2022

openshift-cherrypick-robot commented Sep 27, 2022

openshift-cherrypick-robot commented Sep 27, 2022

bergerhoffer commented Sep 27, 2022

dulek commented Jun 2, 2022 •

edited by maxwelldb

bergerhoffer commented Sep 26, 2022 •

edited