-
Notifications
You must be signed in to change notification settings - Fork 897
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
Introduce FirmwareRegistry and FirmwareBinary (with FirmwareTargets) #18774
Conversation
Hey @agrare not sure if you're aware what we're doing with these firmware thing, so let me recap our current plan: we have a requirement to support operation "update firmware" on physical server (Redfish). We'll be doing it via a (yet not existing) internal state machine. But MIQ currently has no model for "installable firmware software" that user could pick from a drop-down to update server with. So we need to introduce the new model first. Looking forward to your comments. |
1d51b95
to
8ea4d1a
Compare
Provided a bunch of unit tests, VCR ones as well. |
/cc @gtanzillo |
8ea4d1a
to
c439b59
Compare
c439b59
to
efa0f4c
Compare
efa0f4c
to
754206a
Compare
754206a
to
8df2bd0
Compare
8df2bd0
to
84f8249
Compare
Sorry for a bit of delay here @agrare I think we're green now. This PR seems huge, but it's mostly due to all the FactoryBot definitions and specs we are providing. In essence we're only delivering the firmware inventory, as simple as it can get... Would you prefer if I try to split the PR in smaller parts? |
Kicking travis because rspec only failed for ruby 2.4.6 and I can't reproduce locally on same ruby version |
84f8249
to
5c63bdf
Compare
app/models/firmware_binary.rb
Outdated
end | ||
|
||
def urls | ||
endpoints.map(&:url) |
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.
.pluck(:url)
is slightly more efficient
@@ -0,0 +1,59 @@ | |||
class FirmwareRegistrySimple < FirmwareRegistry |
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.
Hm wonder if we could come up with something more descriptive than Simple
...maybe Http
or HttpFileServer
?
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.
That is essentially all this registry type is right? Just an http file server with authentication?
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.
Yeah, kind of: it's http server that servers file metadata; the actual files may be stored separately (on some external ftp or samba or something)
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.
Okay, I ended up with a small refactoring. Now we have
firmware_registry
|- rest_api_depot.rb # actual STI-driven implementation
firmware_registry.rb # base class
I think
class FirmwareRegistry::RestApiDepot
is better than HttpFileServer because we emphasise the metadata part. Thanks for kicking my ass, I didn't like Simple either because it tells literally nothing.
class FirmwareRegistrySimple < FirmwareRegistry | ||
def sync_fw_binaries_raw | ||
remote_binaries.each do |binary_hash| | ||
binary = FirmwareBinary.find_or_create_by(:firmware_registry => self, :external_ref => binary_hash['id']) |
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 feels like something we could use graph refresh for treating the registry as the manager
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 can do that in a follow up though I won't hold this up for that
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've asked @Ladas for some guides how to employ graph refresh on non-ems classes, but it should really be done as a followup IMAO. Because quite some files will have to be added (at least collector, parser, persister, definitions etc.).
|
||
unless binary.urls.sort == binary_hash['urls'].sort | ||
_log.info("Updating FirmwareBinary [#{binary.id} | #{binary.name}] endpoints...") | ||
binary.endpoints.destroy_all |
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 could be a bit better if we consider what needs to be deleted and what needs to be added,
endpoints_by_url = binary.endpoints.index_by(&:url)
urls_to_delete = binary.urls - binary_hash['urls']
urls_to_delete.each { |url| endpoints_by_url[url].destroy }
urls_to_create = binary_hash['urls'] - binary.urls
urls_to_create.each { |url| binary.endpoints.create!(:url => url) }
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.
Sweet, thanks, integrated. I assumed that binary endpoints are so rarely updated that we don't care performance on such event, but it can't hurt to do it properly.
|
||
currents = binary.firmware_targets.map(&:to_hash) | ||
remotes = binary_hash['compatible_server_models'].map { |r| r.symbolize_keys.slice(*FirmwareTarget::MATCH_ATTRIBUTES) } | ||
unless currents.map(&:to_a).sort == remotes.map(&:to_a).sort |
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.
Do you have an example of what the data here would look like? Having a hard time picturing what is going on with the .map(&:to_hash)
above then the .map(&:to_a)
here
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.
end | ||
|
||
def to_hash | ||
attributes.symbolize_keys.slice(*MATCH_ATTRIBUTES) |
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.
Would it make sense to not store attributes that aren't needed instead of overriding to_hash?
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 mainly get rid of created/updated timestamps here to get a "pure" hash with relevant attributes only (:manufacturer and :model). Perhaps I should just rename the function to something else?
9b6b8c6
to
3535018
Compare
I couldn't figure why Travis won't use VCR for examples marked with |
3535018
to
37decaf
Compare
37decaf
to
b06a7e1
Compare
One can update physical server's firmware, but the firmware has to be accessible somewhere, usually on some HTTP/FTP/SAMBA link. Furthermore, there is usually some metadata associated with the firmware to tell what kind of hardware (manufacturer, model type) is it meant for. With this commit we introduce a new model, FirmwareRegistry, that supports various kinds of metadata parsing by leveraging STI. User is supposed to create FirmwareRegistry instance via UI feeding it url+authentication and then click "Refresh Relationships" on it. ManageIQ will then invoke ``` registry.sync_fw_binaries_queue ``` which in turn will connect to the metadata repository to list available firmwares and store them as FirmwareBinary (no binary data is really stored, only HTTP/FTP/SAMBA link and some metadata). In followup PRs we will then be able to list available firmware binaries to update physical server's firmware. Signed-off-by: Miha Pleško <[email protected]>
b06a7e1
to
481b3dc
Compare
Checked commit xlab-si@481b3dc with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 spec/vcr_cassettes/firmware_registry/rest_api_depot_when-200.yml
spec/vcr_cassettes/firmware_registry/rest_api_depot_when-bad-credentials.yml
spec/vcr_cassettes/firmware_registry/rest_api_depot_when-bad-json.yml
|
@agrare this is now green. Had quite some issues with VCR being turned off, not sure why or who does it, so I ended up forcibly turning them on |
end | ||
# Is seems some other test is turning VCR off in some random cases. | ||
# We're best off manually turning it on. | ||
VCR.turn_on! |
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.
Hm this is strange that it'd be needed, I'm okay with this for now but we should look at why it is needed
One can update physical server's firmware, but the firmware has to be accessible somewhere, usually on a HTTP/FTP/SAMBA link. Furthermore, there is usually some metadata associated with the firmware to tell what kind of hardware (manufacturer, model type) is it meant for.
With this commit we introduce a new model, FirmwareRegistry, that supports various kinds of metadata parsing by leveraging STI. User is supposed to create FirmwareRegistry instance via UI feeding it url+authentication and then click "Refresh Relationships" on it. ManageIQ will then invoke
which in turn will connect to the metadata repository to list available firmwares and store them as FirmwareBinary (no binary data is really stored, only HTTP/FTP/SAMBA link and some metadata).
In followup PRs we will then be able to list available firmware binaries to update physical server's firmware.
Depends on:
@miq-bot add_label enhancement
@miq-bot assign @agrare
/cc @tadeboro @matejart
Example usage: