Add test for rolling pool update with VM availability check on updated host - #625
Add test for rolling pool update with VM availability check on updated host#625DatBewar wants to merge 6 commits into
Conversation
df76049 to
2d17a45
Compare
ce0785b to
6db10aa
Compare
DatBewar
changed the title
6db10aa to
1624439
Compare
DatBewar
changed the title
Ythogtha
left a comment
Ythogtha
left a comment
There was a problem hiding this comment.
Feels the way I'd be doing it manually for storage, so I'm ok with this test.
… from testing repo Signed-off-by: Rushikesh Jadhav <rushikesh7@gmail.com>
…po in pool_with_linstor This mechanism allows the test to start with our without testing repo packages Signed-off-by: Rushikesh Jadhav <rushikesh7@gmail.com>
Tests that want to test linstor upgrade can enablerepo during execution Signed-off-by: Rushikesh Jadhav <rushikesh7@gmail.com>
…ity check. - Uses `@pytest.mark.upgrade_test` to mark need of upgrade during the test - Updates of all hosts in a LINSTOR SR pool (starting with the master) - Reboots updated hosts - Verifies VM can start and shutdown successfully on each host after update - Ensures SR remains usable throughout the process Signed-off-by: Rushikesh Jadhav <rushikesh7@gmail.com>
…date` Signed-off-by: Erwan Croze <erwan.croze@vates.tech>
Use new function `yum_update` instead of redundant function added previously, also fix name of function to be more accurate on what's it does Signed-off-by: Erwan Croze <erwan.croze@vates.tech>
1624439 to
d5946aa
Compare
There was a problem hiding this comment.
The full history should be squashed before approval, IMO, to avoid merging it as is. I know that it was initially started by Rishi, but the code changed so much since that I don't think it's necessary to keep the history. We don't want PRs that do things then undo them. I would at least expect reviewers to say something about it.
But my biggest concern here is that I don't see how this test can produce any other result than "No updates available for host". The fixture installs linstor from xcp-ng-linstor-testing, then we try to update from xcp-ng-linstor-testing. Or am I missing something?
In my opinion, this PR started with an implementation when it should have started with a design. Then we asked @DatBewar to fix it but should have stepped back instead, and designed it first.
The core of the test is probably fine. But update tests depend on the setup and here that's the main obstacle. We must ENSURE that there will be updates to test. We must even make sure we know which updates we're actually testing.
Discussions on the design, involving the platform team, would have also shown that testing updates is a vast topic, that raises logistical questions as it depends entirely on the initial state of the pool that we're testing. And is really useful if we make it a rolling pool update with VMs running.
My proposition for what's next is: a call, with the relevant people from storage team and platform team.
Now, maybe I missed something obvious and the test is actually useful the way it's been approved, don't hesitate to tell me.
CC @glehmann
| hostA2: Host, | ||
| lvm_disks: None, | ||
| pool_with_saved_yum_state: Pool, | ||
| _linstor_config: LinstorConfig | ||
| _linstor_config: LinstorConfig, | ||
| ) -> Generator[Pool, None, None]: | ||
| import concurrent.futures | ||
|
|
||
| pool = pool_with_saved_yum_state | ||
|
|
||
| def ensure_linstor_not_installed(host: Host) -> None: |
There was a problem hiding this comment.
This doesn't seem to be directly related to this PR. Should at list be in a dedicated commit?
5 participants
New pr based on #310