Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MEP-15: HAL Improvements #232
base: master
Are you sure you want to change the base?
MEP-15: HAL Improvements #232
Changes from 1 commit
b4a27ee
8a5f1fd
df928e9
1a35b21
1cd4ccb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 27 in docs/src/development/proposals/MEP15/index.md
GitHub Actions / build
Check warning on line 27 in docs/src/development/proposals/MEP15/index.md
GitHub Actions / build
Check warning on line 31 in docs/src/development/proposals/MEP15/index.md
GitHub Actions / build
Check warning on line 35 in docs/src/development/proposals/MEP15/index.md
GitHub Actions / build
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.
One key aspect of test-ability is that for every exposed API call a full set of CRUD operations exists, for example if there is a
GetIdentifierLEDState
call present, there must be aSetIdentifierLEDState
available to be able to test the this single functionality. Same must apply for all other endpoints if possible. Sometimes its not possible for exampleGetBoardInfo
.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.
Is
CRUD
then really the correct term? Because I think we cannot "create" something through this API. I think you want an opportunity to get the current state in case we have a setter for something, right?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 imagine to have a single driver per machine type which covers all of our requirements.
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.
A vendor implementation may provide a fallback to e.g. the IPMI driver if it is available.
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.
Or maybe we can include fallback into our API directly and specify a list of drivers:
If one driver errors out, it will automatically fallback to the next.
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.
idiscover is pretty old, we could just do a golang based port scan for the BMC network:
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.
idiscover was just mentioned as an example of how a discovery can be done. Of course it should be implemented properly in Golang.
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.
In the past we see often that the BMC renews its IP address with a new one, most often after a hard reset of the BMC or if the firmware of the BMC got updated. This needs to be considered.
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.
UUID is already calculated and send back to the metal-api within the pixiecore by inspecting the DHCP Bootp Request. This must be adopted eventually.
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 pixiecore only knows the UUID but not the BMC mac address. I think it can stay as it is. The pixiecore can create the empty machine entity, but it cannot establish the relation between BMC and machine.
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.
This requires IPMI inband access which might not be taken for granted.
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.
We could probably drop BIOS update and keep BMC Update functionality because the later does not require a system power reset.
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.
But how often did we really use it? 😅
I think the current implementation is kind of dangerous because it is not prevented from us that multiple update requests are issued in parallel against a single BMC. The operator does not even know if an update is currently in progress or not.
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.
Yes, but this is a implementation flaw which must be addressed