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

MGMT-15491: Validate vSphere disk.EnableUUID ignoring cluster platform #5416

Conversation

eliorerz
Copy link
Contributor

@eliorerz eliorerz commented Aug 6, 2023

If disk.EnableUUID flag is not enabled on vSphere VMs in any platform, including baremetal, then no symlinks are generated in /dev/disk/by-id for attached disks.

Installing ODF via LSO or something on such a platform results in somewhat fragile installation because disks themselves could be renamed on reboot and since no permanent ids exists for disks, the PVs could become invalid.

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

/cc @eliorerz

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 6, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 6, 2023

@eliorerz: This pull request references MGMT-15491 which is a valid jira issue.

In response to this:

If disk.EnableUUID flag is not enabled on vSphere VMs in any platform, including baremetal, then no symlinks are generated in /dev/disk/by-id for attached disks.

Installing ODF via LSO or something on such a platform results in somewhat fragile installation because disks themselves could be renamed on reboot and since no permanent ids exists for disks, the PVs could become invalid.

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

/cc @eliorerz

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.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2023
@openshift-ci
Copy link

openshift-ci bot commented Aug 6, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@eliorerz
Copy link
Contributor Author

eliorerz commented Aug 6, 2023

/test ?

@openshift-ci
Copy link

openshift-ci bot commented Aug 6, 2023

@eliorerz: GitHub didn't allow me to request PR reviews from the following users: eliorerz.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

If disk.EnableUUID flag is not enabled on vSphere VMs in any platform, including baremetal, then no symlinks are generated in /dev/disk/by-id for attached disks.

Installing ODF via LSO or something on such a platform results in somewhat fragile installation because disks themselves could be renamed on reboot and since no permanent ids exists for disks, the PVs could become invalid.

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

/cc @eliorerz

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.

@openshift-ci
Copy link

openshift-ci bot commented Aug 6, 2023

@eliorerz: The following commands are available to trigger required jobs:

  • /test e2e-agent-compact-ipv4
  • /test edge-assisted-operator-catalog-publish-verify
  • /test edge-ci-index
  • /test edge-e2e-ai-operator-ztp
  • /test edge-e2e-ai-operator-ztp-sno-day2-workers
  • /test edge-e2e-ai-operator-ztp-sno-day2-workers-late-binding
  • /test edge-e2e-metal-assisted
  • /test edge-e2e-metal-assisted-4-10
  • /test edge-e2e-metal-assisted-4-11
  • /test edge-e2e-metal-assisted-4-12
  • /test edge-e2e-metal-assisted-4-9
  • /test edge-e2e-metal-assisted-cnv
  • /test edge-e2e-metal-assisted-lvm
  • /test edge-e2e-metal-assisted-odf
  • /test edge-images
  • /test edge-lint
  • /test edge-subsystem-aws
  • /test edge-subsystem-kubeapi-aws
  • /test edge-unit-test
  • /test edge-verify-generated-code
  • /test images
  • /test mce-images

The following commands are available to trigger optional jobs:

  • /test e2e-agent-ha-dualstack
  • /test e2e-agent-sno-ipv6
  • /test edge-e2e-ai-operator-ztp-3masters
  • /test edge-e2e-ai-operator-ztp-capi
  • /test edge-e2e-ai-operator-ztp-compact-day2-masters
  • /test edge-e2e-ai-operator-ztp-compact-day2-workers
  • /test edge-e2e-ai-operator-ztp-disconnected
  • /test edge-e2e-ai-operator-ztp-hypershift-zero-nodes
  • /test edge-e2e-ai-operator-ztp-multiarch-3masters-ocp-411
  • /test edge-e2e-ai-operator-ztp-multiarch-sno-ocp-411
  • /test edge-e2e-ai-operator-ztp-node-labels
  • /test edge-e2e-ai-operator-ztp-sno-day2-masters
  • /test edge-e2e-ai-operator-ztp-sno-day2-workers-ignitionoverride
  • /test edge-e2e-metal-assisted-4-13
  • /test edge-e2e-metal-assisted-4-14
  • /test edge-e2e-metal-assisted-cnv-4.14
  • /test edge-e2e-metal-assisted-day2
  • /test edge-e2e-metal-assisted-day2-arm-workers
  • /test edge-e2e-metal-assisted-day2-single-node
  • /test edge-e2e-metal-assisted-ipv4v6
  • /test edge-e2e-metal-assisted-ipv6
  • /test edge-e2e-metal-assisted-kube-api-late-binding-single-node
  • /test edge-e2e-metal-assisted-kube-api-late-unbinding-ipv4-single-node
  • /test edge-e2e-metal-assisted-kube-api-net-suite
  • /test edge-e2e-metal-assisted-mce
  • /test edge-e2e-metal-assisted-mce-4-10
  • /test edge-e2e-metal-assisted-mce-sno
  • /test edge-e2e-metal-assisted-metallb
  • /test edge-e2e-metal-assisted-none
  • /test edge-e2e-metal-assisted-odf-4-14
  • /test edge-e2e-metal-assisted-onprem
  • /test edge-e2e-metal-assisted-single-node
  • /test edge-e2e-metal-assisted-static-ip-suite
  • /test edge-e2e-metal-assisted-tang
  • /test edge-e2e-metal-assisted-tpmv2
  • /test edge-e2e-metal-assisted-upgrade-agent
  • /test edge-e2e-nutanix-assisted
  • /test edge-e2e-nutanix-assisted-2workers
  • /test edge-e2e-oci-assisted
  • /test edge-e2e-vsphere-assisted
  • /test edge-e2e-vsphere-assisted-4-12
  • /test edge-e2e-vsphere-assisted-4-13
  • /test edge-e2e-vsphere-assisted-umn
  • /test edge-push-pr-image
  • /test push-pr-image

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-openshift-assisted-service-master-e2e-agent-compact-ipv4
  • pull-ci-openshift-assisted-service-master-edge-ci-index
  • pull-ci-openshift-assisted-service-master-edge-e2e-ai-operator-ztp
  • pull-ci-openshift-assisted-service-master-edge-e2e-metal-assisted
  • pull-ci-openshift-assisted-service-master-edge-images
  • pull-ci-openshift-assisted-service-master-edge-lint
  • pull-ci-openshift-assisted-service-master-edge-subsystem-aws
  • pull-ci-openshift-assisted-service-master-edge-subsystem-kubeapi-aws
  • pull-ci-openshift-assisted-service-master-edge-unit-test
  • pull-ci-openshift-assisted-service-master-edge-verify-generated-code
  • pull-ci-openshift-assisted-service-master-images
  • pull-ci-openshift-assisted-service-master-mce-images

