-
Notifications
You must be signed in to change notification settings - Fork 60
[5/n] [reconfigurator-planning] gate adds/updates on zone image sources being known #8921
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
[5/n] [reconfigurator-planning] gate adds/updates on zone image sources being known #8921
Conversation
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
|
Need to update the planner tests. |
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
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.
Changes LGTM, just a tiny nit on one of the tests. Sorry for the unreasonable delay on this review!
|
|
||
|
|
||
|
|
||
| > # The above run would have triggered an SP update, which we should delete (we want to start |
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.
Not sure how much this matters, but technically the above plan scheduled an RoT update, not SP. Maybe
| > # The above run would have triggered an SP update, which we should delete (we want to start | |
| > # The above blueprint includes a pending MGS update, which we should delete (we want to start |
(I also wonder if delete-sp-update should be delete-mgs-update? Happy to file an issue to fix that up separately if you agree.)
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.
updated!
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
| .with_target_release_0_0_1() | ||
| .expect("set target release to 0.0.1") |
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.
Sorry about this late comment! Wondering if it's possible to test the .with_target_release_0_0_1() functionality in a separate test instead of within the test_update_{ZONE} tests? I'm currently working on #9001 which will make successful MGS driven updates a requirement before updating any zones. In that PR, to get these tests to pass I had to include SP component artifacts that match what we see in the simulated environment so that the planner knows to not attempt any MGS driven updates. I realise I can just change the version for the artifacts, but the issue arises with the Host OS artifact which is not chosen by the planner via version but rather by hash. This new version set by with_target_release_0_0_1() no longer has a predefined hash, but a new calculated one. This makes it pretty tricky to work with.
Another option, but maybe worse, would be to only set zone versions in the fake.0.0.1.toml file. 🤔 Thoughts?
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.
Discussed this with @karencfv in chat -- should be able to pull host OS hash versions from the target release.
Created using spr 1.3.6-beta.1
…es being known (#8921) Expand the set of gates for adds/updates to include the fact that zone image sources should be known. Add tests for this: * `cmds-mupdate-update-flow` contains the bulk of testing for this scenario. * I had to make tweaks to some tests, particularly to `cmds-target-release.txt`, in order to start running the test in earnest from the Artifact state rather than the InstallDataset state.
Expand the set of gates for adds/updates to include the fact that zone image sources should be known. Add tests for this:
cmds-mupdate-update-flowcontains the bulk of testing for this scenario.cmds-target-release.txt, in order to start running the test in earnest from the Artifact state rather than the InstallDataset state.Depends on:
Closes #8726.