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

Discuss host network configuration interfaces #394

Merged
merged 3 commits into from
Aug 31, 2020

Conversation

russellb
Copy link
Member

This change introduces a new enhancement that discusses host network
configuration. It is different from the typical enhancement in that
its goal is informational and to discuss what is already present in
this area. It also provides references to other related works in
progress.

I wrote this first for myself and propose it here in hopes that it may
help others. I find that this context is important and helpful to
understand when discussing or reviewing enhancements for a specific
feature related to host network configuration.

I imagine this as a living document that should get updated as key
improvements are made to how we manage host network configuration for
OpenShift.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 30, 2020
@russellb
Copy link
Member Author

enhancements that this one may reference.


## Use Cases
Copy link
Contributor

Choose a reason for hiding this comment

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

So this talks about some of the features that are needed, but it doesn't explain why they're needed, which would help in understanding the problem space. Why would the customer's environment be set up so that we need to use a VLAN? Why would someone want to manually configure networking on each host rather than setting up a DHCP server to do it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this section is pretty weak. I tried to add a bit more text to each use case listed, but more could certainly be added here over time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be helpful to outline common configurations and how they would be achieved today - it might also help highlight the issues around interfaces, e.g there are multiple partially overlapping interfaces the user gets exposed to (dracut, coreos-install, ignition, nmstate)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, more realistic examples would be great. I view this as a living document where we could submit enhancements like this over time as we come across good examples.

enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved

Some clusters may want to allocate SR-IOV devices to workloads for a secondary
network interface with high performance. These devices are managed by the
[SR-IOV Operator[(https://github.com/openshift/sriov-network-operator).
Copy link
Contributor

Choose a reason for hiding this comment

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

close bracket is backwards.

also this is pretty vague compared to everything else in the doc

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed the bracket. I haven't added more text yet. I honestly don't know much of anything about the SR-IOV operator, so that's why it's vague. :-) I need to look at it more closely!

enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
@russellb
Copy link
Member Author

russellb commented Jul 1, 2020

@danwinship tried to address most of your feedback in the last commit I pushed

@russellb russellb force-pushed the host-network-config branch from 15e368d to becb9c6 Compare July 7, 2020 18:21
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

Thanks for writing this. Nice summary!

enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
Comment on lines +393 to +444
One reason for this “pointer” ignition approach is that on many cloud platforms
the size limit for host initialization information through their API is very
low. To get around the size limit, we provide a small config that contains
instructions for how to download the full configuration at runtime. For
baremetal deployments this size limit is likely to be much higher, so it may be
possible to provide the entire configuration to each node, which potentially
removes the runtime requirement to download the rendered configuration.
Copy link
Member

Choose a reason for hiding this comment

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

Yes. If we can get a flattened ignition config then the work that we're already doing to "make the initramfs only bring up networking if it needs to" (coreos/fedora-coreos-tracker#443) will mean the machine won't attempt to bring up networking in the initramfs and all networking configuration can be applied via Ignition.

Copy link
Contributor

@danwinship danwinship left a comment

Choose a reason for hiding this comment

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

Should deal with the remaining unresolved comments (especially the no-auto-default=* one) and get this merged...

enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
enhancements/host-network-configuration.md Outdated Show resolved Hide resolved
@danwinship
Copy link
Contributor

mostly lgtm but you should probably address #394 (comment) before merging

@russellb
Copy link
Member Author

mostly lgtm but you should probably address #394 (comment) before merging

I did the follow-up I mentioned and tagged dustymabe on the doc where I got this example. I haven't tested the suggested change myself so hadn't applied it here. I'll try adding it to this doc anyway now.

This change introduces a new enhancement that discusses host network
configuration.  It is different from the typical enhancement in that
its goal is informational and to discuss what is already present in
this area.  It also provides references to other related works in
progress.

I wrote this first for myself and propose it here in hopes that it may
help others.  I find that this context is important and helpful to
understand when discussing or reviewing enhancements for a specific
feature related to host network configuration.

I imagine this as a living document that should get updated as key
improvements are made to how we manage host network configuration for
OpenShift.
@russellb russellb force-pushed the host-network-config branch from 32e3b76 to b360f20 Compare August 10, 2020 14:11
@russellb
Copy link
Member Author

mostly lgtm but you should probably address #394 (comment) before merging

I did the follow-up I mentioned and tagged dustymabe on the doc where I got this example. I haven't tested the suggested change myself so hadn't applied it here. I'll try adding it to this doc anyway now.

done

Use DHCP in the MachineConfig example instead of static IP
configuration.  Since MachineConfig applies to a pool of hosts, it is
not a suitable interface for doing static IP configuration.  Clarify
that a future API enhancement is necessary to better support static IP
configuration through the OpenShift API.
Apply suggested change from @dustymabe to reflect the updated version number for RHCOS live ISO

Co-authored-by: Dusty Mabe <[email protected]>
russellb added a commit to russellb/enhancements that referenced this pull request Aug 24, 2020
I've started writing some enhancements that are meant more to discuss
the context for a problem space and not to propose any specific changes.
One example is an overview of how BGP relates to OpenShift, and a list
of use cases that each may be explored in their own future enhancements.
Another is a doc that discusses the different interfaces related to
configuring networking on a host (see PR openshift#394).

These sorts of docs don't fit the normal status workflow, so I propose
adding a new "informational" state that covers these cases.
@cgwalters
Copy link
Member

Overall this PR is really great, thank you so much for doing this! I actually already just had cause to send the link to this document to someone asking about static IP addressing. However, I do think we need some sort of story where this text can end up in the official product documentation. TBH I am unaware of a flow for that.

@russellb
Copy link
Member Author

Overall this PR is really great, thank you so much for doing this! I actually already just had cause to send the link to this document to someone asking about static IP addressing. However, I do think we need some sort of story where this text can end up in the official product documentation. TBH I am unaware of a flow for that.

I'll look into the right place to file an issue about enhancing product docs to cover these topics, including a link to this as a source.

@russellb
Copy link
Member Author

Overall this PR is really great, thank you so much for doing this! I actually already just had cause to send the link to this document to someone asking about static IP addressing. However, I do think we need some sort of story where this text can end up in the official product documentation. TBH I am unaware of a flow for that.

I'll look into the right place to file an issue about enhancing product docs to cover these topics, including a link to this as a source.

I've made contact with our docs team regarding this doc and we will be looking at how best to incorporate it.

It seems like this PR should probably just merge and be improved and updated with follow-up PRs at this point.

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, danwinship, russellb

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:

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

@openshift-merge-robot openshift-merge-robot merged commit c6a1aa8 into openshift:master Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants