✨ Implement API version negotiation #1435
Conversation
In preparation for the future microversion negotiation handling: 1) Remove the unnecessary ironicDependenciesChecker abstraction. It probably made more sense when Inspector was used explicitly. 2) Change tests to make Ready the default state (and NotReady an explicit override for the few tests that need it).
Rather than bumping the microversion every time we need new features, we will keep the baseline version and try to negotiate a higher version in the provisioner. The new AvailableFeatures object provides information about the supported features.
metal3-io-bot
added
the
size/L
|
/test-centos-e2e-integration-main |
|
/cc @lentzi90 @kashifest |
| // NOTE: Some versions of Ironic inspector returns 404 for /v1/ but 200 for /v1, | ||
| // which seems to be the default behavior for Flask. Remove the trailing slash | ||
| // from the client endpoint. | ||
| endpoint := strings.TrimSuffix(client.Endpoint, "/") |
There was a problem hiding this comment.
Is it certain that removing this will not cause any issues? Could there be users relying on this?
There was a problem hiding this comment.
From the Ironic's perspective, these URLs are identical (inspector did have this bug at some point). What scenario do you have in mind?
There was a problem hiding this comment.
I'm just wondering why this was here to begin with and why it would be safe to remove it now. Sounds like the answer to the first question is that inspector had a bug. Then the question remains, can we be certain that no user is running a version of inspector that is affected?
I guess this is old enough that anyone still using an affected inspector would be out of support anyway :)
There was a problem hiding this comment.
BMO no longer accesses Inspector directly, so it does not matter which version a user is running. It's between Ironic and Inspector now.
There was a problem hiding this comment.
Right! I forgot about that!
Nothing to see here, move along 😄
| logger.Info("supported Ironic API features", | ||
| "maxVersion", fmt.Sprintf("1.%d", af.MaxVersion), | ||
| "chosenVersion", af.ChooseMicroversion(), | ||
| "firmwareUpgrades", af.HasFirmwareUpgrades()) |
There was a problem hiding this comment.
Maybe rename to HasFirmwareUpdates ?- since the interface can be used to upgrade and downgrade firmware
There was a problem hiding this comment.
I was thinking that the log messages would read like .. "supported Ironic API features ... firmwareUpgrades = true". WDYT?
There was a problem hiding this comment.
I think this could me miss interpreted (they would think they can only use to Upgrade firmware, if they had to downgrade maybe they would do manually before using BMO? Updates make more sense I think..)
There was a problem hiding this comment.
Feel free to adjust the namings in your patch to make them consistent with what you propose.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: honza 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:
Approvers can indicate their approval by writing |
metal3-io-bot
added
the
approved
|
/retest |
2 similar comments
|
/retest |
|
/retest |
Rather than bumping the microversion every time we need new features,
we will keep the baseline version and try to negotiate a higher version
in the provisioner. The new AvailableFeatures object provides
information about the supported features.
Includes: