MGMT-10542: Allow users to skip writing over specific disks - SaaS #4199
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
omertuc
changed the title
openshift-ci
bot
added
do-not-merge/work-in-progress
omertuc
force-pushed
the
skip
branch
3 times, most recently
from
f649c8f
to
302d959
Compare
openshift-ci
bot
added
the
api-review
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: omertuc 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 |
openshift-ci
bot
added
the
approved
Codecov Report
@@ Coverage Diff @@
## master #4199 +/- ##
=======================================
Coverage 65.86% 65.86%
=======================================
Files 188 188
Lines 26482 26482
=======================================
Hits 17443 17443
Misses 7442 7442
Partials 1597 1597 |
|
/retest |
omertuc
force-pushed
the
skip
branch
3 times, most recently
from
0d63dcf
to
4db7f30
Compare
openshift-ci
bot
added
size/XL
omertuc
force-pushed
the
skip
branch
4 times, most recently
from
a100351
to
270f20b
Compare
omertuc
changed the title
openshift-ci
bot
added
size/XXL
omertuc
force-pushed
the
skip
branch
3 times, most recently
from
59a253a
to
520dd32
Compare
|
/retest |
|
/retest |
avishayt
reviewed
Aug 4, 2022
swagger.yaml
Outdated
Comment on lines
4069
to
4071
| A comma seperated list of host disks that the service will avoid | ||
| formatting no matter what. This parameter can be modified via | ||
| v2UpdateHost |
There was a problem hiding this comment.
Suggested change
| A comma seperated list of host disks that the service will avoid | |
| formatting no matter what. This parameter can be modified via | |
| v2UpdateHost | |
| A comma-seperated list of host disks that the service will avoid | |
| formatting. |
swagger.yaml
Outdated
Comment on lines
4060
to
4064
| A comma separated list of disks that are automatically planned to | ||
| undergo formatting once installation begins, unless otherwise set to | ||
| be skipped by skip_formatting_disks. This means that this list also | ||
| includes disks that appear in skip_formatting_disks. This parameter | ||
| is managed by the service and cannot be modified by the user. |
There was a problem hiding this comment.
Suggested change
| A comma separated list of disks that are automatically planned to | |
| undergo formatting once installation begins, unless otherwise set to | |
| be skipped by skip_formatting_disks. This means that this list also | |
| includes disks that appear in skip_formatting_disks. This parameter | |
| is managed by the service and cannot be modified by the user. | |
| A comma-separated list of disks will be formatted | |
| once installation begins, unless otherwise set to | |
| be skipped by skip_formatting_disks. This means that this list also | |
| includes disks that appear in skip_formatting_disks. This property | |
| is managed by the service and cannot be modified by the user. |
internal/host/validator.go
Outdated
| func (v *validator) noSkipInstallationDisk(c *validationContext) (ValidationStatus, string) { | ||
| const ( | ||
| successMessage string = "No request to skip formatting of the installation disk" | ||
| failureMessage string = "Requesting to skip the formatting of the installation disk is not allowed. The installation disk has to be formatted. Please either change this host's installation disk or don't skip the formatting of the installation disk" |
There was a problem hiding this comment.
Suggested change
| failureMessage string = "Requesting to skip the formatting of the installation disk is not allowed. The installation disk has to be formatted. Please either change this host's installation disk or don't skip the formatting of the installation disk" | |
| failureMessage string = "Requesting to skip the formatting of the installation disk is not allowed. The installation disk must be formatted. Please either change this host's installation disk or do not skip the formatting of the installation disk" |
| return ValidationFailure, failureMessage | ||
| } | ||
|
|
||
| func (v *validator) noSkipMissingDisk(c *validationContext) (ValidationStatus, string) { |
There was a problem hiding this comment.
This should never happen because the identifiers that we now use are hardware-based, but I appreciate your paranoia :)
tl;dr This is an implementation of a feature that gives users the ability to tell Assisted to not format disks that it otherwise would. # Background After the assisted-installer writes the OCP operating system to the host's installation disk and then reboots the host, we can only hope that the disk that will boot after that reboot is the installation disk and not some other device. After the reboot, one of the following three things could happen: 1) The discovery ISO media gets booted, again. This is bad, but we can detect this and let the user know. 2) Some disk other than the installation disk gets booted. This is bad, and there's nothing we can do about it. We can't even tell for sure if that happened. 3) The installation disk boots and everything is fine and installation / cluster initialization proceeds as expected. On UEFI based systems we can usually make (3) happen using `efibootmgr`, on BIOS based systems we cannot influence the boot process in the same way. However, there is one thing yet we try to do to avoid (2) - during installation the assisted-service asks the assisted-installer to attempt to format the boot partition of some of the disks that the assisted-agent recognized as "Bootable" (these are disks that have an MBR bootloader as their first sector). By attempting to clear the boot sector of these disks, we hope that BIOS will no longer recognize these disks as bootable and so will not try to automatically boot from them, thus preventing (2). # The issue ALL disks that we recognize as bootable (save for some special exceptions) get formatted, all for the sake of ensuring the user won't run into confusing boot order issues. This is an opinionated decision we made to improve the ease of use of the Assisted Installer, but it comes at a cost of destroying user disks, some of which could potentially be important and might be used by the cluster / server hardware. Some users may be unhappy with this decision and may want to have more control. # The solution Give users an abiltiy to opt out of formatting particular disk(s). When users choose to opt out of formatting, the UI (not as part of this PR) will show them a warning explaining that it is on them to ensure they won't run into any boot order issues, and it's up to them to take care of it. # Implementation Two new API changes have been introduced - * New read-only `disks_to_be_formatted` column added to every host. This column is managed by the service. It's a comma separated list of disks that are automatically planned to undergo formatting once installation begins, unless otherwise set to be skipped by `skip_formatting_disks`. The reason this new API was added is because we would like the new UI for this feature to also show the users which disks are planned to undergo formatting. * New user-updatable `skip_formatting_disks` column. Can be modified through a newly added host UPDATE API. The structure of this new UPDATE API is a list of disks, and for each disk, whether the user wants it removed-from or added-to the `skip_formatting_disks` list. In order to make sure that direct API users didn't make a typo (which could lead to potentially harmful formatting), this API would also validate that the disk ID that the user asked to skip actually appears in the inventory. This also ensures the user doesn't choose to skip a disk before the inventory arrives - as that could lead to the installation disk being chosen to be one of the skipped disks, which would be awkward. Finally, when determining which disks the assisted-service should ask the assisted-installer to format, the assisted-service will consult both `disks_to_be_formatted` and `skip_formatting_disks` to decide on the final list (the final list is a simple `set(disks_to_be_formatted)` minus `set(skip_formatting_disks)`) # Installation disk I've also added a blocking validation to hosts which checks that the installation disk of the host does not appear in the host's `skip_formatting_disks` list. This validation would be the simplest way to block the user from asking to "skip the formatting of the installation disk" (which would obviously be non-sensical, as the installation disk has to be formatted). Another way to solve this would be to block the user during the host UPDATE API call, but that would be mixing / tightly coupling the installation disk choice API with the new skip formatting disks API. An "offline" validation happening in the background is simpler. # Missing skip-formatting disks validation If for some reason one of the disks that is set to be skipped no longer appears in the inventory, a new validation will block installation. This is to ensure that if the user reboots the system and somehow the disk they asked to skip the formatting of changed ID, then they would have to manually and consciously remove it from the `skip_formatting_disks` list.
|
/lgtm |
|
/test edge-e2e-ai-operator-ztp |
|
@omertuc: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
jkilzi
reviewed
Aug 28, 2022
danielerez
pushed a commit
to danielerez/assisted-service
that referenced
this pull request
Oct 15, 2023
…penshift#4199) tl;dr This is an implementation of a feature that gives users the ability to tell Assisted to not format disks that it otherwise would. # Background After the assisted-installer writes the OCP operating system to the host's installation disk and then reboots the host, we can only hope that the disk that will boot after that reboot is the installation disk and not some other device. After the reboot, one of the following three things could happen: 1) The discovery ISO media gets booted, again. This is bad, but we can detect this and let the user know. 2) Some disk other than the installation disk gets booted. This is bad, and there's nothing we can do about it. We can't even tell for sure if that happened. 3) The installation disk boots and everything is fine and installation / cluster initialization proceeds as expected. On UEFI based systems we can usually make (3) happen using `efibootmgr`, on BIOS based systems we cannot influence the boot process in the same way. However, there is one thing yet we try to do to avoid (2) - during installation the assisted-service asks the assisted-installer to attempt to format the boot partition of some of the disks that the assisted-agent recognized as "Bootable" (these are disks that have an MBR bootloader as their first sector). By attempting to clear the boot sector of these disks, we hope that BIOS will no longer recognize these disks as bootable and so will not try to automatically boot from them, thus preventing (2). # The issue ALL disks that we recognize as bootable (save for some special exceptions) get formatted, all for the sake of ensuring the user won't run into confusing boot order issues. This is an opinionated decision we made to improve the ease of use of the Assisted Installer, but it comes at a cost of destroying user disks, some of which could potentially be important and might be used by the cluster / server hardware. Some users may be unhappy with this decision and may want to have more control. # The solution Give users an abiltiy to opt out of formatting particular disk(s). When users choose to opt out of formatting, the UI (not as part of this PR) will show them a warning explaining that it is on them to ensure they won't run into any boot order issues, and it's up to them to take care of it. # Implementation Two new API changes have been introduced - * New read-only `disks_to_be_formatted` column added to every host. This column is managed by the service. It's a comma separated list of disks that are automatically planned to undergo formatting once installation begins, unless otherwise set to be skipped by `skip_formatting_disks`. The reason this new API was added is because we would like the new UI for this feature to also show the users which disks are planned to undergo formatting. * New user-updatable `skip_formatting_disks` column. Can be modified through a newly added host UPDATE API. The structure of this new UPDATE API is a list of disks, and for each disk, whether the user wants it removed-from or added-to the `skip_formatting_disks` list. In order to make sure that direct API users didn't make a typo (which could lead to potentially harmful formatting), this API would also validate that the disk ID that the user asked to skip actually appears in the inventory. This also ensures the user doesn't choose to skip a disk before the inventory arrives - as that could lead to the installation disk being chosen to be one of the skipped disks, which would be awkward. Finally, when determining which disks the assisted-service should ask the assisted-installer to format, the assisted-service will consult both `disks_to_be_formatted` and `skip_formatting_disks` to decide on the final list (the final list is a simple `set(disks_to_be_formatted)` minus `set(skip_formatting_disks)`) # Installation disk I've also added a blocking validation to hosts which checks that the installation disk of the host does not appear in the host's `skip_formatting_disks` list. This validation would be the simplest way to block the user from asking to "skip the formatting of the installation disk" (which would obviously be non-sensical, as the installation disk has to be formatted). Another way to solve this would be to block the user during the host UPDATE API call, but that would be mixing / tightly coupling the installation disk choice API with the new skip formatting disks API. An "offline" validation happening in the background is simpler. # Missing skip-formatting disks validation If for some reason one of the disks that is set to be skipped no longer appears in the inventory, a new validation will block installation. This is to ensure that if the user reboots the system and somehow the disk they asked to skip the formatting of changed ID, then they would have to manually and consciously remove it from the `skip_formatting_disks` list.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tl;dr This is an implementation of a feature that gives users the
ability to tell Assisted to not format disks that it otherwise would.
Background
After the assisted-installer writes the OCP operating system to the
host's installation disk and then reboots the host, we can only hope
that the disk that will boot after that reboot is the installation disk
and not some other device.
After the reboot, one of the following three things could happen:
The discovery ISO media gets booted, again. This is bad, but we can
detect this and let the user know.
Some disk other than the installation disk gets booted. This is bad,
and there's nothing we can do about it. We can't even tell for sure if
that happened.
The installation disk boots and everything is fine and installation
/ cluster initialization proceeds as expected.
On UEFI based systems we can usually make (3) happen using
efibootmgr,on BIOS based systems we cannot influence the boot process in the same
way. However, there is one thing yet we try to do to avoid (2) - during
installation the assisted-service asks the assisted-installer to attempt
to format the boot partition of some of the disks that the
assisted-agent recognized as "Bootable" (these are disks that have an
MBR bootloader as their first sector).
By attempting to clear the boot sector of these disks, we hope that BIOS
will no longer recognize these disks as bootable and so will not try to
automatically boot from them, thus preventing (2).
The issue
ALL disks that we recognize as bootable (save for some special
exceptions) get formatted, all for the sake of ensuring the user won't
run into confusing boot order issues. This is an opinionated decision we
made to improve the ease of use of the Assisted Installer, but it comes
at a cost of destroying user disks, some of which could potentially be
important and might be used by the cluster / server hardware. Some users
may be unhappy with this decision and may want to have more control.
The solution
Give users an abiltiy to opt out of formatting particular disk(s). When
users choose to opt out of formatting, the UI (not as part of this PR)
will show them a warning explaining that it is on them to ensure they
won't run into any boot order issues, and it's up to them to take care
of it.
Implementation
Two new API changes have been introduced -
New read-only
disks_to_be_formattedcolumn added to every host. Thiscolumn is managed by the service. It's a comma separated list of disks
that are automatically planned to undergo formatting once installation
begins, unless otherwise set to be skipped by
skip_formatting_disks.The reason this new API was added is because we would like the new UI
for this feature to also show the users which disks are planned to
undergo formatting.
New user-updatable
skip_formatting_diskscolumn. Can be modifiedthrough a newly added host UPDATE API. The structure of this new
UPDATE API is a list of disks, and for each disk, whether the user
wants it removed-from or added-to the
skip_formatting_diskslist. Inorder to make sure that direct API users didn't make a typo (which
could lead to potentially harmful formatting), this API would also
validate that the disk ID that the user asked to skip actually appears
in the inventory. This also ensures the user doesn't choose to skip a
disk before the inventory arrives - as that could lead to the
installation disk being chosen to be one of the skipped disks, which
would be awkward.
Finally, when determining which disks the assisted-service should ask
the assisted-installer to format, the assisted-service will consult both
disks_to_be_formattedandskip_formatting_disksto decide on thefinal list (the final list is a simple
set(disks_to_be_formatted)minus
set(skip_formatting_disks))Installation disk
I've also added a blocking validation to hosts which checks that the
installation disk of the host does not appear in the host's
skip_formatting_diskslist. This validation would be the simplest wayto block the user from asking to "skip the formatting of the
installation disk" (which would obviously be non-sensical, as the
installation disk has to be formatted). Another way to solve this would
be to block the user during the host UPDATE API call, but that would be
mixing / tightly coupling the installation disk choice API with the new
skip formatting disks API. An "offline" validation happening in the
background is simpler.
Missing skip-formatting disks validation
If for some reason one of the disks that is set to be skipped no longer
appears in the inventory, a new validation will block installation. This
is to ensure that if the user reboots the system and somehow the disk
they asked to skip the formatting of changed ID, then they would have to
manually and consciously remove it from the
skip_formatting_diskslist.List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Manual API interaction, unit-tests
Assignees
/cc @
/cc @
Checklist
docs, README, etc)Reviewers Checklist