Skip to content
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.

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

Adds guide for adding a hint file to the node ip configuration #52200

Closed
wants to merge 1 commit into from

Conversation

stevsmit
Copy link
Member

@stevsmit stevsmit commented Oct 26, 2022

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

ocpdocs-previewbot commented Oct 26, 2022

🤖 Updated build preview is available at:
https://52200--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/5597

@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 27, 2022
@rbbratta
Copy link

rbbratta commented Nov 3, 2022

Linking back to old nodeip doc BZ https://bugzilla.redhat.com/show_bug.cgi?id=2105010 This should eventually resolve it.

@cybertron
Copy link
Member

Linking back to old nodeip doc BZ https://bugzilla.redhat.com/show_bug.cgi?id=2105010 This should eventually resolve it.

Oh wow, I completely forgot I opened that. Maybe we should just remove the old method then and rely on the new override mechanism. We can always revisit this if it turns out that the new method is insufficient for someone.

@stevsmit
Copy link
Member Author

stevsmit commented Nov 3, 2022

@cybertron @rbbratta

To be sure, we're talking about removing the entirety of "How the network interface is selected", correct?

@cybertron
Copy link
Member

No, we still need this section to discuss the new method. However, I think we should remove everything from "In some situations, creating a hint file might be insufficient..." on.

Also, looking at this in context a bit more I'm realizing that some of the other parts should be changed. For example, "The service iterates through the network interfaces on the node and the first network interface that is configured with a subnet than can host the IP address for the API server is selected for {product-title} communication." is describing the IPI behavior, which we do not support overriding because it breaks connectivity to the VIPs. It should say that nodeip-configuration selects the IP from the interface associated with the default route.

This is also not only relevant in case of hardware or networking changes. This is needed any time there is a networking layout where the node ip should not come from the default route interface, which is more commonly going to happen on day 1. It appears I did not review this documentation carefully enough when it was first added. :-/

@stevsmit stevsmit added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. branch/enterprise-4.12 labels Nov 7, 2022
@stevsmit stevsmit added this to the Planned for 4.12 GA milestone Nov 7, 2022
Copy link
Member

@cybertron cybertron left a comment

Choose a reason for hiding this comment

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

Okay, we still have some issues here. I left some comments inline, but overall they boil down to this should be included in the UPI docs but not the IPI docs, and it shouldn't be necessary to write a procedure to determine the available address ranges because those have to be provided as part of the installation regardless of whether you do this or not. If we just remove that section entirely a lot of the problems I noted go away.

modules/overriding-default-node-ip-selection-logic.adoc Outdated Show resolved Hide resolved
modules/overriding-default-node-ip-selection-logic.adoc Outdated Show resolved Hide resolved
modules/overriding-default-node-ip-selection-logic.adoc Outdated Show resolved Hide resolved
modules/nw-how-nw-iface-selected.adoc Outdated Show resolved Hide resolved
modules/overriding-default-node-ip-selection-logic.adoc Outdated Show resolved Hide resolved
modules/overriding-default-node-ip-selection-logic.adoc Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2022
@stevsmit stevsmit force-pushed the opnet-150-1 branch 6 times, most recently from 30eb19a to a49947a Compare November 10, 2022 15:06
@stevsmit
Copy link
Member Author

@rbbratta @cybertron please take a look. I have made all request changes, including to the procedure and location of the docs. Thanks.

@cybertron
Copy link
Member

/lgtm

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2022
@rbbratta
Copy link

/lgtm

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2022
@rbbratta
Copy link

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2022
Updates the proc that may not be needed
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2022
@openshift-ci
Copy link

openshift-ci bot commented Dec 19, 2022

New changes are detected. LGTM label has been removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.12 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants