Design for BMC eventing API #167

stbenjam · 2021-03-09T11:55:45Z

This design document contains an initial draft of an API for configuring
BMC event subscriptions, using a new Ironic API.

stbenjam · 2021-03-09T12:57:24Z

design/baremetal-operator/bmc-events.md

+- The BMCEventSubscription must have a way to inject headers, to allow for tokens,
+  basic auth, etc.
+
+#### Open Questions


We need to decide when to register the notifications, the Ironic design allows them at any time, but I think we probably want to only configure them when a host is active? And remove them when it's not?

I don't think we should tie the event subscription to the provisioning state. The goal downstream is to support monitoring hardware events. It doesn't matter whether the hosts being monitored are in use yet or not.

Does relying on Ironic to proxy the subscription calls mean that we need to wait for a host to be successfully registered? So the ironic node is in the "manageable" state? Can anything go wrong in that process that would affect provisioning but not affect the event subscription? Or does Ironic basically check the credentials and that's it?

Subscription it's an entity that could have its own state (active/not active), and I also agree should be independent by the current host provisioning state.

I don't think we should tie the event subscription to the provisioning state. The goal downstream is to support monitoring hardware events. It doesn't matter whether the hosts being monitored are in use yet or not.

Does relying on Ironic to proxy the subscription calls mean that we need to wait for a host to be successfully registered? So the ironic node is in the "manageable" state? Can anything go wrong in that process that would affect provisioning but not affect the event subscription? Or does Ironic basically check the credentials and that's it?

I believe it'd have to be in manageable or later, but not enroll or an error state. Is the right behavior to keep trying to create the event in Ironic until it succeeds once the BareMetalHost is registered?

If we know we have to not be in the enroll state, we could handle that cleanly with a retry w/o error. Other states should keep retrying and report an error if one is produced. Maybe we can reuse the backoff logic that @andfasano added so we don't thrash ironic with requests.

dhellmann · 2021-03-09T13:33:53Z

design/baremetal-operator/bmc-events.md

+- The BMCEventSubscription must have a way to inject headers, to allow for tokens,
+  basic auth, etc.
+
+#### Open Questions


I don't think we should tie the event subscription to the provisioning state. The goal downstream is to support monitoring hardware events. It doesn't matter whether the hosts being monitored are in use yet or not.

Does relying on Ironic to proxy the subscription calls mean that we need to wait for a host to be successfully registered? So the ironic node is in the "manageable" state? Can anything go wrong in that process that would affect provisioning but not affect the event subscription? Or does Ironic basically check the credentials and that's it?

design/baremetal-operator/bmc-events.md

andfasano · 2021-03-10T11:01:57Z

design/baremetal-operator/bmc-events.md

+   headers:
+     X-Event-Source: "metal3.io"
+   headerRef: webhookBridgeAuth
+   context: “SomeUserContext”


What about adding an enabled field to turn on/off the subscription?

The API is fairly simple, is there a reason to not just delete the CR if one isn't interested in an event anymore?

I was thinking to something more like a "pause", but I'd guess that CR deletion could be good enough to start

andfasano · 2021-03-10T11:03:08Z

design/baremetal-operator/bmc-events.md

+metadata:
+  name: worker-1-events
+spec:
+   hostRef: ostest-worker-1


I guess here there's the implicit assumption that the event resource will be in the same namespace of the referenced host?

I was wondering about namespaces now that we can watch multiple in some cases. We probably wouldn't want someone setting up an event subscription to a host in another namespace.

dhellmann · 2021-03-10T20:36:31Z

design/baremetal-operator/bmc-events.md

+spec:
+   hostRef: ostest-worker-1
+   targetURI: https://events.apps.corp.example.com/webhook
+   filters:


Are the filter values common across vendors? Are they part of the Redfish standard or defined by Ironic or just free-form?

I was looking at the values reported by Redfish and the ones reported by the HW vendor, what the HW vendor reports is a subset of what Redfish would accept.

http://redfish.dmtf.org/schemas/v1/Event.json#/definitions/EventType

EventType is deprecated as of Redfish v1.5 and replaced by RegistryPrefix and ResourceType so we can't assume filters are values of the same type.

I expect the filters to take a generic JSON string like this:
'{EventType: [...], RegistryPrefix:[...], ResourceType:[...], EventFormatType: "x", OriginResources: [...]'

originResources: [...] can be a separate field outside of the filters.

Do you have a link to the schema? This is more relevant to the Ironic design than here, we're an abstraction that sits on top so this is designed around their original proposal.

cc: @iurygregory @honza

http://redfish.dmtf.org/schemas/DSP2046_2021.1.html#eventtype

@jzding I've checked a Dell R640 with FW 4.40.10.00 using RedfishVersion:1.9.0, the information from EventService (using v1.5.0 doesn't provide the RegistryPrefix/RegistryPrefixes so it's not supported) http://paste.openstack.org/show/806812/

filters won't be necessary, since HW vendors may not have the support for RegistryPrefix/RegistryPrefixes we are not adding the EventTypes to ironic API, by default we will be using Alert option till HW vendors have the support for it.

For more details see the discussion: https://meetings.opendev.org/irclogs/%23openstack-ironic/%23openstack-ironic.2021-06-25.log.html#t2021-06-25T13:41:57

I removed all references to filters.

dhellmann · 2021-03-10T20:44:42Z

design/baremetal-operator/bmc-events.md

+   context: “SomeUserContext”
+status:
+  errorMessage: ""
+  errorCount: 0


I know we've stayed away from Condition fields in the past because the advice was they were deprecated. I don't see any evidence of that in https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties though, and I do see a comment about "phases" being deprecated so I wonder if we got the wrong message at the time.

Would it make sense to use a Condition here to indicate whether the hook is Registered, to be more consistent with other k8s APIs? That doesn't give us the progressive back-off behavior, I guess.

@honza this comment seems to be outstanding, could you take look?

The current status implementation is slightly different

dhellmann · 2021-03-10T20:46:36Z

design/baremetal-operator/bmc-events.md

+
+### Upgrade / Downgrade Strategy
+
+Not required, this is a new API being introduced


During an upgrade with this feature in use we will lose the Ironic database. How will we address that? We don't want to have duplicate registrations of web hooks on BMCs, for example.

Adding a field in the "Status" specifying if the event registration at the BMC was successful might solve this issue?

The real state is outside of the cluster (in the BMC), though, so if something else modifies the BMC configuration the status field would no longer reflect the real configuration.

That's a good point, @dtantsur @iurygregory How does the Ironic side handle this if we adopt a host? Will you be able to import the subscriptions into Ironic so we could check the API for existing subscriptions?

So, if I do remember in the ironic API we will offer create/delete/list subscriptions, I will try to check if the BMC's would accept duplicate registrations, Ironic can also check if the node has a subscription that matches the one you are trying to add.

In my experimentation I found that the BMC would not report all of the details of subscriptions that were registered. I presume that was to protect values that might need to be kept private/secret. I had to encode a signature of the subscription in the context field so I could find the match again and avoid re-registering it. See https://github.com/dhellmann/redfish-event-controller/blob/main/controllers/redfish/eventsubscription_controller.go#L148 for details

jzding · 2021-05-14T16:35:23Z

design/baremetal-operator/bmc-events.md

+- A BMCEventSubscription resource represents a subscription to the events generated
+  by a specific BMC.
+- Ironic will manage configuring the subscription, using a new API for managing them.
+- The BMCEventSubscription with maintain a reference to a BareMetalHost.


typo with -> will

dhellmann · 2021-07-15T13:56:51Z

/uncc

honza · 2021-07-16T09:50:45Z

Let's have another look at this.

Recent updates:

Fix typos
Make it explicit that subscriptions and baremetal hosts will share a namespace
Remove all mentions of filters

I like Doug's idea for using subscription signatures for the scenario when the Ironic database goes away. I wonder if using the context field makes sense though. Are we using it for two different purposes? Do we have to come up with some concrete encoding mechanism?

Are we happy with the decision about provisioning states? The latest seems to be that we keep retrying until we succeed; perhaps with a jittery back-off to avoid overwhelming the system.

iurygregory · 2021-08-03T08:06:42Z

design/baremetal-operator/bmc-events.md

