feat: add fake power actions #47
Conversation
synackd
left a comment
synackd
left a comment
There was a problem hiding this comment.
I think this looks good, just need to update some comments. I also still need to test this.
pkg/client/smd/smd.go
Outdated
| // This struct is a partially duplicated copy of the schemas InventoryDetail type, originally from SMD: | ||
| // https://github.com/OpenCHAMI/schemas/blob/9aad17a286c405e1d75463298ec8db553cc4ca12/schemas/inventory.go#L61-L88 | ||
| // smd.parseRedfishEndpointDataV2 unmarshals the Systems field of a RedfishEndpoint request into one: | ||
| // https://github.com/OpenCHAMI/smd/blob/f07680ffb6c41e75945bc32bc7ba948a56afe2e5/cmd/smd/smd-api.go#L2826-L2842 | ||
| // SMD will have zero values for any fields this currently lacks, and will have unexpected behavior when it tries to | ||
| // use them later. | ||
|
|
||
| // System represents data that would be retrieved from BMC System data, except | ||
| // reduced to a minimum needed for discovery. |
There was a problem hiding this comment.
The merge of OpenCHAMI/smd#74 and #48 will require the first part of this comment to be revised. The definition will be here (this is a branch so the URI will need to be changed when merged).
Also, this comment block should be connected to the struct comment below it to indicate that the comment describes the struct. While we're at it, could we limit the line width to 80 characters to match the other comment lines? E.g.
// System represents data that would be retrieved from BMC System data, except
// reduced to a minimum needed for discovery. It is a partially duplicated copy
// of ...There was a problem hiding this comment.
We're just gonna toss these local definitions then? #48 doesn't do that yet, but it looks like the end expectation is we just import the SMD types, and tossing this entirely avoids git confusion.
There was a problem hiding this comment.
I think that is the end goal. Looks like #48 needs to be rebased, so we don't have to revise the comment in this PR and instead revise it in that one (which probably makes sense scope-wise). My main concern with my review comment here is the second part of it, which is to merge this comment to the struct comment. With that change, I think we can resolve this.
There was a problem hiding this comment.
On a second read I think you maybe just meant the package path.
To clarify, will we still have a separate ochami/pkg/client/smd.System shadow type, or will ochami CLI code use smd/v2/pkg/schemas.InventoryDetail directly? If it's the latter we don't need this comment at all.
If it's the former we should still have it, and ideally use a tag instead of a commit. Do you know how soon we plan to have a release with OpenCHAMI/smd#74 ?
I'd intentionally kept it separate to not include it in the docstring, since it is, for lack of better description, messy--it's halfway to being a TODO to stop using the shadow type if we keep having to duplicate fields into it.
There was a problem hiding this comment.
On a second read I think you maybe just meant the package path.
Yep.
To clarify, will we still have a separate
ochami/pkg/client/smd.Systemshadow type, or will ochami CLI code usesmd/v2/pkg/schemas.InventoryDetaildirectly? If it's the latter we don't need this comment at all.If it's the former we should still have it, and ideally use a tag instead of a commit.
The goal is to move entirely to a package within SMD and move away from the schemas repo completely. That way, SMD owns its own schema and no shadow types are needed. The shadow types were created because a while back it looked like we might transition everything to a schemas repo and the current schemas repo lacked a lot of things the ochami CLI needed, but the paradigm has since shifted.
Do you know how soon we plan to have a release with OpenCHAMI/smd#74 ?
I don't really have an accurate gauge. I think we should merge this soon, so I'm inclined to agree that we can keep a TODO comment around for switching over to the SMD package once it gets merged, at which point in time we can get rid of the SMD shadow types.
I'd intentionally kept it separate to not include it in the docstring, since it is, for lack of better description, messy--it's halfway to being a TODO to stop using the shadow type if we keep having to duplicate fields into it.
That's fair. Since a transition to an SMD client package is underway, I don't think this should be a blocker. If you want to add the TODO and then comment here (so I'm notified) and resolve this, we can move forward with this PR.
rainest
force-pushed
the
rainest/power-action
branch
2 times, most recently
from
87882ce to
c495ae7
Compare
|
Do you have instructions I can test this with? Are you using a PCS quadlet or similar deployment? |
The simple option is we simply ignore the E2E testing here, it's just aligning IDK how we want to handle that more broadly for this repo--I wouldn't want it to be responsible for E2E tests anyway, but it could maybe do single-service tests for "the actual service API accepts this request". We'd need to build out more SMD mocks for things that aren't SMD though. The E2E answer is sorta yes, in that I have something that works, but that it's still in not release-ready form, and this is one of the chicken-egg problem pieces to getting it there. I can invite you to my sandbox repo. The PCS image currently used by that has hacks to deal with #51 and to avoid using Vault. |
I should clarify: I'm not currently running PCS on my test systems and so haven't interacted with it much (we should coordinate on adding it to the release repo at some point). I more wanted some guidance on how to run PCS and run the ochami CLI to test these changes. Are you running the quadlet deployment (e.g. from the release repo) somewhere or a docker-compose deployment from the deployment-recipes repo?
We do need to figure out how to add integration tests at some point. I considered doing SMD mocking, and maybe adding an SMD client package/schema would prevent us from having to ensure the mocks keep up with the SMD API. However, it might be worth a wider discussion on how to go about this org-wide.
Sure, we can communicate offline about this. |
Add the System.Actions field and populate it with all possible actions during fake discovery. PCS rejects transitions if SMD doesn't recognize the action for a given system, so this provides a permissive default. Signed-off-by: Travis Raines <571832+rainest@users.noreply.github.com>
rainest
force-pushed
the
rainest/power-action
branch
from
c495ae7 to
980850b
Compare
|
Force-pushed and squashed the review fixes to get past a phantom conflict (I do love when git says there's a conflict for lines that no longer exist in the changeset and forces you to fix it twice before squashing makes it irrelevant 😐 ) |
|
In my environment, I haven't been able to test performing transitions in PCS after the nodes are discovered, but I can verify that they are being sent: @rainest I don't want complications in my environment to be a blocker. Have you tested this to work on your end? |
|
It did indeed actually reboot the machine after in mine. In the strictest sense the supported list check happens only at transition creation time, and actually sending them happens after. That's kinda pedantic, but you should be able to check that PCS doesn't accept any transition creates without this, and that it does accept them after (even if the BMC's not reachable). |
synackd
left a comment
synackd
left a comment
There was a problem hiding this comment.
I see that the transitions get added to SMD:
hmsds=> SELECT * FROM comp_endpoints where id='x3000c0s0b0n0';
-[ RECORD 1 ]---+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
id | x3000c0s0b0n0
type | Node
domain |
redfish_type | ComputerSystem
redfish_subtype |
rf_endpoint_id | x3000c0s0b0
mac |
uuid | e3da88d9-993b-4e51-bd97-c278e9f33ef6
odata_id | /redfish/v1/Systems/x3000c0s0b0n0
component_info | {"Name":"re01","Actions":{"#ComputerSystem.Reset":{"ResetType@Redfish.AllowableValues":["On","ForceOff","GracefulShutdown","GracefulRestart","ForceRestart","Nmi","ForceOn","PushPowerButton","PowerCycle","Suspend","Pause","Resume"],"@Redfish.ActionInfo":"/redfish/v1/Systems/x3000c0s0b0n0/ResetActionInfo","target":"/redfish/v1/Systems/x3000c0s0b0n0/Actions/ComputerSystem.Reset"}},"EthernetNICInfo":[{"RedfishId":"","@odata.id":"","Description":"Interface 0 for re01","InterfaceEnabled":true,"MACAddress":"5c:ed:8c:21:ac:08"}]}
and @rainest has confirmed this has worked in his setup. This LGTM and rather than delay this any further waiting for my own PCS setup to be complete, I stamp this with my approval.
Add the System.Actions field and populate it with all possible actions during fake discovery. This allows PCS to attempt those actions.
Fake discovery does not directly expose the System struct, so there wasn't an existing place to slot this into config. We'll probably need to revisit that eventually: there are many other fields in SMD/schemas types that aren't yet in the fake discovery type, and I expect there are more unexpected behaviors when they have zero values.
Providing a permissive default on the SMD side is probably good enough for fake discovery, since you can just ignore (actually) unsupported actions by not requesting PCS perform them.
Downstream, this looks like: