Skip to content

TELCODOCS-2037 - Add Dynamic ASN #84440

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 18, 2024
Merged

TELCODOCS-2037 - Add Dynamic ASN #84440

merged 1 commit into from
Nov 18, 2024
+20 −6

Conversation

slovern
Copy link
Contributor

@slovern slovern commented Nov 4, 2024 •
edited
Loading

TELCODOCS-2037 - CNF-13767 - MetalLB: BGP Dynamic AS Number

Version(s): 4.18

Issue: https://issues.redhat.com/browse/TELCODOCS-2037

Link to docs preview:

QE review:

  • QE has approved this change.

Additional information:

Sorry, something went wrong.

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 4, 2024
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Nov 4, 2024 •
edited
Loading

🤖 Fri Nov 15 16:56:48 - Prow CI generated the docs preview:

https://84440--ocpdocs-pr.netlify.app/openshift-enterprise/latest/networking/metallb/metallb-configure-bgp-peers.html
https://84440--ocpdocs-pr.netlify.app/openshift-enterprise/latest/networking/metallb/metallb-frr-k8s.html

oribon
oribon reviewed Nov 6, 2024
View reviewed changes
modules/nw-metallb-bgppeer-cr.adoc Outdated
|`spec.dynamicASN`
|`string`
| Detects the ASN to use for the remote end of the session without explicitly setting it.
Specify `internal` if the routers have the same ASN or `external` if the routers have different ASNs. If you use this field, you cannot specify a value in the `spec.peerASN` field.
Copy link

Choose a reason for hiding this comment

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

on second thought maybe instead of "the routers" we should use "the peer", since it seems we don't refer to the cluster nodes as "routers" anywhere else. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the review @oribon

modules/nw-metallb-frr-k8s-configuration-crd.adoc Outdated
|`spec.bgp.routers.neighbors.dynamicASN`
|`string`
|Detects the ASN to use for the remote end of the session without explicitly setting it.
Specify `internal` if the routers have the same ASN or `external` if the routers have different ASNs.
Copy link

Choose a reason for hiding this comment

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

same as the other comment (but with neighbor instead of peer)

@slovern slovern force-pushed the TELCODOCS-2037 branch 2 times, most recently from 9a8f63a to 3b83718 Compare November 7, 2024 16:27
@oribon
Copy link

oribon commented Nov 7, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2024
gkopels
gkopels reviewed Nov 15, 2024
View reviewed changes
modules/nw-metallb-frr-k8s-configuration-crd.adoc

|`spec.bgp.routers.neighbors.dynamicASN`
|`string`
|Detects the ASN to use for the remote end of the session without explicitly setting it.
Copy link

Choose a reason for hiding this comment

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

Do you want to add here that it is detected in the OPEN message sent during session establishment?

@gkopels
Copy link

gkopels commented Nov 15, 2024

lgtm

@slovern
Copy link
Contributor Author

slovern commented Nov 15, 2024

/label telco

@openshift-ci openshift-ci bot added the telco Label for all Telco PRs label Nov 15, 2024
@slovern
Copy link
Contributor Author

slovern commented Nov 15, 2024

/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 Nov 15, 2024
@lahinson lahinson added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Nov 15, 2024
lahinson
lahinson reviewed Nov 15, 2024
View reviewed changes
Copy link
Contributor

@lahinson lahinson left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few minor suggestions for your consideration.

modules/nw-metallb-bgppeer-cr.adoc Outdated
@@ -26,14 +26,21 @@ The fields for the BGP peer custom resource are described in the following table

|`spec.myASN`
|`integer`
|Specifies the Autonomous System number for the local end of the BGP session.
|Specifies the Autonomous System Number (ASN) for the local end of the BGP session.
Specify the same value in all BGP peer custom resources that you add.
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
Specify the same value in all BGP peer custom resources that you add.
In all BGP peer custom resources that you add, specify the same value.

Because the line just before this starts with "Specifies", consider revising this line to start differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @lahinson
Makes sense

modules/nw-metallb-bgppeer-cr.adoc Outdated
|`spec.dynamicASN`
|`string`
| Detects the ASN to use for the remote end of the session without explicitly setting it.
Specify `internal` for a peer with the same ASN or `external` if the peer has a different ASN.
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
Specify `internal` for a peer with the same ASN or `external` if the peer has a different ASN.
Specify `internal` for a peer with the same ASN or `external` for a peer with a different ASN.

modules/nw-metallb-frr-k8s-configuration-crd.adoc Outdated
|`spec.bgp.routers.neighbors.dynamicASN`
|`string`
|Detects the ASN to use for the remote end of the session without explicitly setting it.
Specify `internal` for a neighbor with the same ASN, or `external` if the neighbor has a different ASN.
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
Specify `internal` for a neighbor with the same ASN, or `external` if the neighbor has a different ASN.
Specify `internal` for a neighbor with the same ASN, or `external` for a neighbor with a different ASN.

@lahinson lahinson added peer-review-done Signifies that the peer review team has reviewed this PR branch/enterprise-4.18 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 Nov 15, 2024
@lahinson lahinson added this to the Planned for 4.18 GA milestone Nov 15, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Nov 15, 2024

New changes are detected. LGTM label has been removed.

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Nov 15, 2024

@slovern: 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.

@slovern
Copy link
Contributor Author

slovern commented Nov 15, 2024

/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 Nov 15, 2024
@stevsmit stevsmit added merge-review-in-progress Signifies that the merge review team is reviewing this PR lgtm Indicates that a PR is ready to be merged. and removed merge-review-needed Signifies that the merge review team needs to review this PR merge-review-in-progress Signifies that the merge review team is reviewing this PR labels Nov 18, 2024
@stevsmit stevsmit merged commit 02413f2 into openshift:main Nov 18, 2024
2 checks passed
@stevsmit
Copy link
Member

/cherry-pick enterprise-4.18

@openshift-cherrypick-robot
Copy link

@stevsmit: new pull request created: #85077

In response to this:

/cherry-pick 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.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Nov 18, 2024
Merged
This was referenced May 1, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oribon oribon

@lahinson lahinson

@gkopels gkopels

Assignees

@oribon oribon

Labels
branch/enterprise-4.18 lgtm Indicates that a PR is ready to be merged. peer-review-done Signifies that the peer review team has reviewed this PR size/S Denotes a PR that changes 10-29 lines, ignoring generated files. telco Label for all Telco PRs
Projects
None yet
Milestone
OCP 4.18 GA
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@slovern @ocpdocs-previewbot @oribon @gkopels @stevsmit @openshift-cherrypick-robot @lahinson