⚠️ Use ironic inventory API instead of calling ironic-inspector #1355
Conversation
metal3-io-bot
added
do-not-merge/work-in-progress
|
/test-ubuntu-integration-main |
dtantsur
force-pushed
the
ironic-inventory
branch
from
8a880c8
to
a84273b
Compare
|
/test-ubuntu-integration-main |
dtantsur
force-pushed
the
ironic-inventory
branch
from
a84273b
to
c6482d4
Compare
|
/test-ubuntu-integration-main |
dtantsur
force-pushed
the
ironic-inventory
branch
from
c6482d4
to
6525397
Compare
|
/test-ubuntu-integration-main |
metal3-io-bot
added
size/L
|
/test-ubuntu-integration-main |
This removes the last direct dependency on ironic-inspector.
dtantsur
force-pushed
the
ironic-inventory
branch
from
6525397
to
ee69555
Compare
|
/test-ubuntu-integration-main |
dtantsur
changed the title
metal3-io-bot
removed
the
do-not-merge/work-in-progress
metal3-io-bot
added
size/XL
|
/test-ubuntu-integration-main |
|
@dtantsur it might perhaps be helpful to clarify the Ironic dependencies related to this change (since not all users are consuming the upstream/master images), e.g I see the API docs say Looking at the OpenStack stable branches and the release notes it looks like this requires Ironic 21.3.0 so this should work with |
|
@hardys (hey Steve, been a while!) this is an interesting point, would you like to bring it to the upstream meeting? The thing is, we've never expressed any explicit requirement on Ironic, essentially assuming that everyone is using the latest Ironic. Meanwhile, https://docs.openstack.org/ironic/latest/contributor/webapi-version-history.html is a helpful link to match API versions to Ironic releases, in this case - from the Antelope series (we forgot to put exact numbers, I'll fix that). |
Sure I'll raise it in the meeting so we can hopefully ensure we're all aligned - IMO assuming everyone is using latest/main Ironic probably isn't ideal - but in this case it seems the change is compatible with the current stable |
|
/test-centos-e2e-integration-main |
|
/test-ubuntu-integration-main |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zaneb 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
| @@ -5,7 +5,7 @@ go 1.20 | |||
| require ( | |||
| github.com/go-logr/logr v1.2.4 | |||
| github.com/google/safetext v0.0.0-20230106111101-7156a760e523 | |||
| github.com/gophercloud/gophercloud v1.6.0 | |||
| github.com/gophercloud/gophercloud v1.5.1-0.20230912084133-c79ed6d3b371 | |||
There was a problem hiding this comment.
I'm a bit uncomfortable with this. Is the goal to merge it with the development commit or wait for gophercloud v2 to be released?
There was a problem hiding this comment.
V2 may take more time to get releases. What's the cause of your discomfort? For better or worse, using git commits is a normal thing in the Go world. The last time I tried to hurry gophercloud folks about a release they went "wtf, just use a git hash".
There was a problem hiding this comment.
Would this be landing in v2 only? Do you have any guess when would that be? AFAIK when we need to test commits that has landed in dev branches but not ready for release yet, we use pseudo-versions with git hashes. Use of pseudo version signals module is still under development and not stable. But since go has now stricter rules now to generate pseudo-numbers, it can be used incase we cannot wait for proper release to exist.
There was a problem hiding this comment.
Yep, this is going to v2 only because this work involved breaking changes. I don't know when that happens, we can ask on slack.
I think I'm using what you're describing here. Since the last release before splitting the 1.* branch was 1.5.0, it assumes the new version will 1.5.1-something (which is wrong, but go tell go!)
There was a problem hiding this comment.
What's the cause of your discomfort?
I'm mainly worried about the upgrade path and the potential for API changes that would impact us. It is probably not an issue but I would not want to just skip over it without thinking it through. So how do we expect the upgrade will look like?
We start with a scenario where a user is running an older version of BMO/Ironic relying on the older API and gophercloud. What would be required to upgrade? My understanding is something like this:
- Upgrade Ironic to a version that supports microversion 1.81. (Is something else needed here to "refresh" the inventory, I guess not?)
- Upgrade BMO.
This seems easy enough, but definitely worth a mention in the release notes for BMO.
Now my worry is that there would be some migration steps needed to go from gophercloud v1 to v2 and we end up somewhere in the middle. Do you see any situation where some kind of patch would be needed to support the bump to v2? (I know of projects where bugs have made it impossible to upgrade from e.g. 0.3.1 to 0.4.0. Instead users were required to first upgrade to the latest patch release, e.g. 0.3.4 and from there on to 0.4.0.) For example, could there be a required upgrade path for gophercloud to do v1.7.1 -> v2.0.0 and we miss it because we do not have the v1.7.1 patch?
There was a problem hiding this comment.
Hmm, you seem to be conflating the Ironic upgrade and the Gophercloud upgrade.
Gophercloud is just an HTTP library. 2.0.0 will see some Go API changes, but these are easy to address on any bump. For better or worse, Go does not update anything automatically (we will need to keep an eye on dependabot, of course). This is why I'm not sure what you mean by "we don't have the v1.7.1 patch". Gophercloud has no local state.
Ironic upgrade is a bit different. Historically, we have taken bumping microversions very lightly. Now Steven has raised a valid point that some of our consumers may not always use the latest-and-greatest versions. Fair. We need to start having some sort of upgrade docs per release (which I assume don't exist now).
In any case, what are the suggested action items here?
There was a problem hiding this comment.
Thanks for the explanation, that makes sense!
It seems like there is not much reason to worry. We just need to document the Ironic version bump so users notice it.
Perhaps we can bring this up at the community meeting today also to make sure developers are aware of it, in case there are any concerns?
metal3-io-bot
added
the
do-not-merge/hold
|
Since this specific feature is using a feature from newest ironic release, Shall we have a BMO release before this patch lands to make sure whoever is using an older version ironic with BMO, shall atleast have a newer BMO release to work with. Also this looks like a ✨ to me. |
+1, wanna do that?
It's not user-visible though, hence my choice. |
It is definitely breaking to users that depend on a specific (older) version of Ironic though since this bumps the required version. I would say |
dtantsur
changed the title
@metal3-io/metal3_release_team can you folks do a BMO release before this PR merges. |
We checked the main branch content, and there is not a lot to release before we have first breaking change/new feature merged after 0.4.0. 0.4.1 is hence out of the question already and if we're bumping to 0.5.0, this should fit right in with the other changes for later release in regular schedule. |
Good point, two ways, we go to v0.5.0 if we cut a release now, or we dont cut a release and do a minor release in due time. Its a pity again that we won't be able to patch releases 0.4.x anymore. |
OK, as discussed: we are already working to add CI for release branches, so we keep doing that and will release 0.4.1 from the branch when able. 0.5.0 will be release from main later on. The key here is that Ironic image is not pinning Ironic source, so releasing BMO from where ever does not magically build old Ironic anymore. We need Ironic source pinning for that. |
Yep, we had a good discussion about this, just to add a minor note, branching 0.4.0 out at an "earlier" point in history and working on that branch will serve as a good separation line between what has come before this commit and what has come after. We also don't need to rush 0.5.0 release (or any release) with this approach . |
|
Okay, so are we good to proceed with this PR? |
Yes. |
metal3-io-bot
removed
the
do-not-merge/hold
|
Forgot about this: baremetal-operator/docs/configuration.md Line 73 in 137006a Please update the required API version there @dtantsur |
|
/test-ubuntu-integration-main |
|
/test-ubuntu-integration-main |
|
/lgtm |
Removes the direct dependency on ironic-inspector, leaving it an implementation detail of Ironic.
Note for reviewers: it's much easier to review each of 3 commits separately.