Skip to content

Conversation

kquinn1204
Copy link
Contributor

@kquinn1204 kquinn1204 commented Oct 4, 2024

[OCPBUGS-25129: metallb should make sure that secondary nics are forwarding traffic

Version(s):4.14, 4.15, 4.16, 4.17, 4.18 and main

Issue:https://issues.redhat.com/browse/OCPBUGS-25129

Link to docs preview:https://83087--ocpdocs-pr.netlify.app/openshift-enterprise/latest/networking/metallb/about-advertising-ipaddresspool.html#nw-metallb-configure-secondary-interface_about-advertising-ip-address-pool

QE review:

  • QE has approved this change.

Additional information:

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 4, 2024
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Oct 4, 2024

@kquinn1204
Copy link
Contributor Author

@asood-rh @fedepaol Can you please review, wonder how I could add some verification steps, I tried creating a secondary network but not sure I was successful

@kquinn1204 kquinn1204 force-pushed the OCPBUGS-25129 branch 2 times, most recently from 91dc592 to 681a70c Compare October 11, 2024 09:47
@kquinn1204
Copy link
Contributor Author

/label peer-review-needed

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label Oct 11, 2024
[id="nw-metallb-configure-secondary-interface_{context}"]
= Configuring MetalLb with secondary networks

From {product-title} 4.14 the default behavior is to not allow forwarding of IP packets between network interfaces. Therefore when MetalLb is configured on a secondary interfaces which is common practice you need to add a machine configuration to enable IP forwarding for only the required interfaces.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [error] Vale.Terms: Use 'MetalLB' instead of 'MetalLb'.

@abhatt-rh
Copy link
Contributor

/label peer-review-in-progress

Copy link
Contributor

@abhatt-rh abhatt-rh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kquinn1204, I just added some nits, else LGTM!

/remove-label peer-review-in-progress
/remove-label peer-review-needed
/label peer-review-done

osImageURL: ""
----
+
<1> Node role where you want to enable IP forwarding for example `worker`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<1> Node role where you want to enable IP forwarding for example `worker`
<1> Node role where you want to enable IP forwarding, for example, `worker`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kquinn1204 There is another way to enable forwarding by using flag ipForwarding

oc patch network.operator cluster -p '{"spec":{"defaultNetwork":{"ovnKubernetesConfig":{"gatewayConfig":{"ipForwarding": "Global"}}}}}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @asood-rh I saw that "Change install-config manifests prior to deployment to reflect the new parameter and globally enable forwarding between all interfaces." Do I need to change install-config as I have also been assigned this https://issues.redhat.com/browse/TELCODOCS-1853 with mentions simply setting ipForwarding: Global. Seems to be a couple tied to this change. What is preferred method globally or as in this example ob specific secondary interface using MachineConfig.

@@ -0,0 +1,49 @@
:_mod-docs-content-type: PROCEDURE
[id="nw-metallb-configure-secondary-interface_{context}"]
= Configuring MetalLb with secondary networks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
= Configuring MetalLb with secondary networks
= Configuring MetalLB with secondary networks

@openshift-ci openshift-ci bot 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 Oct 11, 2024
@asood-rh
Copy link

/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Oct 14, 2024
Copy link

openshift-ci bot commented Oct 14, 2024

New changes are detected. LGTM label has been removed.

@kquinn1204
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Oct 15, 2024
Copy link

openshift-ci bot commented Oct 15, 2024

@kquinn1204: 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-sigs/prow repository. I understand the commands that are listed here.

Copy link
Contributor

@ShaunaDiaz ShaunaDiaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One FYI, not a merge breaker as far as I can see.

= Configuring MetalLB with secondary networks

From {product-title} 4.14 the default network behavior is to not allow forwarding of IP packets between network interfaces. Therefore, when MetalLB is configured on a secondary interface, you need to add a machine configuration to enable IP forwarding for only the required interfaces.
[NOTE]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[NOTE]
[NOTE]

Renders OK on the docs.openshift.com preview, but you'll want to check the docs.redhat.com preview after merge; hard to say if the lack of the line space breaks anything there.

@ShaunaDiaz ShaunaDiaz removed the merge-review-needed Signifies that the merge review team needs to review this PR label Oct 15, 2024
@ShaunaDiaz ShaunaDiaz merged commit 0c6569e into openshift:main Oct 15, 2024
2 checks passed
@ShaunaDiaz
Copy link
Contributor

/cherrypick enterprise-4.14

@ShaunaDiaz
Copy link
Contributor

/cherrypick enterprise-4.15

@ShaunaDiaz
Copy link
Contributor

/cherrypick enterprise-4.16

@ShaunaDiaz
Copy link
Contributor

/cherrypick enterprise-4.17

@ShaunaDiaz
Copy link
Contributor

/cherrypick enterprise-4.18

@openshift-cherrypick-robot

@ShaunaDiaz: new pull request created: #83521

In response to this:

/cherrypick enterprise-4.14

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-sigs/prow repository.

@openshift-cherrypick-robot

@ShaunaDiaz: new pull request created: #83522

In response to this:

/cherrypick enterprise-4.15

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-sigs/prow repository.

@openshift-cherrypick-robot

@ShaunaDiaz: new pull request created: #83523

In response to this:

/cherrypick enterprise-4.16

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-sigs/prow repository.

@openshift-cherrypick-robot

@ShaunaDiaz: new pull request created: #83524

In response to this:

/cherrypick enterprise-4.17

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-sigs/prow repository.

@openshift-cherrypick-robot

@ShaunaDiaz: new pull request created: #83525

In response to this:

/cherrypick enterprise-4.18

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.14 branch/enterprise-4.15 branch/enterprise-4.16 branch/enterprise-4.17 branch/enterprise-4.18 peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants