-
Notifications
You must be signed in to change notification settings - Fork 101
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
✨ Add bond hash policy #708
Conversation
Hi @shibaPuppy. 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. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
https://github.com/canonical/cloud-init/blob/main/cloudinit/sources/helpers/openstack.py#L679 seems to require bond_links actually? I'm not sure how the link you pasted proves it's optional.
@dtantsur links:
- id: bond0
type: bond
ethernet_mac_address: ff:ff:ff:ff:ff:ff
bond_links: [] # here empty array
- id: bond1
type: bond
ethernet_mac_address: 11:22:33:44:55:66
bond_links:
- eth0
- eth1
.... |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
1b2a764
to
02704ad
Compare
@dtantsur |
417dff1
to
cff3208
Compare
// +kubebuilder:validation:Enum="layer2";"layer3+4";"layer2+3" | ||
// Selects the transmit hash policy used for port selection in balance-xor and 802.3ad modes | ||
// +optional | ||
BondHashPolicy string `json:"bond_xmit_hash_policy"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSON name does not conform to the used standard and does not fully match the field name. Why not just bondHashPolicy
?
// +kubebuilder:validation:Enum="layer2";"layer3+4";"layer2+3" | ||
// Selects the transmit hash policy used for port selection in balance-xor and 802.3ad modes | ||
// +optional | ||
BondHashPolicy string `json:"bond_xmit_hash_policy"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
baremetal/metal3data_manager.go
Outdated
"mtu": link.MTU, | ||
"ethernet_mac_address": macAddress, | ||
"bond_mode": link.BondMode, | ||
"bond_xmit_hash_policy": link.BondHashPolicy, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot find this field in the network data schema. Where is it coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dtantsur
It does not exist in the network-data schema, but I confirmed that the code exists in the ironic sample & cloud-init.
7f57db9
to
71f9a5b
Compare
https://github.com/openstack/ironic/blob/master/ironic/tests/unit/drivers/modules/network/json_samples/network_data.json#L31 Adds the bond hash policy feature added to Ironic.
/test-centos-e2e-integration-main |
/test-centos-e2e-integration-main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Looks good as far as I can tell.
/test-ubuntu-e2e-features-main |
@kashifest seems like the test haven't been triggered |
/test-ubuntu-e2e-feature-main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/retitle ✨ Add bond hash policy |
/test-e2e-upgrade-main-from-release-1-5 |
/test-ubuntu-e2e-feature-main |
/test-e2e-upgrade-main-from-release-1-5 |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kashifest, Rozzii 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 |
https://github.com/openstack/ironic/blob/master/ironic/tests/unit/drivers/modules/network/json_samples/network_data.json#L31
Adds the bond hash policy feature added to Ironic.
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #