design for automatic configuration of the network#132
design for automatic configuration of the network#132zhouhao3 wants to merge 1 commit intometal3-io:mainfrom
Conversation
metal3-io-bot
added
size/L
|
Hi @zhouhao3. Thanks for your PR. I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
/assign @dhellmann |
|
PTAL |
|
This is related to #111 |
|
@maelk @dhellmann @zaneb Can you give us some suggestions? If this method is feasible, we will proceed to the coding phase. |
maelk
left a comment
maelk
left a comment
There was a problem hiding this comment.
First let me apologize for the delay in the review. I was on holidays, and then had to work through my backlog.
That is definitely a step in the good direction, thank you for the submission. There are some areas that are very unclear to me, specifically around the persona definitions and user stories, the permissions and isolation side of it was completely left out. Then around the workflows. I think the current approach should in some part be reversed. Also, there will not be only one network CR per cluster. each node can have one network CR per interface. and the configuration might be different for all nodes. Also you should consider physical hosts connected to multiple switches, where the user must be able to select specifically an interface for a configuration to be able to match it with the configuration done in cloud-init.
Overall I think this would probably need a call with all interested parties to discuss the design and make decisions on the approach we want to take. I am afraid that doing it over a PR would not be manageable considering the extent of the discussions that are expected.
And finally, please create new commits for now when you do modifications so that we can track it easily.
Thank you for this submission, let's keep the discussion rolling.
| - Define a Kubernetes API for configuring network switches. | ||
| - Automatically configure the network infrastructure for a host when adding it | ||
| to a cluster. |
There was a problem hiding this comment.
what about the following points:
- Integrate a network configuration API with CAPM3 APIs
- Support multiple southbound configuration protocols, i.e. offer a plug-in mechanism to add additional network controller such as SDN controllers or other.
- Do network configuration when deprovisioning the hosts
- design a network abstraction that can represent any of the target networking configuration, independently of the controller used.
in addition smart nics are not mentioned here.
There was a problem hiding this comment.
Good point! We have made a new design for this document based on your comments, and it will take some time to change it.
| to a cluster. | ||
|
|
||
| ### Non-Goals | ||
| - Add new Switch API to BMO. |
There was a problem hiding this comment.
what about the following point:
- implement a network controller, this is aimed at being an integration with an existing controller only
- implement a solution for a specific underlying infrastructure.
|
|
||
| ## Proposal | ||
| This document proposes to add a new mechanic to automatically perform physical | ||
| network device configuration before provisioning a BareMetalHost. |
There was a problem hiding this comment.
and after deprovisioning ? you might want to reconfigure it into a state where it cannot interact with the cluster it was part of.
There was a problem hiding this comment.
After deprovisioning, we plan to remove the interface from the previous configuration. (It will be explained in detail in a new document)
|
|
||
| #### Story 1 | ||
| As a consumer of Metal³, when adding a machine resource to the cluster, it can | ||
| automatically put the adapted BMH into the correct Switch's VLAN. |
There was a problem hiding this comment.
it might not be restricted to vlan, what about QinQ, vxlan etc. ?
There was a problem hiding this comment.
In the new document, we will take these situations into consideration.
| - Direction: ingress or egress | ||
| - Protocol: SSH, HTTP, …all | ||
| - Action: allow, deny | ||
| - Port status: up, down (status of the switch port) |
There was a problem hiding this comment.
what about also restricting the available vlans ? that would ensure that a user cannot misconfigure the switch and have a host in a vlan dedicated to another tenant
There was a problem hiding this comment.
Because we are not familiar with this, many situations are not considered. If you have a good idea about this, you can help us improve this story, thank you very much.
| - Whenever a new Interface CR is created, Interface Controller will find the | ||
| corresponding SwitchPort CR according to `.spec.portID` and | ||
| `.spec.switchMacaddress`, and write the reference of the SwitchPort CR into | ||
| `spec.neighborRef`. |
There was a problem hiding this comment.
How would the controller know which switch this is the port of ? I think we should rather give the switch reference in the spec. We could get the switch name through lldp and match it with a switch CR, but we must consider cases where we do not have LLDP, so the linking must be done manually by the admin.
| interfaces: | ||
| - name: bm0-eth0 | ||
| - name: bm1-eth1 |
There was a problem hiding this comment.
that should rather be in the status, otherwise there are higher risks of misconfiguration, if it can be done by the user. I proposed to reverse that workflow.
| port: 22 | ||
| // Store the username and password of the switch | ||
| // in the form of secret | ||
| accessSecret: example-switch-secret |
There was a problem hiding this comment.
we should probably have some info that can be matched through LLDP like the actual switch name (not the CR name). Maybe we could add here also a map of the switch ports, referencing the switchport CRs maybe, and listing things like allowed configurations for that port.
| ownerRef: | ||
| name: sample-switch |
| - Implementation of Switch, Network and Interface Controller | ||
| - Change the API of Metal3Cluster and BareMetalHost | ||
| - Unit tests | ||
| - E2e tests in metal3-dev-env |
There was a problem hiding this comment.
This proposal does not say anything about a test setup. Metal3-dev-env switches are for now not configurable. it means we would need to switch to OVS probably (the setup with libvirt must be checked), and the maybe run an SDN controller on top, to not directly interact with OVS ? However, metal3-dev-env should probably still support deployments without the network APIs part, i.e. as it is now, being backwards compatible. This kind of changes should be addressed in the proposal too.
| design: | ||
| - Switch CRD: | ||
| - Switch CR represents a physical switch. It contains the system information | ||
| (vendor, OS, ...) and the access information of the switch. |
There was a problem hiding this comment.
If we move ports to a separate namespace, we have to worry about duplication and conflicts.
Perhaps tenants should not be consuming ports directly at all, but using something like a claim to ask for configuration changes on the switch? I'm not sure that really resolves the issue with conflicts, though.
| the reference of that SwitchPort CR in the Interface CR. | ||
| - CAPM3: | ||
| - Gets the LLDP information of all the interfaces in the host. If there is no | ||
| corresponding Interface CR, create one. |
There was a problem hiding this comment.
Why would CAPM3 create these? Why not a new controller?
| - CAPM3: | ||
| - Gets the LLDP information of all the interfaces in the host. If there is no | ||
| corresponding Interface CR, create one. | ||
| - If a host has multiple interfaces, selects one for network configuration. |
There was a problem hiding this comment.
If a host has multiple interfaces, I would assume they all need some configuration. So it's not a matter of picking one, it's a matter of determining the correct configuration for each interface. We still need more detail about how that would work, but perhaps lines 102-103 describe the manual process for establishing the right links?
| - Interface controller: | ||
| - Monitors Interface CRs. When there is a new Interface CR, find the | ||
| corresponding SwitchPort CR according to the provided information and store | ||
| the reference of that SwitchPort CR in the Interface CR. |
There was a problem hiding this comment.
Why does the switch port also need to be updated?
|
|
||
| - After an Interface is put into the provisioning network, the LLDP information | ||
| of other interfaces can be obtained and network configuration can be | ||
| performed. |
There was a problem hiding this comment.
This section needs to be rewritten to cover what is doing each action (obtaining the LLDP information and performing the network configuration).
|
|
||
| The workflow of a basic user story: | ||
| - Administrator: | ||
| - Creates a Network resource used as provisioning network. |
There was a problem hiding this comment.
I think we're going to want 2 levels to the API, to manage the RBAC. The low level will include the actual physical connections, and only admins should be able to use those resources. The higher level will describe a desired configuration from the tenant's perspective in a more general way that can be mapped to the hardware configuration (2 separate networks with specific VLAN IDs, for example).
We should also consider the SDN case, and whether that changes the way we need to define networks to go beyond just a VLAN.
It doesn't feel like we should be inventing this from nothing. We should be able to learn from the APIs of other tools like AWS, OpenStack, etc. What do those look like? How do they separate the RBAC concerns?
| - The Switch controller will configure the switch based on the SwitchPort | ||
| CR. | ||
| - After the switch configuration is completed, the BMH is connected to the | ||
| provisioning network. Ironic will perform introspection and update the |
There was a problem hiding this comment.
The baremetal-operator is going to try to perform inspection as soon as it has BMC credentials for the host, which happens up on line 116. If the host has to be attached to the provisioning network before that inspection process will work, then the admin is going to have to do that very early in the process, and the tenant won't get to pick the network.
| corresponding Interface CR if it is not created. | ||
| - User: | ||
| - Creates production Network CRs. | ||
| - Creates a Cluster and specify its `.spec.networkRef`. |
There was a problem hiding this comment.
Do people really run clusters with the hosts on different networks? Do you mean for the secondary networks or the main one where the ingress traffic is?
| - After CAPM3 observes that the SwitchPort configuration is complete, it | ||
| triggers provisioning via the provisioning network. |
There was a problem hiding this comment.
Yes, I think we can set the ownerRef of the host to avoid having something else claim it and the when we're ready to provision set the image details.
| status: | ||
| … | ||
| hardware: | ||
| … | ||
| nics: | ||
| // The following data is obtained by Baremetal Operator and | ||
| // filled with each lldp information. | ||
| - name: eth0 | ||
| ... | ||
| lldp: | ||
| - name: eth1 | ||
| ... |
There was a problem hiding this comment.
I think the API would be easier to understand if the interfaces were linked to NICs via a name or other value instead of just the order.
zhouhao3
left a comment
zhouhao3
left a comment
There was a problem hiding this comment.
Thank you very much for your suggestions. We will redesign this document based on your suggestions. After the document is updated, we will tell you the main update points, and hope to get your suggestions at that time.
| - Define a Kubernetes API for configuring network switches. | ||
| - Automatically configure the network infrastructure for a host when adding it | ||
| to a cluster. |
There was a problem hiding this comment.
Good point! We have made a new design for this document based on your comments, and it will take some time to change it.
|
|
||
| ## Proposal | ||
| This document proposes to add a new mechanic to automatically perform physical | ||
| network device configuration before provisioning a BareMetalHost. |
There was a problem hiding this comment.
After deprovisioning, we plan to remove the interface from the previous configuration. (It will be explained in detail in a new document)
|
|
||
| #### Story 1 | ||
| As a consumer of Metal³, when adding a machine resource to the cluster, it can | ||
| automatically put the adapted BMH into the correct Switch's VLAN. |
There was a problem hiding this comment.
In the new document, we will take these situations into consideration.
| - Direction: ingress or egress | ||
| - Protocol: SSH, HTTP, …all | ||
| - Action: allow, deny | ||
| - Port status: up, down (status of the switch port) |
There was a problem hiding this comment.
Because we are not familiar with this, many situations are not considered. If you have a good idea about this, you can help us improve this story, thank you very much.
| design: | ||
| - Switch CRD: | ||
| - Switch CR represents a physical switch. It contains the system information | ||
| (vendor, OS, ...) and the access information of the switch. |
There was a problem hiding this comment.
Regarding the issue of RBAC, we will explain in the new design document.
|
@dhellmann @maelk We have updated the design, Contains new three CRDs:
And a new controller:
PTAL |
|
Thanks for the update, i'll try to have a look by the end of the week |
|
I took a look at the new proposal. Could we maybe have a meeting to discuss it ? We could bring more people on this proposal. We can schedule something in the cluster-api-baremetal channel on slack if ok for you |
|
Agree about the meeting. We also think it is difficult to design the API only by us and we need the knowledge and experience about network from the community. But please schedule the meeting after October 8th as we will soon enter a national holiday. |
|
Which timezone are you in ? (to see which slots to propose) |
|
Most of us are in UTC+8, the others are in UTC+9 |
|
I have created a doodle for next week, please select the times that would work for you : https://doodle.com/poll/ai96y8ev7iu22hcn |
|
Sorry for the late response, I have answered the doodle. |
|
Let's make it Friday 12:00 UTC. I will send an invite to the metal3-dev group. do you want to receive it on a specific email address ? |
|
Similarly to how we use ironic instead of re-implementing all various quirks of hardware management, have you considered using some existing backend? I know some people use ironic together with https://opendev.org/x/networking-ansible. Also, not sure if you know it or not, but ironic has support for automating VLAN changes to use a separate provisioning network (completely isolated from other flows). Currently it requires OpenStack Neutron, but we're open to revisiting standalone networking operations if there is enough interest. |
|
@maelk No, the metal3-dev group will be enough. Thanks for arranging the meeting. |
|
Notes from the meeting : https://docs.google.com/document/d/1ES_I9P1-EI2xS6hWqOGXMBIcwk4EPaae9Zv755VHVj4/edit (recording link at the top) |
|
Thanks for the note and recording. The meeting is very helpful. |
|
Once you'll have time to rework the base of the proposal, we could setup a second one, to discuss in more details the permissions and also the different restrictions we could put in place, such as allowing only specific vlans for a specific tenant. |
|
@fmuyassarov: GitHub didn't allow me to request PR reviews from the following users: sshukun. Note that only metal3-io members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
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. |
|
this lgtm. We can put the lazy consensus, IMO |
|
I'm also fine with the lazy consesus |
|
If there are no objections, I would suggest to have a week of lazy consensus from today. As such, if there are no comments to be addresses by 15.10, we should be good to merge IMO. |
zhouhao3
force-pushed
the
network-configure
branch
from
f89ccfb to
6d4dbbb
Compare
| separation between namespaces. | ||
| - Full multi-tenancy : one set of CRs (the most up-to-date), one set of | ||
| webhooks. All other controllers per namespace. No permission separation | ||
| within the same namespace. |
There was a problem hiding this comment.
I would not worry so much about running multiple controllers for different namespaces (this is never going to work well for k8s due to the CRD and webhooks being global, as you've identified).
What is more interesting is making sure that roles can be separate. So e.g. an administrator can assign a given Host to one of several tenants (each with their own namespace) and allow the tenants to control how it is provisioned (e.g. by using CAPM3, or something else) while making sure that e.g. only the administrator can define which networks the Host can connect to.
This will only actually be possible once the BaremetalHost itself is decomposed in some way along the lines of my draft proposal in https://docs.google.com/document/d/1FL8oA0_WNcPdiC-0zrToinl8aol-NPQUjWe-OkcvTqA/edit?usp=sharing (you must be a member of the Metal³ Google Group to view this doc), but it is worth thinking about how the network configuration would work in such a scenario. In particular, I think this scenario suggests that the network config should in future be associated with a BaremetalAllocation, not a BaremetalImage (i.e. deployment), and it follows from that perhaps the network configuration should precede the Host being selected by the CAPM3 rather than being part of the provisioning done by CAPM3.
| team is the owner of the management cluster and manages the physical resources. | ||
| A consumer is the manager of the target cluster, responsible for its lifecycle. | ||
| The consumer gets resources from the infra team (tenant). No consumer action | ||
| should impact another consumer. Metal3 setup on a shared management cluster, |
There was a problem hiding this comment.
I agree that no consumer action should impact another consumer, but the rules below don't enforce that because they assume that the only resource that is exclusive to a consumer is the Port.
In reality, the common multitenancy use case would be to assign different VLANs to different consumers. Allowing each consumer to configure their own ports in any way they want would permit them to connect their hosts to any VLAN.
There was a problem hiding this comment.
This is indeed a problem that we did not consider, and we intend to solve it in the following ways:
Each user can specify any vlan. If this vlan is used by other users in the switch, it will randomly find an unused vlan and put the port in this new vlan. But the user does not know, only the operator knows and records this information. This allows different users to specify the same vlan, and the actual configuration is not in the same vlan. WDYT?
There was a problem hiding this comment.
I would very strongly advise against that.
My intention here was to point out one of the ways in which the current model fails to account for multitenancy of the network as discussed above. (I suspect this is also what @maelk was getting at when he originally suggested documenting the approach to RBAC.) We should look at the problem in that context.
There was a problem hiding this comment.
Well, in this case, the administrator should assign a specified VLAN range to each user, and the network-operator ensures that each user can only use/configure the VLAN specified by the administrator.
We plan to add a configuration file, and the administrator will write the VLAN allocation range for each user in this file according to the actual situation. The network-operator reads this configuration file through ConfigMap, and ensures that this user can use this VLAN when configuring the port. What do you think of this method?
There was a problem hiding this comment.
I could imagine that being useful, although the config needs to be in a CRD, not in a file.
There was a problem hiding this comment.
@zaneb does the current version address your concerns? I assume the configuration-port split does it?
|
I finally finished the review I started 3 months ago. Apologies for the delay. |
zhouhao3
left a comment
zhouhao3
left a comment
There was a problem hiding this comment.
Thank you very much for your review, we need more consideration and discussion to solve the multi-tenant issue.
| team is the owner of the management cluster and manages the physical resources. | ||
| A consumer is the manager of the target cluster, responsible for its lifecycle. | ||
| The consumer gets resources from the infra team (tenant). No consumer action | ||
| should impact another consumer. Metal3 setup on a shared management cluster, |
There was a problem hiding this comment.
This is indeed a problem that we did not consider, and we intend to solve it in the following ways:
Each user can specify any vlan. If this vlan is used by other users in the switch, it will randomly find an unused vlan and put the port in this new vlan. But the user does not know, only the operator knows and records this information. This allows different users to specify the same vlan, and the actual configuration is not in the same vlan. WDYT?
| 1. BMO clears the configuration field in the switchPort CR | ||
| corresponding to the network card. | ||
| 2. SwitchPort controller starts to deconfigure the network by calling | ||
| `device.DeConfigurePort()`. Return the port to a `cleaning` state. |
There was a problem hiding this comment.
What does the "Idle" state look like after cleaning?
When performing the clean operation, we will call the delete_port function of network_runner, which will be processed according to the definition of each manufacturer, mainly to delete the vlan-related configuration.
Is there some default config for ports that are not explicitly configured?
No, ports without a specified configuration file will not be configured
How do you differentiate between ports that are 'deconfigured' and ports that are simply not managed by the controller
We have a Disabled field in Switch.ports to indicate whether this port is available or controllable.
zhouhao3
force-pushed
the
network-configure
branch
from
6d4dbbb to
b1c3d48
Compare
|
@zaneb We have updated the proposal based on your suggestions, including updated user stories, added a new CR to restrict users' use of vlan, etc. Please review again, thank you! |
zhouhao3
force-pushed
the
network-configure
branch
from
b1c3d48 to
6af8d71
Compare
|
There are some problems with the previous design:
So we updated the proposal and added SwitchResource Controller for monitoring. |
zhouhao3
force-pushed
the
network-configure
branch
from
6af8d71 to
7f2a424
Compare
zhouhao3
force-pushed
the
network-configure
branch
2 times, most recently
from
60f4a29 to
b347466
Compare
|
Hi @zaneb, as I said above, we have modified the implementation structure of this proposal. Could you please review again? Thanks! |
| team is the owner of the management cluster and manages the physical resources. | ||
| A consumer is the manager of the target cluster, responsible for its lifecycle. | ||
| The consumer gets resources from the infra team (tenant). No consumer action | ||
| should impact another consumer. Metal3 setup on a shared management cluster, |
There was a problem hiding this comment.
@zaneb does the current version address your concerns? I assume the configuration-port split does it?
zhouhao3
force-pushed
the
network-configure
branch
from
b347466 to
afefcff
Compare
| That controller need to monitor the corresponding `DevicePort`, | ||
| and whenever the `Configuration` of a `DevicePort` is changed, | ||
| the controller should be able to interact with the `Device` through | ||
| the `ProviderDevice` to do the configuration job. |
There was a problem hiding this comment.
Thanks for clarifying this. I remain skeptical of the value of having ProviderDevice CRDs when they're just reconciled by the same controller. Whatever information they contain could just as easily be added to the corresponding Device CRDs.
The real value of separate CRDs would be if we could let different controllers to implement the interface (as e.g. Metal3Machine does for Machine).
There was a problem hiding this comment.
At the beginning, we also considered putting the ProviderDevice information into the Device, because there may be multiple back-end implementations, so we think that adding ProviderDevice CRD can make the Device CRD look more concise and readable.
But we think it is acceptable to integrate them into one CRD.
There was a problem hiding this comment.
So, what's the resolution here? I see ProviderDevice still mentioned below, do we keep it or remove it?
There was a problem hiding this comment.
We think it would be better to keep the ProviderDevice, and we also explained the reasons above, but also need to see @zaneb 's views on this.
zhouhao3
force-pushed
the
network-configure
branch
2 times, most recently
from
fc88ebd to
9aa2ca2
Compare
Co-Authored-By: Andrea Fasano(andfasano) Co-Authored-By: Maël Kimmerlin(maelk) Co-Authored-By: Song Shukun <song.shukun@fujitsu.com> Co-Authored-By: Zou Yu <zouy.fnst@cn.fujitsu.com> Signed-off-by: Zhou Hao <zhouhao@cn.fujitsu.com>
|
@zaneb @dtantsur Hi, Because this PR has not been reviewed for a long time, I have no choice but to ask you to review it again. |
|
@zaneb Hi, This PR requires you to see if the current version address your concerns. We cannot move forward until you confirm it, so can you review it? Thanks! |
|
Hi, @zaneb PTAL. |
|
/hold |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
|
Stale issues close after 30d of inactivity. Reopen the issue with /close |
|
@metal3-io-bot: Closed this PR. DetailsIn response to this:
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. |
13 participants
This proposal is to expand the scope of Metal³ to include an API to manage physical network devices.
Google Docs address: https://docs.google.com/document/d/1QT3ojXlfy0J0TjbpmOR86mgeN-mZuEUa81v_R755Urs
Issue address: metal3-io/baremetal-operator#570
Demo address: https://github.com/Hellcatlk/network-operator
Co-Authored-By: Andrea Fasano(andfasano)
Co-Authored-By: Maël Kimmerlin(maelk)
Co-Authored-By: Song Shukun song.shukun@fujitsu.com
Co-Authored-By: Zou Yu zouy.fnst@cn.fujitsu.com
Signed-off-by: Zhou Hao zhouhao@cn.fujitsu.com