+
+### Dependencies
+
+[Ironic Eventing API](https://storyboard.openstack.org/#!/story/2008366)


s/Ironic Eventing API/ Ironic Vendor Passthru for Subscriptions
the link is https://storyboard.openstack.org/#!/story/2009061

iurygregory · 2021-08-03T08:07:27Z

design/baremetal-operator/bmc-events.md

+
+## References
+
+- [Ironic Eventing API](https://storyboard.openstack.org/#!/story/2008366)


We also need to update this to https://storyboard.openstack.org/#!/story/2009061

iurygregory · 2021-08-03T08:12:18Z

design/baremetal-operator/bmc-events.md

+status:
+  errorMessage: ""
+  errorCount: 0
+  subscriptionID: aa618a32-9335-42bc-a04b-20ddeed13ade


This would be the ID that the BMC will give when we create a sbuscription? If yes, this is not required to be an UUID (Dell uses UUID, HP uses numbers)

I think this is an arbitrary string. Is that sufficient?

@honza oh ok! I was wondering if could have some sort of validation to make sure it was an UUID, if is just a string it's ok. Thanks!

iurygregory · 2021-08-03T08:18:51Z

design/baremetal-operator/bmc-events.md

+spec:
+   hostRef: ostest-worker-1
+   targetURI: https://events.apps.corp.example.com/webhook
+   headerRef: webhookBridgeAuth


Atm the vendor passthru will only accept Destination (required) and Context (default to empty string), EventTypes (default to ["Alert"]), Protocol (by default is redfish)

payload = { 'Destination': kwargs.get('Destination'), 'Protocol': kwargs.get('Protocol', "Redfish"), 'Context': kwargs.get('Context', ""), 'EventTypes': kwargs.get('EventTypes', ["Alert"]) }

This is how the payload looks like in Ironic

iurygregory · 2021-08-04T09:17:41Z

design/baremetal-operator/bmc-events.md

+- The BMCEventSubscription will maintain a reference to a BareMetalHost.
+- The BMCEventSubscription will reside in the same namespace as the referenced
+  BareMetalHost.
+- The BMCEventSubscription will maintain a reference to the Ironic


This won't be true, since Ironic won't have any information about the subscription in each node. I think we should probably maintain a reference to the node UUID + subscription ID (this will be different based on the BMC)

Done; clarified the source of the subscription ID. We can use the host ref to look up the node UUID.

iurygregory · 2021-08-04T09:27:47Z

design/baremetal-operator/bmc-events.md

+  BareMetalHost.
+- The BMCEventSubscription will maintain a reference to the Ironic
+  subscription ID.
+- The BMCEventSubscription will allow injection of headers using a


Let's remove this since we don't have the support for headers, I'm wondering if this is a requirement for our downstream case.

iurygregory · 2021-08-04T09:30:38Z

design/baremetal-operator/bmc-events.md

+
+- A BMCEventSubscription resource represents a subscription to the events generated
+  by a specific BMC.
+- Ironic will manage configuring the subscription using a new API for managing them.


Maybe we should say Ironic will be using vendor passthru methods instead of a new API.

dtantsur · 2021-08-18T14:07:19Z

design/baremetal-operator/bmc-events.md

+URI. This design document describes a Metal3 API for configuring a
+subscription. While Redfish is the primary target for this design, the
+Ironic API is vendor-neutral and seeks to provide a unified interface
+for configuring events.


FYI the unified interface was delayed, we've concentrated on redfish only for now.

dtantsur · 2021-08-18T14:08:38Z

design/baremetal-operator/bmc-events.md

+
+### Dependencies
+
+[Ironic Vendor Passthru for Subscriptions](https://storyboard.openstack.org/#!/story/2009061)


Already done, but needs gophercloud support.

Can be removed now

++ we can remove this, we don't have any Dependencies anymore

iurygregory · 2021-09-14T07:59:10Z

design/baremetal-operator/bmc-events.md

+URI. This design document describes a Metal3 API for configuring a
+subscription. While Redfish is the primary target for this design, the
+Ironic API is vendor-neutral and seeks to provide a unified interface
+for configuring events.


iurygregory · 2021-09-14T08:01:33Z

design/baremetal-operator/bmc-events.md

+   context: “SomeUserContext”
+   eventTypes:
+   - Alert
+   protocol: Redfish


We need to add a field for http_headers, it's a list of headers that ironic will send in the request if they are set.

example: [{"key1":"value1"}, {"key2":"value2"}] or [{"key1":"value1", "key2":"value2"}]

@honza I've pushed the gophercloud PR to add the support for HttpHeaders gophercloud/gophercloud#2224 if you have some time this week can you update this proposal?

Updated, thanks!

And also this one

dtantsur · 2021-10-15T13:13:06Z

design/baremetal-operator/bmc-events.md

+  obtained from the BMC.
+- The baremetal-operator binary will be expanded to include 2
+  reconcilers with dedicated controller/reconcile loops for
+  BareMetalHost and BMCEventSubscriptions.


What happens if the driver does not support the vendor passthru? E.g. something not redfish-based is used?

@dtantsur in this case Ironic would return 400

@honza are we adding something that will prevent creating a subscription in case they are not using redfish in the BMO?

Not at the moment; we can easily add this

I think we should at least somehow indicate the failure instead of retrying infinitely

This patch implements the fast eventing API: metal3-io/metal3-docs#167 We introduce a new CRD, and a new reconciler. We use the Ironic API to communicate with the BMC to manage node subscriptions.

sadasu · 2021-11-18T13:59:43Z

/retitle Design for BMC eventing API

iurygregory · 2021-11-19T13:17:29Z

/test markdownlint

This patch implements the fast eventing API: metal3-io/metal3-docs#167 We introduce a new CRD, and a new reconciler. We use the Ironic API to communicate with the BMC to manage node subscriptions.

andfasano · 2021-12-15T14:28:28Z

design/baremetal-operator/bmc-events.md

+- I'd like to configure my BMC to send events to a destination URI.
+- I'd like to provide context to a particular event subscription.
+- I'd like to configure an event type
+- I'd like to specify a BMC protocol


@honza probably this one could be removed now?

andfasano · 2021-12-15T14:31:48Z

design/baremetal-operator/bmc-events.md

+  BareMetalHost.
+- The BMCEventSubscription will maintain a reference to the subscription ID
+  obtained from the BMC.
+- The baremetal-operator binary will be expanded to include 2


currently there are more controllers (ie firmware settings), so I'd suggest to change it to "an additional controller for BMCEventSubscriptions resources"

This patch implements the fast eventing API: metal3-io/metal3-docs#167 We introduce a new CRD, and a new reconciler. We use the Ironic API to communicate with the BMC to manage node subscriptions.

andfasano · 2021-12-15T16:45:11Z

design/baremetal-operator/bmc-events.md

+- The baremetal-operator binary will be expanded to include an additional
+  reconciler with a dedicated controller/reconcile loop for
+  BMCEventSubscriptions.
+


Is it possible to expand this section to define more precisely the expected behavior in case of host deletion? In case the subscription resource remains alive, what will happen when/if the host will come back? What will be status shown to the user meanwhile?

kashifest · 2022-01-13T08:58:59Z

/hold
Until default branch of the repo is main

omichaeli · 2022-01-26T11:58:04Z

design/baremetal-operator/bmc-events.md

+metadata:
+  name: worker-1-events
+spec:
+   hostRef: ostest-worker-1


hostRef was changed to hostName

metal3-io-bot · 2022-01-26T12:06:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur

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:

~~OWNERS~~ [dtantsur]

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 · 2022-04-26T12:20:13Z

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

furkatgofurov7 · 2022-04-26T13:34:52Z

/remove-lifecycle stale

furkatgofurov7 · 2022-04-26T13:38:23Z

/hold
Until default branch of the repo is main

I believe this is completed, unholding
/hold cancel
@andfasano @iurygregory @omichaeli is there anything left to merge this, considering the implementation already landed in BMO and it has been approved?

andfasano · 2022-05-05T15:06:08Z

/hold
Until default branch of the repo is main

I believe this is completed, unholding /hold cancel @andfasano @iurygregory @omichaeli is there anything left to merge this, considering the implementation already landed in BMO and it has been approved?

Maybe @honza can follow up on this thread to see if there's anything missing and/or pending points?

honza · 2022-05-11T12:49:03Z

I think this is good to go.

furkatgofurov7 · 2022-06-27T21:44:24Z

@honza how do you feel about changing the status from provisional to implemented unless I am missing implementation details?

This design document contains an initial draft of an API for configuring BMC event subscriptions, using a new [Ironic API](https://storyboard.openstack.org/#!/story/2008366). Co-Authored-By: Honza Pokorny <honza@redhat.com>

honza · 2022-06-29T12:28:20Z

how do you feel about changing the status from provisional to implemented unless I am missing implementation details?

@furkatgofurov7 Done, thanks!

furkatgofurov7

/lgtm

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 9, 2021

stbenjam force-pushed the bmc-events branch 2 times, most recently from ac0e64e to d79c0a8 Compare March 9, 2021 12:06

stbenjam commented Mar 9, 2021

View reviewed changes

dhellmann reviewed Mar 9, 2021

View reviewed changes

stbenjam commented Mar 9, 2021

View reviewed changes

design/baremetal-operator/bmc-events.md Outdated Show resolved Hide resolved

andfasano reviewed Mar 10, 2021

View reviewed changes

stbenjam force-pushed the bmc-events branch from 28286aa to 9d220c3 Compare March 10, 2021 11:59

dhellmann reviewed Mar 10, 2021

View reviewed changes

jzding reviewed May 14, 2021

View reviewed changes

iurygregory suggested changes Aug 3, 2021

View reviewed changes

iurygregory reviewed Aug 4, 2021

View reviewed changes

dtantsur approved these changes Aug 18, 2021

View reviewed changes

iurygregory reviewed Sep 14, 2021

View reviewed changes

dtantsur reviewed Oct 15, 2021

View reviewed changes

honza mentioned this pull request Nov 4, 2021

Add BMC subscription CRD and reconciler metal3-io/baremetal-operator#1014

Merged

metal3-io-bot changed the title ~~Add draft design for BMC eventing API~~ Design for BMC eventing API Nov 18, 2021

honza force-pushed the bmc-events branch from 1e2c7ef to 496171c Compare November 18, 2021 18:54

andfasano reviewed Dec 15, 2021

View reviewed changes

honza force-pushed the bmc-events branch from a5f0d9d to 3a41931 Compare December 15, 2021 15:44

andfasano reviewed Dec 15, 2021

View reviewed changes

metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 13, 2022

kashifest changed the base branch from master to main January 13, 2022 09:28

omichaeli reviewed Jan 26, 2022

View reviewed changes

honza force-pushed the bmc-events branch from 3a41931 to 3daae33 Compare January 26, 2022 12:05

metal3-io-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 26, 2022

metal3-io-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 26, 2022

metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 26, 2022

Add design document for BMC eventing API

216d701

This design document contains an initial draft of an API for configuring BMC event subscriptions, using a new [Ironic API](https://storyboard.openstack.org/#!/story/2008366). Co-Authored-By: Honza Pokorny <honza@redhat.com>

honza force-pushed the bmc-events branch from 3daae33 to 216d701 Compare June 29, 2022 12:28

furkatgofurov7 reviewed Jul 5, 2022

View reviewed changes

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2022

metal3-io-bot merged commit 0f59e46 into metal3-io:main Jul 5, 2022


		### Upgrade / Downgrade Strategy

		Not required, this is a new API being introduced


		### Dependencies

		[Ironic Eventing API](https://storyboard.openstack.org/#!/story/2008366)


		## References

		- [Ironic Eventing API](https://storyboard.openstack.org/#!/story/2008366)


		### Dependencies

		[Ironic Vendor Passthru for Subscriptions](https://storyboard.openstack.org/#!/story/2009061)

Design for BMC eventing API #167

Design for BMC eventing API #167

Conversation

stbenjam commented Mar 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stbenjam Mar 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzding Jun 10, 2021 • edited

Choose a reason for hiding this comment

stbenjam Jun 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhellmann Apr 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhellmann commented Jul 15, 2021

honza commented Jul 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasu commented Nov 18, 2021

iurygregory commented Nov 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashifest commented Jan 13, 2022

stbenjam Mar 10, 2021 •

edited

jzding Jun 10, 2021 •

edited

stbenjam Jun 11, 2021 •

edited

dhellmann Apr 5, 2021 •

edited