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

MTL-2230 add get bios and set bios options via redfish #18

Merged
merged 5 commits into from
Nov 21, 2023
Merged

Conversation

jacobsalmela
Copy link
Contributor

@jacobsalmela jacobsalmela commented Oct 9, 2023

Summary and Scope

  • adds get bios and set bios commands with several flags
  • gets current and pending bios attributes
  • sets arbitrary attributes from cli args, yaml file, or pre-defined shortcuts

Issue Type

RFE Pull Request

  • Adds get/set functionality for the bios subcommand
  • allows passing of variadic args to async/MapPrint
  • converts MapPrint to use a type-assertion switch case instead of using reflect. this keeps compatibility with everything being able to use the function but may still need adjustments or modifications going forward
  • adds cli/types.go to prevent import cycles (for MapPrint)
  • condensed the code to get the *redfish.Bios object into its own function
  • updates makeAttributes to convert strings to ints to work properly with Intel
  • adds unmarshalBiosKeyValFile() to get k/v pairs from a yaml file
  • adds virtSettings() and exported variables, which pre-define the attributes needed to enable virtualization on a per-vendor basis, which is tied to the --virt shortcut flag
  • adds getPendingAttributes(), which returns a map of pending keys (these are "staged" in a different key than the current settings)
  • adds updated tests for the new commands (setting values fails at present due to a limitation in the simulator. see: https://github.com/Cray-HPE/csm-redfish-interface-emulator/tree/master#dynamic-resources
  • adds more hardware to the simulator and updates the config fixture as appropriate
  • requires 1 minimum arg for these commands (a host)
  • adds a rome decoder ring that converts cryptic Rome0565 entries to their friendly names
  • adds a be_json matcher for shellspec that checks for valid json via jq
  • updates cobra library to an unreleased version, to gain access to a new function that checks that at least one flag is set (needed for some of these changes)

Risks and Mitigations

Low to medium. This changes the print function to work with more output/data types, thus requiring modifications to everything currently using it. It also cannot be fully tested in the pipeline yet due to the simulator limitation mentioned above

@jacobsalmela jacobsalmela self-assigned this Oct 9, 2023
@jacobsalmela jacobsalmela changed the title Handy night add get bios and set bios options via redfish Oct 12, 2023
@jacobsalmela jacobsalmela force-pushed the handy-night branch 5 times, most recently from bec5dfe to 37d5e71 Compare October 13, 2023 18:59
pkg/cmd/cli/bios/virt.go Outdated Show resolved Hide resolved
pkg/cmd/cli/bios/virt.go Show resolved Hide resolved
pkg/cmd/cli/bios/bios.go Outdated Show resolved Hide resolved
@jacobsalmela jacobsalmela force-pushed the handy-night branch 3 times, most recently from 63a907d to f072e68 Compare October 17, 2023 10:45
@jacobsalmela jacobsalmela force-pushed the handy-night branch 4 times, most recently from 41301d8 to eb77326 Compare October 26, 2023 12:25
@jacobsalmela jacobsalmela marked this pull request as ready for review October 26, 2023 13:43
@jacobsalmela jacobsalmela requested a review from a team as a code owner October 26, 2023 13:43
@rustydb rustydb changed the title add get bios and set bios options via redfish MTL-2230 add get bios and set bios options via redfish Oct 26, 2023
@rustydb
Copy link
Contributor

rustydb commented Oct 26, 2023

Looking at the JSON files, I have one comment.

"Rome" is a codename for a product line in the AMD EPYC CPUs, I think we should sequester them into a lower subdirectory.

├── pkg
│   ├── auth
│   │   └── auth.go
│   ├── cmd
│   │   ├── cli
│   │   │   ├── bios
│   │   │   │   ├── bios.go
│   │   │   │   ├── get.go
│   │   │   │   ├── init.go
│   │   │   │   ├── models
│   │   │   │   |   └── amd
│   │   │   │   |       └── epyc
|   │   │   │   │           └── rome

My thoughts are:

  • "rome" has no context
  • EPYC's other product lines may have different decoder rings

@jacobsalmela
Copy link
Contributor Author

That is easy enough to implement. I will address that shorty

@rustydb
Copy link
Contributor

rustydb commented Nov 3, 2023

Maybe I missed it, but can we add the scripts for generating the Gigabyte codename files to the repo somewhere?
Then maintainers can use them to fix up the local files.

@rustydb
Copy link
Contributor

rustydb commented Nov 3, 2023

Noticed this too after the rebase.

redbull-management:~ # gru --version
gru version 0.0.4~17~g245c360
redbull-management:~ # conman -q | gru chassis boot http --persist
Asynchronously updating [    9] hosts ...
ncn-h002-mgmt:
	UNKNOWN TYPE: boot.Override
ncn-h003-mgmt:
	UNKNOWN TYPE: boot.Override
ncn-h004-mgmt:
	UNKNOWN TYPE: boot.Override
ncn-h005-mgmt:
	UNKNOWN TYPE: boot.Override
ncn-h006-mgmt:
	UNKNOWN TYPE: boot.Override
ncn-s001-mgmt:
	UNKNOWN TYPE: boot.Override
ncn-s002-mgmt:
	UNKNOWN TYPE: boot.Override
ncn-s003-mgmt:
	UNKNOWN TYPE: boot.Override
ncn-s004-mgmt:
	UNKNOWN TYPE: boot.Override

It works fine for the current HEAD (gofish0.14.0).

redbull-management:~ # gru --version
gru version 0.0.4~9~gf830d91
redbull-management:~ # conman -q | gru chassis boot http --persist
Asynchronously updating [    9] hosts ...
ncn-h002-mgmt:
	Target              : UefiHttp
ncn-h003-mgmt:
	Target              : UefiHttp
ncn-h004-mgmt:
	Target              : UefiHttp
ncn-h005-mgmt:
	Target              : UefiHttp
ncn-h006-mgmt:
	Target              : UefiHttp
ncn-s001-mgmt:
	Target              : UefiHttp
ncn-s002-mgmt:
	Target              : UefiHttp
ncn-s003-mgmt:
	Target              : UefiHttp
ncn-s004-mgmt:
	Target              : UefiHttp

@rustydb
Copy link
Contributor

rustydb commented Nov 8, 2023

Piping still doesn't seem to work for set|get bios

redbull-management:~ # /tmp/gru --version
gru version 0.0.4~18~ge8302b7
redbull-management:~ # conman -q | /tmp/gru --json set bios --attributes "OnboardNICEnable_0=0,OnboardNICEnable_1=0,ProcessorHyperThreadingDisable=0,ProcessorVmxEnable=1,ProcessorX2apic=1,VTdSupport=1,ProcessorPowerPolicy=0"
Error: requires at least 1 arg(s), only received 0
Usage:
  gru set bios [flags]

Flags:
      --attributes strings   Comma delimited list of attributes and values to set them to: attribute=value[,keyN=valueN].
      --defaults             Reset all BIOS attributes to vendor defaults
      --from-file string     Path to a key/value YML file with bios attributes and their desired value(s)
  -h, --help                 help for bios
      --virt                 Enable virtualization using pre-determined, per-vendor settings

Global Flags:
  -c, --config string   Configuration file containing BMC credentials, necessary if USERNAME and PASSWORD are not defined in the environment gru
                         (default "./gru.yml")
      --insecure        Ignore untrusted or insecure certificates
  -j, --json            Output results in JSON

An error occurred: requires at least 1 arg(s), only received 0
redbull-management:~ # conman -q | /tmp/gru chassis power status
Asynchronously querying [    9] hosts ...
ncn-h002-mgmt:
	PowerState          : Off
ncn-h003-mgmt:
	PowerState          : On
ncn-h004-mgmt:
	PowerState          : On
ncn-h005-mgmt:
	PowerState          : On
ncn-h006-mgmt:
	PowerState          : On
ncn-s001-mgmt:
	PowerState          : Off
ncn-s002-mgmt:
	PowerState          : Off
ncn-s003-mgmt:
	PowerState          : Off
ncn-s004-mgmt:
	PowerState          : Off

The Pending results do report the correct, pending attribute. However they report the current value instead of the pending value.

redbull-management:~ # /tmp/gru --version
gru version 0.0.4~18~ge8302b7
redbull-management:~ # for ncn in $(conman -q); do /tmp/gru --json set bios --attributes "OnboardNICEnable_0=0,OnboardNICEnable_1=0,ProcessorHyperThreadingDisable=0,ProcessorVmxEnable=1,ProcessorX2apic=1,VTdSupport=1,ProcessorPowerPolicy=0" $ncn;done
{
  "ncn-h002-mgmt": {
    "Pending": {
      "Attributes": {
        "ProcessorVmxEnable": 0,
        "ProcessorX2apic": 0,
        "VTdSupport": 0
      }
    }
  }
}
{
  "ncn-h003-mgmt": {
    "Pending": {
      "Attributes": {
        "ProcessorPowerPolicy": 0,
        "ProcessorVmxEnable": 0,
        "ProcessorX2apic": 0
      }
    }
  }
}
{
  "ncn-h004-mgmt": {
    "Pending": {
      "Attributes": {
        "ProcessorPowerPolicy": 0,
        "ProcessorVmxEnable": 0,
        "ProcessorX2apic": 0
      }
    }
  }
}
{
  "ncn-h005-mgmt": {
    "Pending": {
      "Attributes": {
        "ProcessorPowerPolicy": 0,
        "ProcessorVmxEnable": 0,
        "ProcessorX2apic": 0
      }
    }
  }
}
{
  "ncn-h006-mgmt": {
    "Pending": {
      "Attributes": {
        "ProcessorPowerPolicy": 0,
        "ProcessorVmxEnable": 0,
        "ProcessorX2apic": 0,
        "VTdSupport": 0
      }
    }
  }
}
{
  "ncn-s001-mgmt": {
    "Pending": {
      "Attributes": {
        "ProcessorPowerPolicy": 0,
        "ProcessorVmxEnable": 0,
        "ProcessorX2apic": 0,
        "VTdSupport": 0
      }
    }
  }
}
{
  "ncn-s002-mgmt": {
    "Pending": {
      "Attributes": {
        "ProcessorPowerPolicy": 0,
        "ProcessorVmxEnable": 0
      }
    }
  }
}
{
  "ncn-s003-mgmt": {
    "Pending": {
      "Attributes": {
        "ProcessorPowerPolicy": 0,
        "ProcessorVmxEnable": 0
      }
    }
  }
}
{
  "ncn-s004-mgmt": {
    "Pending": {
      "Attributes": {
        "ProcessorPowerPolicy": 0,
        "ProcessorVmxEnable": 0,
        "ProcessorX2apic": 0,
        "VTdSupport": 0
      }
    }
  }
}

Jacob, I have some changes queued for this PR. To avoid conflicts I can take a stab at fixing these two issues. I made some UX changes to the GRU confluence page that flip get bios and set bios to bios get and bios set.

@rustydb
Copy link
Contributor

rustydb commented Nov 8, 2023

The piping is broken due to the nargs requirement on the new options, I'll remove that.

@jacobsalmela
Copy link
Contributor Author

Oh forgot to check the piping thing... if you got it, go ahead. Otherwise I can look

@jacobsalmela jacobsalmela added the bug Something isn't working label Nov 15, 2023
@jacobsalmela
Copy link
Contributor Author

Updated to fix piping in from STDIN. Also added a test for it.

@jacobsalmela jacobsalmela removed the bug Something isn't working label Nov 15, 2023
@rustydb
Copy link
Contributor

rustydb commented Nov 15, 2023

Thank you.

I'll rebase my changes.

@rustydb
Copy link
Contributor

rustydb commented Nov 21, 2023

I'll merge this, and make a new branch for my changes. I tracked what my changes were addressing via #27 #28 and #29.

Signed-off-by: Jacob Salmela <jacob.salmela@hpe.com>
Signed-off-by: Jacob Salmela <jacob.salmela@hpe.com>
Signed-off-by: Jacob Salmela <jacob.salmela@hpe.com>
Signed-off-by: Jacob Salmela <jacob.salmela@hpe.com>
Signed-off-by: Jacob Salmela <jacob.salmela@hpe.com>
Copy link
Contributor

@rustydb rustydb left a comment

Choose a reason for hiding this comment

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

Approving and merging. Addressing some of my own concerns in a follow-up PR, since the majority/heavy-lifting work is accomplished with what's here already.

@rustydb rustydb merged commit 9c7f12c into main Nov 21, 2023
3 checks passed
@rustydb rustydb deleted the handy-night branch November 21, 2023 02:45
@jacobsalmela
Copy link
Contributor Author

I did make some compromises to conform to the existing skeleton code. There are some other approaches to some of the things you mentioned but at the very least, the functionality is there and can be refined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants