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.

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

Add LLDPProcessed information from ironic-inspector data #1167

Closed
wants to merge 1 commit into from
Closed

Add LLDPProcessed information from ironic-inspector data #1167

wants to merge 1 commit into from

Conversation

shibaPuppy
Copy link

@shibaPuppy shibaPuppy commented Sep 19, 2022 •
edited
Loading

Collect node LLDP information.

ex) if there is no LLDP information

apiVersion: metal3.io/v1alpha1
kind: BareMetalHost
metadata:
  name: node-3
  namespace: metal3
spec:
  automatedCleaningMode: metadata
  bmc:
    address: ipmi://192.168.111.1:6233
    credentialsName: node-3-bmc-secret
  bootMACAddress: 00:cc:0f:73:1b:fb
  bootMode: legacy
  online: true
status:
.....
    nics:
    - ip: 192.168.111.49
      lldp: {}
      mac: 00:cc:0f:73:1b:fd
      model: 0x1af4 0x0001
      name: enp2s0
    - ip: fe80::2cc:fff:fe73:1bfd%enp2s0
      lldp: {}
      mac: 00:cc:0f:73:1b:fd
      model: 0x1af4 0x0001
      name: enp2s0
    - ip: 172.22.0.83
      lldp: {}
      mac: 00:cc:0f:73:1b:fb
      model: 0x1af4 0x0001
      name: enp1s0
      pxe: true
    - ip: fe80::2cc:fff:fe73:1bfb%enp1s0
      lldp: {}
      mac: 00:cc:0f:73:1b:fb
      model: 0x1af4 0x0001
      name: enp1s0
      pxe: true
    ramMebibytes: 8192
....

@metal3-io-bot metal3-io-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 19, 2022
@metal3-io-bot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 19, 2022
dtantsur
dtantsur requested changes Sep 19, 2022
View reviewed changes
Copy link
Member

@dtantsur dtantsur left a comment

Choose a reason for hiding this comment

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

/ok-to-test

I've left some comments, but I think it's a significant enough API addition to justify a spec in metal3-docs first.

apis/metal3.io/v1alpha1/baremetalhost_types.go Outdated
@@ -599,6 +630,9 @@ type NIC struct {

// Whether the NIC is PXE Bootable
PXE bool `json:"pxe,omitempty"`

// Save ironic inspection data to lldp
LLDPProcessed LLDPProcessed `json:"lldp,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

The "Processes" part is essentially an internal detail, let's leave just LLDP.

apis/metal3.io/v1alpha1/baremetalhost_types.go Outdated
@@ -564,6 +564,37 @@ type Storage struct {
// +kubebuilder:validation:Maximum=4094
type VLANID int32

type LLDPProcessed struct {
// https://github.com/openstack/python-ironic-inspector-client/blob/master/ironic_inspector_client/resource.py#L24-L59
Copy link
Member

Choose a reason for hiding this comment

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

This becomes a comment from the Interface field, which is wrong. Ideally, we need a meaningful comment for each item, but definitely not a link to the code.

apis/metal3.io/v1alpha1/baremetalhost_types.go Outdated
Interface string `json:"interface,omitempty"`
Mac string `json:"mac,omitempty"`
NodeIdent string `json:"node_ident,omitempty"`
SwitchCapabilitiesEnabled []string `json:"switch_capabilities_enabled,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should create a Switch substructure just to make the API a bit more usable.

apis/metal3.io/v1alpha1/baremetalhost_types.go Outdated
SwitchCapabilitiesSupport []string `json:"switch_capabilities_support,omitempty"`
SwitchChassisId string `json:"switch_chassis_id,omitempty"`
SwitchMgmtAddresses []string `json:"switch_mgmt_addresses,omitempty"`
SwitchPortAutonegotiation_enabled string `json:"switch_port_autonegotiation_enabled,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Let's not mix cases.

apis/metal3.io/v1alpha1/baremetalhost_types.go Outdated
SwitchPortMTU int `json:"switch_port_mtu,omitempty"`
SwitchPortPhysicalCapabilities string `json:"switch_port_physical_capabilities,omitempty"`
SwitchPortProtocolVlanEnabled string `json:"switch_port_protocol_vlan_enabled,omitempty"`
SwitchPortProtocolVlanIds string `json:"switch_port_protocol_vlan_ids,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

It's weird to have VlanIds as string in one place and as []string (why not int?) in another. I don't remember why it was done initially, but I suggest we clean it up.

pkg/provisioner/ironic/hardwaredetails/hardwaredetails.go Outdated
@@ -24,6 +28,21 @@ func GetHardwareDetails(data *introspection.Data) *metal3v1alpha1.HardwareDetail
return details
}

func getLLDPProcessed(intf introspection.BaseInterfaceType) metal3v1alpha1.LLDPProcessed {
lldpjson, err := json.Marshal(intf.LLDPProcessed)
Copy link
Member

Choose a reason for hiding this comment

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

I hope there is a more efficient way to get the structure than to serialize it and deserialize back...

Copy link
Author

Choose a reason for hiding this comment

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

@dtantsur
i can't think of a good way.
do you have any code to suggest?

Copy link
Author

Choose a reason for hiding this comment

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

instead of dump the entiry data, how about saving only specific values ​​first?

@metal3-io-bot metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 19, 2022
@metal3-io-bot metal3-io-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 23, 2022
@shibaPuppy
Copy link
Author

/retest

@shibaPuppy shibaPuppy mentioned this pull request Oct 28, 2022
Merged
@furkatgofurov7
Copy link
Member

Please label the PR appropriately based on https://github.com/metal3-io/baremetal-operator/blob/main/CONTRIBUTING.md#contributing-a-patch to make it easier to sort it out while releasing in the changelog, thanks!

@dtantsur
Copy link
Member

On a second thought... maybe add a new CR for LLDP information and link to it? My worry is that this change will make hardware data absolutely unreadable in case when LLDP data is actually present.

@Rozzii Rozzii mentioned this pull request Mar 1, 2023
Closed
@metal3-io-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@metal3-io-bot metal3-io-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 21, 2023
@Rozzii
Copy link
Member

Rozzii commented Jun 7, 2023

/remove-lifecycle stale

@metal3-io-bot metal3-io-bot added needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 7, 2023
@metal3-io-bot
Copy link
Contributor

PR needs rebase.

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.

@metal3-io-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@metal3-io-bot metal3-io-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 20, 2023
@shibaPuppy
Copy link
Author

@Rozzii @dtantsur @furkatgofurov7
Doesn't anyone work on the lldp controller?

If no one does it, I'll try to start next month.

@Rozzii
Copy link
Member

Rozzii commented Oct 5, 2023

@Rozzii @dtantsur @furkatgofurov7 Doesn't anyone work on the lldp controller?

If no one does it, I'll try to start next month.

Hello,
I believe there is no one working on that as far as I know.
It would be a great help if you would work on that :D !!!

@metal3-io-bot
Copy link
Contributor

Stale issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle stale.

/close

@metal3-io-bot
Copy link
Contributor

@metal3-io-bot: Closed this PR.

In response to this:

Stale issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle stale.

/close

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.

@Rozzii
Copy link
Member

Rozzii commented Nov 6, 2023 •
edited
Loading

@shibaPuppy Should we let this PR get closed? I assume you are working on the new CR, so this is not relevant anymore.

@shibaPuppy
Copy link
Author

@Rozzii @dtantsur @furkatgofurov7
https://github.com/metal3-io/baremetal-operator/blob/main/apis/metal3.io/v1alpha1/baremetalhost_types.go#L669-L677

would it be better to create a CR in the form of "NetworkDiscoveryData" referenced in "HardwareDetail" and add it?
I'm curious about your opinion.

ex)

type HardwareDetails struct {
	SystemVendor HardwareSystemVendor `json:"systemVendor,omitempty"`
	Firmware     Firmware             `json:"firmware,omitempty"`
	RAMMebibytes int                  `json:"ramMebibytes,omitempty"`
	NIC          []NIC                `json:"nics,omitempty"`
	Storage      []Storage            `json:"storage,omitempty"`
	CPU          CPU                  `json:"cpu,omitempty"`
	Hostname     string               `json:"hostname,omitempty"`
	NetworkDiscoveryData *NetworkDiscoveryData `json: "networkdiscoverydata,omitempty"`
}

@Rozzii
Copy link
Member

Rozzii commented May 17, 2024

I see there is still interest in this so I will reopen.

@Rozzii Rozzii reopened this May 17, 2024
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kashifest for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@metal3-io-bot
Copy link
Contributor

@shibaPuppy: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
gofmt 7142603 link true /test gofmt
golint 7142603 link true /test golint
unit 7142603 link true /test unit
generate 7142603 link true /test generate
gomod 7142603 link true /test gomod
manifestlint 7142603 link true /test manifestlint

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/test-infra repository. I understand the commands that are listed here.

@Rozzii Rozzii added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 17, 2024
@Rozzii
Copy link
Member

Rozzii commented May 17, 2024

@Rozzii @dtantsur @furkatgofurov7 https://github.com/metal3-io/baremetal-operator/blob/main/apis/metal3.io/v1alpha1/baremetalhost_types.go#L669-L677

would it be better to create a CR in the form of "NetworkDiscoveryData" referenced in "HardwareDetail" and add it? I'm curious about your opinion.

ex)

type HardwareDetails struct {
	SystemVendor HardwareSystemVendor `json:"systemVendor,omitempty"`
	Firmware     Firmware             `json:"firmware,omitempty"`
	RAMMebibytes int                  `json:"ramMebibytes,omitempty"`
	NIC          []NIC                `json:"nics,omitempty"`
	Storage      []Storage            `json:"storage,omitempty"`
	CPU          CPU                  `json:"cpu,omitempty"`
	Hostname     string               `json:"hostname,omitempty"`
	NetworkDiscoveryData *NetworkDiscoveryData `json: "networkdiscoverydata,omitempty"`
}

This PR has passed is a bit out of date now let's organize this discussion a bit better.
Please create an Issue in BMO related to LLDP then let's continue the discussion there, also feel free to create a proposal about the "NetworkDiscoveryData" in the docs repo @shibaPuppy

@Rozzii Rozzii closed this May 17, 2024
@shibaPuppy shibaPuppy mentioned this pull request May 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtantsur dtantsur Awaiting requested review from dtantsur

Assignees
No one assigned
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shibaPuppy @metal3-io-bot @furkatgofurov7 @dtantsur @Rozzii