In response to this:

/test ?

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.

@openshift-ci openshift-ci bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 6, 2023
@eliorerz
Copy link
Contributor Author

eliorerz commented Aug 6, 2023

/test edge-unit-test edge-subsystem-aws

@romfreiman
Copy link
Contributor

Lets understand whether this is indeed the right behavior. And in case it's a blocking validation...

@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Merging #5416 (6d5b34c) into master (83852f8) will decrease coverage by 0.04%.
Report is 24 commits behind head on master.
The diff coverage is 50.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5416      +/-   ##
==========================================
- Coverage   67.69%   67.65%   -0.04%     
==========================================
  Files         226      227       +1     
  Lines       33386    33430      +44     
==========================================
+ Hits        22600    22617      +17     
- Misses       8749     8777      +28     
+ Partials     2037     2036       -1     
Files Changed Coverage Δ
internal/host/validator.go 82.22% <50.00%> (+0.13%) ⬆️

... and 13 files with indirect coverage changes

@eliorerz eliorerz force-pushed the MGMT-15491_Validate_vSphere_disk.EnableUUID_ignoring_cluster_platform branch from 4262b07 to 5c19d95 Compare August 6, 2023 18:56
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 6, 2023
@eliorerz eliorerz marked this pull request as ready for review August 6, 2023 18:56
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2023
@eliorerz
Copy link
Contributor Author

eliorerz commented Aug 6, 2023

/cc @gamli75

@openshift-ci openshift-ci bot requested a review from gamli75 August 6, 2023 18:58
@eliorerz
Copy link
Contributor Author

eliorerz commented Aug 7, 2023

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 7, 2023
@eliorerz eliorerz changed the title MGMT-15491: Validate vSphere disk.EnableUUID ignoring cluster platform [WIP] MGMT-15491: Validate vSphere disk.EnableUUID ignoring cluster platform Aug 8, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2023
@eliorerz eliorerz force-pushed the MGMT-15491_Validate_vSphere_disk.EnableUUID_ignoring_cluster_platform branch from 5c19d95 to 6d5b34c Compare August 13, 2023 06:28
@eliorerz eliorerz changed the title [WIP] MGMT-15491: Validate vSphere disk.EnableUUID ignoring cluster platform MGMT-15491: Validate vSphere disk.EnableUUID ignoring cluster platform Aug 13, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 13, 2023
@eliorerz
Copy link
Contributor Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 13, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2023
@openshift-ci
Copy link

openshift-ci bot commented Aug 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eliorerz, gamli75

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD ead6675 and 2 for PR HEAD 6d5b34c in total

@eliorerz
Copy link
Contributor Author

/override ci/prow/edge-e2e-metal-assisted

@openshift-ci
Copy link

openshift-ci bot commented Aug 13, 2023

@eliorerz: Overrode contexts on behalf of eliorerz: ci/prow/edge-e2e-metal-assisted

In response to this:

/override ci/prow/edge-e2e-metal-assisted

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.

@openshift-ci
Copy link

openshift-ci bot commented Aug 13, 2023

@eliorerz: 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.

@openshift-merge-robot openshift-merge-robot merged commit 338964f into openshift:master Aug 13, 2023
14 checks passed
CrystalChun pushed a commit to CrystalChun/assisted-service that referenced this pull request Aug 25, 2023
@eliorerz eliorerz deleted the MGMT-15491_Validate_vSphere_disk.EnableUUID_ignoring_cluster_platform branch August 30, 2023 07:01
danielerez pushed a commit to danielerez/assisted-service that referenced this pull request Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants