OCPBUGS-61892, OCPBUGS-59743, OCPBUGS-59520, OCPBUGS-63147: Resolve issues with azure data disk - #9947
OCPBUGS-61892, OCPBUGS-59743, OCPBUGS-59520, OCPBUGS-63147: Resolve issues with azure data disk#9947jcpowermac wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
/test ? |
|
/test e2e-azure-ovn |
|
/test unit /test e2e-azure-ovn-multidisk-techpreview |
|
/test unit /test e2e-azure-ovn-multidisk-techpreview |
jcpowermac
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@jcpowermac: This pull request references Jira Issue OCPBUGS-61892, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-59743, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-59522, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-59521, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-59520, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jcpowermac: This pull request references Jira Issue OCPBUGS-61892, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-59743, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-59522, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-59521, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-59520, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@jinyunma I think I got all the bugs that were opened for data disks. If you can take a look I would appreciate it, thanks! one issue that might have to be an rfe is machine-api doesn't support data disks and security profiles |
|
/test e2e-azure-ovn |
|
/test e2e-azure-ovn |
|
@jcpowermac I did pre-merge test with this PR, and checked each bug:
Here for worke machines also needs to be updated, else got same panic error.
looks good, installer has pre-check that data disk does not support on azure stack hub.
Seems good on compute node, installer has pre-check that security profile does not support on compute machine. Issue still can be reproduced with below install-config.yaml file issue get fixed, no more panic error. |
14b44ab to
859361e
Compare
|
|
||
| // Control plane/master machines use Cluster API which supports data disk security profiles | ||
|
|
||
| // https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/cc24323ded8947bef736f90bf73fdb7ff3bb54e4/api/v1beta1/types.go#L693 |
There was a problem hiding this comment.
@jinyunma after looking at the API docs for capz, diskEncryptionSet looks like it needs confidential vm enabled
There was a problem hiding this comment.
For non-confidential vm, diskEncryptionSet can also be applied, see the the structure of ManagedDiskParameters, two DES are defined. One is under ManagedDiskParameters, another is under ManagedDiskParameters.SecurityProfile.
I think they are two different kinds of disk encryption. And ManagedDiskParameters.SecurityProfile requires confidential vm enabled.
Here are Azure docs FYI.
service-side encryption of azure disk
Confidential OS disk encryption
This is also the same as what configures on os disk in installer now
$ ./openshift-install explain installconfig.platform.azure.defaultMachinePlatform.osDisk.diskEncryptionSet
KIND: InstallConfig
VERSION: v1
RESOURCE: <object>
DiskEncryptionSet defines a disk encryption set.
FIELDS:
name <string> -required-
Name is the name of the disk encryption set.
resourceGroup <string> -required-
ResourceGroup defines the Azure resource group used by the disk
encryption set.
subscriptionId <string> -required-
SubscriptionID defines the Azure subscription the disk encryption
set is in.
$ ./openshift-install explain installconfig.platform.azure.defaultMachinePlatform.osDisk.securityProfile.diskEncryptionSet
KIND: InstallConfig
VERSION: v1
RESOURCE: <object>
DiskEncryptionSet specifies the customer managed disk encryption set resource id for the
managed disk that is used for Customer Managed Key encrypted ConfidentialVM OS Disk and
VMGuestState blob.
FIELDS:
name <string> -required-
Name is the name of the disk encryption set.
resourceGroup <string> -required-
ResourceGroup defines the Azure resource group used by the disk
encryption set.
subscriptionId <string> -required-
SubscriptionID defines the Azure subscription the disk encryption
set is in.
There was a problem hiding this comment.
@jinyunma this is what I used...
controlPlane:
architecture: amd64
hyperthreading: Enabled
name: master
diskSetup:
- type: etcd
etcd:
platformDiskID: "etcddisk"
- type: user-defined
userDefined:
platformDiskID: "containers"
mountPath: /var/lib/containers
- type: swap
swap:
platformDiskID: "swap"
platform:
azure:
type: Standard_D4s_v5
dataDisks:
- nameSuffix: etcddisk
diskSizeGB: 32
lun: 0
- nameSuffix: containers
diskSizeGB: 128
lun: 1
managedDisk:
storageAccountType: StandardSSD_LRS
diskEncryptionSet:
id: /subscriptions/19875ffa-41a7-46ea-8714-bddf656ed287/resourceGroups/JCALLEN-RG-DISK-ENCRYPTION-SET/providers/Microsoft.Compute/diskEncryptionSets/jcallen-disk-encryption-set
- nameSuffix: swap
diskSizeGB: 128
lun: 2
replicas: 3
There was a problem hiding this comment.
Still failing, will have to troubleshoot it more tomorrow. The strange thing is bootstrap is up but MCS never starts running, so the control plane machines of course never come up.
There was a problem hiding this comment.
yeah, there is no issue with your configuration. Failed with below configured disk encryption set + mountPath: /var
controlPlane:
architecture: amd64
hyperthreading: Enabled
name: master
platform:
azure:
dataDisks:
- cachingType: ReadWrite
diskSizeGB: 128
lun: 1
nameSuffix: vardisk
managedDisk:
storageAccountType: Premium_LRS
diskEncryptionSet:
id: /subscriptions/53b8f551-f0fc-4bea-8cba-6d1fefd54c8a/resourceGroups/jima-test-rg/providers/Microsoft.Compute/diskEncryptionSets/jima-test-des
diskSetup:
- type: user-defined
userDefined:
platformDiskID: vardisk
mountPath: /var
replicas: 3
./openshift-install craete cluster --dir ipi/
[......]
INFO Waiting up to 20m0s (until 6:50AM UTC) for the Kubernetes API at https://api.jima31diska.qe.azure.devcluster.openshift.com:6443...
INFO API v1.34.1 up
INFO Waiting up to 45m0s (until 7:15AM UTC) for bootstrapping to complete...
ERROR Bootstrap failed to complete: timed out waiting for the condition
ERROR Failed to wait for bootstrapping to complete. This error usually happens when there is a problem with control plane hosts that prevents the control plane operators from creating the control plane.
I could see that master machines were running, but I am unable to ssh connection to them for debug.
There was a problem hiding this comment.
I am guessing this is the cause.
Let me test skipping this if datasetup is defined with /var
There was a problem hiding this comment.
I pushed the changes, still going to test locally
There was a problem hiding this comment.
I was able to install the cluster after this change
There was a problem hiding this comment.
The change has taken effect, the installation passed on my end too.
|
/test e2e-azure-ovn |
859361e to
f9cb5ef
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@jcpowermac: This pull request references Jira Issue OCPBUGS-61892, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-59743, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-59522, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-59521, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-59520, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
f9cb5ef to
7cc49dd
Compare
I think I am leaning towards keeping it as-is. We keep our alignment towards capz types (which we will eventually be moving to). There is validation in place so the customer isn't surprised when the policy for the guest isn't applied. It also greatly simplifies the code, we would have to create replacement types, then within the machine creation do conversion to the capz datadisk. Will fix the documentation. |
427240f to
18e0ab3
Compare
|
Retest these bugs: OCPBUGS-61892 OCPBUGS-59743 OCPBUGS-59522 OCPBUGS-59521 OCPBUGS-63147 OCPBUGS-59520 |
18e0ab3 to
ce9b962
Compare
Added validation that control plane nodes are currently unsupported for swap |
|
All bugs are verified, lgtm on QE side. /verified by jima |
openshift-ci-robot
added
the
verified
|
@jinyunma: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
ce9b962 to
d06a6d0
Compare
openshift-ci-robot
removed
the
verified
|
@jcpowermac: This pull request references Jira Issue OCPBUGS-61892, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-59743, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-59522, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-59521, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-59520, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-63147, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
d06a6d0 to
da006fc
Compare
|
/test e2e-azure-ovn |
|
/assign @tthvo |
azure: fix data disk validation and swap disk references Fix copy-paste error where swap disks incorrectly referenced etcd disk PlatformDiskID in master.go and worker.go. Fix disk encryption set assignment in machines.go to use disk.ManagedDisk.DiskEncryptionSet instead of incorrect SecurityProfile path. Add validation to block data disks on Azure Stack Cloud and enforce storage account type requirements for managed disks. Enhance confidential VM validation to check both OS and data disk security encryption types together for VMGuestStateOnly and DiskWithVMGuestState scenarios. Skip default /var partition when user-defined disk already mounts /var. Co-Authored-By: Cursor <noreply@cursor.com> Co-Authored-By: Claude <noreply@anthropic.com>
da006fc to
1a9d5a3
Compare
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
Tbh, I have little experience with Azure 😓 I just have a few code-related comments...
| return append(allErrs, field.Invalid(fldPath, azurePool.DataDisks, | ||
| fmt.Sprintf("the field dataDisks is not supported on %s.", azure.StackCloud))) |
There was a problem hiding this comment.
| return append(allErrs, field.Invalid(fldPath, azurePool.DataDisks, | |
| fmt.Sprintf("the field dataDisks is not supported on %s.", azure.StackCloud))) | |
| return append(allErrs, field.Forbidden(fldPath, | |
| fmt.Sprintf("the field dataDisks is not supported on %s", azure.StackCloud))) |
💡 nit: We can shorten the error message by using Forbidden.
<path>.dataDisks: Forbidden: the field dataDisks is not supported on AzureStackCloud
| func dataDiskContainsSecurityEncryptionType(dataDisk capz.DataDisk) bool { | ||
| if dataDisk.ManagedDisk != nil && dataDisk.ManagedDisk.SecurityProfile != nil { | ||
| if dataDisk.ManagedDisk.SecurityProfile.SecurityEncryptionType != "" { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
| func dataDiskContainsSecurityEncryptionType(dataDisk capz.DataDisk) bool { | |
| if dataDisk.ManagedDisk != nil && dataDisk.ManagedDisk.SecurityProfile != nil { | |
| if dataDisk.ManagedDisk.SecurityProfile.SecurityEncryptionType != "" { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| func dataDiskContainsSecurityEncryptionType(dataDisk capz.DataDisk) bool { | |
| return dataDisk.ManagedDisk != nil && | |
| dataDisk.ManagedDisk.SecurityProfile != nil && | |
| dataDisk.ManagedDisk.SecurityProfile.SecurityEncryptionType != "" | |
| } |
💡 nit: we can simplify it a bit...
| return "" | ||
| } | ||
|
|
||
| func validateDataDisk(p *azure.MachinePool, poolName string, fldPath *field.Path) field.ErrorList { |
There was a problem hiding this comment.
💡 I noticed there are duplicate codes for each type of machine pool. Let's refactor a bit, WDYT 👇?
func validateDataDisks(p *azure.MachinePool, poolName string, fldPath *field.Path) field.ErrorList {
var allErrs field.ErrorList
// Default machine pool platform has already been validated
// to not allow specifying dataDisks field.
if poolName == "" {
return allErrs
}
for i, dataDisk := range p.DataDisks {
if dataDiskContainsSecurityEncryptionType(dataDisk) {
allErrs = append(allErrs, field.Invalid(
fldPath.Index(i).Child("managedDisk").Child("securityProfile").Child("securityEncryptionType"),
dataDisk.ManagedDisk.SecurityProfile.SecurityEncryptionType,
"security encryption types are not supported on data disks"))
}
switch poolName {
case types.MachinePoolControlPlaneRoleName:
// Control plane/master machines use Cluster API which supports data disk security profiles
// If using a ManagedDisk the StorageAccountType must be defined
if dataDisk.ManagedDisk != nil && dataDisk.ManagedDisk.StorageAccountType == "" {
allErrs = append(allErrs, field.Required(
fldPath.Index(i).Child("managedDisk").Child("storageAccountType"),
"storageAccount type must not be empty"))
}
case types.MachinePoolComputeRoleName:
// Worker/compute machines use Machine API which does not support data disk security profiles
if dataDisk.ManagedDisk != nil && dataDisk.ManagedDisk.SecurityProfile != nil {
allErrs = append(allErrs, field.Forbidden(
fldPath.Index(i).Child("managedDisk").Child("securityProfile"),
"data disk security profiles are not supported for worker machines (Machine API limitation). Security profiles for data disks are only supported on control plane machines."))
}
}
}
return allErrs
}There was a problem hiding this comment.
❓ I have a few questions for this validation:
- Do we need to consider arbiter and edge nodes? I am not sure if these types are applicable to Azure...?
- The field
dataDisks.managedDisk.storageAccountTypeis marked as// +optional, but the validation says it should not be empty (thus, required). I am unsure if we can change the kubebuilder marker...
| // Note: All data disks should have the same security encryption type to avoid | ||
| // configuration conflicts and ensure consistent security posture. |
There was a problem hiding this comment.
❓ This comment says All data disks should have the same security encryption type, but there is no validation for it, right? Should we add it?
| switch dataDisk.ManagedDisk.SecurityProfile.SecurityEncryptionType { | ||
| case capz.SecurityEncryptionTypeDiskWithVMGuestState: | ||
| return capz.SecurityEncryptionTypeDiskWithVMGuestState | ||
| case capz.SecurityEncryptionTypeVMGuestStateOnly: | ||
| return capz.SecurityEncryptionTypeVMGuestStateOnly | ||
|
|
||
| default: | ||
| continue |
There was a problem hiding this comment.
❓ IIUC, we want to make sure to only get the valid security encryption type right (under the assumption that all have the same type)? Should we add a validation for that?
| if encryptionSetID == "" { | ||
| return nil, fmt.Errorf("data disk %s has invalid disk encryption set: empty ID", disk.NameSuffix) | ||
| } |
There was a problem hiding this comment.
💡 Should this check be performed early in machinepool static validation instead?
| if ic.Azure != nil { | ||
| varMountPath := false | ||
| if ic.ControlPlane != nil { | ||
| for _, ds := range ic.ControlPlane.DiskSetup { | ||
| if ds.Type == types.UserDefined && ds.UserDefined != nil { | ||
| if ds.UserDefined.MountPath == "/var" { | ||
| varMountPath = true | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if !varMountPath { | ||
| logrus.Debugf("Adding /var partition to skip CoreOS growfs step") | ||
| // See https://issues.redhat.com/browse/OCPBUGS-43625 | ||
| ignition.AppendVarPartition(a.Config) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Looks like we can extract the logic into a helper func for easier read...
func (a *Master) Generate(_ context.Context, dependencies asset.Parents) error {
// ...output-omitted...
if installConfig.Config.Platform.Name() == azure.Name {
if !hasUserDefinedVarMount(ic.ControlPlane) {
logrus.Debugf("Adding /var partition to skip CoreOS growfs step")
// See https://issues.redhat.com/browse/OCPBUGS-43625
ignition.AppendVarPartition(a.Config)
}
}
// ...output-omitted...
}
func hasUserDefinedVarMount(pool *types.MachinePool) bool {
if pool == nil {
return false
}
for _, ds := range pool.DiskSetup {
if ds.Type == types.UserDefined &&
ds.UserDefined != nil &&
ds.UserDefined.MountPath == "/var" {
return true
}
}
return false
}There was a problem hiding this comment.
Also, we can skip if ic.Azure != nil as the installConfig.Config.Platform.Name() == azure.Name already does that.
|
@jcpowermac: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
re-run tests to see the current state of PR as past runs have been expired: /test e2e-azure-ovn |
|
fyi we are splitting the bug fixes from this PR to smaller one so we can validate it individually as GA readiness. Those will be shown as reference/links to this PR. Eventually we'll re-title removing the ones that were already splitted: OCPBUGS-59521: azure: fixes when setting DiskEncryptionSet on data disks /retitle OCPBUGS-61892, OCPBUGS-59743, OCPBUGS-59520, OCPBUGS-63147: Resolve issues with azure data disk |
openshift-ci
Bot
added
the
do-not-merge/hold
openshift-ci
Bot
changed the title
openshift-ci-robot
added
jira/invalid-bug
|
@jcpowermac: This pull request references Jira Issue OCPBUGS-61892, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-59743, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-59520, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-63147, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
8 participants
Fix copy-paste error where swap disks incorrectly referenced etcd disk
PlatformDiskID in master.go and worker.go.
Fix disk encryption set assignment in machines.go to use
disk.ManagedDisk.DiskEncryptionSet instead of incorrect SecurityProfile path.
Add validation to block data disks on Azure Stack Cloud and enforce
storage account type requirements for managed disks.
Enhance confidential VM validation to check both OS and data disk
security encryption types together for VMGuestStateOnly and
DiskWithVMGuestState scenarios.
Skip default /var partition when user-defined disk already mounts /var.
Co-Authored-By: Cursor noreply@cursor.com
Co-Authored-By: Claude noreply@anthropic.com