Skip to content
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

Update scsi id boot from slof for ppc64le #14681

Merged
merged 1 commit into from Apr 28, 2022

Conversation

tinawang123
Copy link
Contributor

@tinawang123 tinawang123 commented Apr 8, 2022

New device serial will be added which caused the scsi device id
changed from 5 to 7. Got the scsi id from devalias command.

@github-actions
Copy link

github-actions bot commented Apr 8, 2022

Great PR! Please pay attention to the following items before merging:

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files

@tinawang123
Copy link
Contributor Author

tinawang123 commented Apr 8, 2022

@okurz Please take a look. Refer old PR: #6312

okurz
okurz previously requested changes Apr 8, 2022
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you put into the description of the pull request could also be in the commit message details. I don't know how you created your pull request but your git commit messages has only the subject line while the pull request description has more details. Please keep in mind that the github pull request description is only visible on github, the commit can be considered permanent information storage.

I can recommend the tool hub (zypper in rubygem-hub) for easier PR creation. Also, I myself use a script git-pr-last to create a PR with proper description for these simple one-commit PRs:

$ cat $(which git-pr-last )
#!/bin/sh -e
target="${target:-"$USER"}"
git push $target && git show --no-patch --format=%B | hub pull-request -F -

See https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/CONTRIBUTING.md#coding-style for more details

Also, why adding 2 in case of virtio-console is not obvious to me. Is this valid for all product versions? Instead of applying such magic number please consider a more robust automatic resolution of the necessary device

@tinawang123
Copy link
Contributor Author

@okurz At grub shell, we cannot use command to get scsi id. Refer case: https://openqa.suse.de/tests/8502556#step/boot_to_desktop/6. This blocked all of my powerKVM cases.

@rfan1
Copy link
Contributor

rfan1 commented Apr 11, 2022

@tinawang123

I would suggest you find the RC why the disk id is changed before merging this request

  1. Is it caused by new qemu backend device changes?
  2. Is it caused by new ISO image [new kernel dirvers loaded process]?
  3. Can we do some more re-try logic to make sure it can boot the "right" disk device before fail cases there? [e.g. try scsi6, 7, and 8
    ]

@tinawang123
Copy link
Contributor Author

tinawang123 commented Apr 11, 2022

@tinawang123

I would suggest you find the RC why the disk id is changed before merging this request

1. Is it caused by new qemu backend device changes?

2. Is it caused by new ISO image [new kernel dirvers loaded process]?

3. Can we do some more re-try logic to make sure it can boot the "right" disk device before fail cases there? [e.g. try scsi6, 7, and 8
   ]

We can investigate those situation, but now it blocked all my powerKVM cases. I need fix it first.

@lemon-suse
Copy link
Contributor

In fact, it is clearly insert a device which caused the scsi id increated from 6->7.
The device info on 119.1 https://openqa.suse.de/tests/8450987/logfile?filename=serial0.txt
Populating /pci@800000020000000
00 0000 (D) : 1234 1111 qemu vga
00 0800 (D) : 8086 2668 multimedia-device*
00 1000 (D) : 1af4 1000 virtio [ net ]
00 1800 (D) : 1033 0194 serial bus [ usb-xhci ]
00 2000 (D) : 1af4 1003 virtio [ serial ]
00 2800 (D) : 1af4 1004 virtio [ scsi ]
The device info on 124.1: https://openqa.suse.de/tests/8486442/logfile?filename=serial0.txt
Populating /pci@800000020000000
00 0000 (D) : 1234 1111 qemu vga
00 0800 (D) : 8086 2668 multimedia-device*
00 1000 (D) : 1af4 1000 virtio [ net ]
00 1800 (D) : 1af4 1005 legacy-device*
00 2000 (D) : 1033 0194 serial bus [ usb-xhci ]
00 2800 (D) : 1af4 1003 virtio [ serial ]
00 3000 (D) : 1af4 1004 virtio [ scsi ]

So the new inserted device is ' 00 1800 (D) : 1af4 1005 legacy-device*'.

Compared the openQA cmd, I found since 124.1 it added a new device, '-device virtio-rng-pci,rng=rng0', I think it is this device inserted caused scsi id need change from 6 to 7.

@tinawang123
Copy link
Contributor Author

Use setting "QEMU_VIRTIO_RNG=0 " can workaround this problem. But we should fix this one with correct scsi ID.

@foursixnine
Copy link
Member

Use setting "QEMU_VIRTIO_RNG=0 " can workaround this problem. But we should fix this one with correct scsi ID.

I Guess https://progress.opensuse.org/issues/109980#note-4 may be related, but indeed @tinawang123 it should help for now.

In fact, it is clearly insert a device which caused the scsi id increated from 6->7. The device info on 119.1
...
Compared the openQA cmd, I found since 124.1 it added a new device, '-device virtio-rng-pci,rng=rng0', I think it is this device inserted caused scsi id need change from 6 to 7.

Is it possible that the qemu versions are different and are causing the problem? but also there's the change in the bootloader (see progress ticket)

@coolgw
Copy link
Contributor

coolgw commented Apr 20, 2022

@foursixnine @okurz Is there any smart way detect scsi id in grub mode?

@okurz
Copy link
Member

okurz commented Apr 20, 2022

@foursixnine @okurz Is there any smart way detect scsi id in grub mode?

I don't know any specific way. I suggest you research how qemu behaves and what should be expected from that.

lib/bootloader_setup.pm Outdated Show resolved Hide resolved
lib/bootloader_setup.pm Outdated Show resolved Hide resolved
Copy link
Contributor

@hjluo hjluo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lib/bootloader_setup.pm Show resolved Hide resolved
New device serial will be added which caused the scsi device id
changed from 5 to 7. Got the scsi id from devalias command.
@coolgw
Copy link
Contributor

coolgw commented Apr 27, 2022

@okurz could you help check again?

@foursixnine foursixnine requested a review from okurz April 27, 2022 15:50
@foursixnine foursixnine dismissed okurz’s stale review April 28, 2022 13:50

Oliver's objection was addressed. Commit states the "why".

@foursixnine foursixnine merged commit 5ca0c7d into os-autoinst:master Apr 28, 2022
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks nice, thank you

@tinawang123 tinawang123 deleted the boot branch May 6, 2022 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants