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.

Sign up for GitHub

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

Data volume source image #751

Merged
merged 7 commits into from
Jun 20, 2024
Merged

Data volume source image #751

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fd1e856
Support worker config volume source image
hebelsan Apr 30, 2024
c00c822
Make generate
hebelsan Apr 30, 2024
648306b
Add docu for worker config sourceImage
hebelsan Apr 30, 2024
43cf20d
Make generate
hebelsan Apr 30, 2024
9802b76
Move imageSource to DataVolume config
hebelsan May 10, 2024
bfc9b2e
Adjust docu
hebelsan May 10, 2024
3dccc37
Use slice.ContainsFunc in validation
hebelsan Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions docs/usage/usage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ The worker configuration contains:

If you attach the disk with `SCRATCH` type, either an `NVMe` interface or a `SCSI` interface must be specified.
It is only meaningful to provide this volume interface if only `SCRATCH` data volumes are used.

* Volume Encryption config that specifies values for `kmsKeyName` and `kmsKeyServiceAccountName`.
* The `kmsKeyName` is the
key name of the cloud kms disk encryption key and must be specified if CMEK disk encryption is needed.
Expand All @@ -166,6 +167,12 @@ The worker configuration contains:
gcloud projects add-iam-policy-binding projectId --member
serviceAccount:[email protected] --role roles/cloudkms.cryptoKeyEncrypterDecrypter
```

* Setting a volume image with `dataVolume.sourceImage`.
However, this parameter should only be used with particular caution.
For example Gardenlinux works with filesystem LABELs only and creating another disk form the very same image causes the LABELs to be duplicated.
See: https://github.com/gardener/gardener-extension-provider-gcp/issues/323

* Service Account with their specified scopes, authorized for this worker.

Service accounts created in advance that generate access tokens that can be accessed through the metadata server and used to authenticate applications on the instance.
Expand Down
2 changes: 2 additions & 0 deletions example/30-worker.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ spec:
# encryption: # Encryption Details
# kmsKeyName: "projects/projectId/locations/<zoneName>/keyRings/<keyRingName>/cryptoKeys/alpha"
# kmsKeyServiceAccount: "[email protected]"
# dataVolume: # provider specific dataVolume configuration
# sourceImage: "projects/sap-se-gcp-gardenlinux/global/images/..."
# gpu:
# acceleratorType: nvidia-tesla-t4
# count: 1
Expand Down
59 changes: 59 additions & 0 deletions hack/api-reference/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,20 @@ Volume
</tr>
<tr>
<td>
<code>dataVolumes</code></br>
<em>
<a href="#gcp.provider.extensions.gardener.cloud/v1alpha1.DataVolume">
[]DataVolume
</a>
</em>
</td>
<td>
<em>(Optional)</em>
<p>DataVolumes contains configuration for the additional disks attached to VMs.</p>
</td>
</tr>
<tr>
<td>
<code>minCpuPlatform</code></br>
<em>
string
Expand Down Expand Up @@ -487,6 +501,51 @@ string
</tr>
</tbody>
</table>
<h3 id="gcp.provider.extensions.gardener.cloud/v1alpha1.DataVolume">DataVolume
</h3>
<p>
(<em>Appears on:</em>
<a href="#gcp.provider.extensions.gardener.cloud/v1alpha1.WorkerConfig">WorkerConfig</a>)
</p>
<p>
<p>DataVolume contains configuration for data volumes attached to VMs.</p>
</p>
<table>
<thead>
<tr>
<th>Field</th>
<th>Description</th>
</tr>
</thead>
<tbody>
<tr>
<td>
<code>name</code></br>
<em>
string
</em>
</td>
<td>
<p>Name is the name of the data volume this configuration applies to.</p>
</td>
</tr>
<tr>
<td>
<code>sourceImage</code></br>
<em>
string
</em>
</td>
<td>
<p>SourceImage is the image to create this disk
However, this parameter should only be used with particular caution.
For example GardenLinux works with filesystem LABELs only and creating
another disk form the very same image causes the LABELs to be duplicated.
See: <a href="https://github.com/gardener/gardener-extension-provider-gcp/issues/323">https://github.com/gardener/gardener-extension-provider-gcp/issues/323</a></p>
</td>
</tr>
</tbody>
</table>
<h3 id="gcp.provider.extensions.gardener.cloud/v1alpha1.DiskEncryption">DiskEncryption
</h3>
<p>
Expand Down
17 changes: 17 additions & 0 deletions pkg/apis/gcp/types_worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ type WorkerConfig struct {
// Volume contains configuration for the root disks attached to VMs.
Volume *Volume

// DataVolumes contains configuration for the additional disks attached to VMs.
// +optional
DataVolumes []DataVolume

// MinCpuPlatform is the name of the minimum CPU platform that is to be
// requested for the VM.
MinCpuPlatform *string
Expand All @@ -46,6 +50,19 @@ type Volume struct {
Encryption *DiskEncryption
}

// DataVolume contains configuration for data volumes attached to VMs.
type DataVolume struct {
// Name is the name of the data volume this configuration applies to.
Name string

// SourceImage is the image to create this disk
// However, this parameter should only be used with particular caution.
// For example GardenLinux works with filesystem LABELs only and creating
// another disk form the very same image causes the LABELs to be duplicated.
// See: https://github.com/gardener/gardener-extension-provider-gcp/issues/323
SourceImage *string
}

// DiskEncryption encapsulates the encryption configuration for a disk.
type DiskEncryption struct {
// KmsKeyName specifies the customer-managed encryption key (CMEK) used for encryption of the volume.
Expand Down
17 changes: 17 additions & 0 deletions pkg/apis/gcp/v1alpha1/types_worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ type WorkerConfig struct {
// +optional
Volume *Volume `json:"volume,omitempty"`

// DataVolumes contains configuration for the additional disks attached to VMs.
// +optional
DataVolumes []DataVolume `json:"dataVolumes,omitempty"`

// MinCpuPlatform is the name of the minimum CPU platform that is to be
// requested for the VM.
MinCpuPlatform *string `json:"minCpuPlatform,omitempty"`
Expand Down Expand Up @@ -52,6 +56,19 @@ type Volume struct {
Encryption *DiskEncryption `json:"encryption,omitempty"`
}

// DataVolume contains configuration for data volumes attached to VMs.
type DataVolume struct {
// Name is the name of the data volume this configuration applies to.
Name string `json:"name"`

// SourceImage is the image to create this disk
// However, this parameter should only be used with particular caution.
// For example GardenLinux works with filesystem LABELs only and creating
// another disk form the very same image causes the LABELs to be duplicated.
// See: https://github.com/gardener/gardener-extension-provider-gcp/issues/323
SourceImage *string `json:"sourceImage,omitempty"`
}

// DiskEncryption encapsulates the encryption configuration for a disk.
type DiskEncryption struct {
// KmsKeyName specifies the customer-managed encryption key (CMEK) used for encryption of the volume.
Expand Down
34 changes: 34 additions & 0 deletions pkg/apis/gcp/v1alpha1/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions pkg/apis/gcp/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

51 changes: 44 additions & 7 deletions pkg/apis/gcp/validation/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/gardener/gardener/pkg/apis/core"
extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1"
"golang.org/x/exp/slices"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/apimachinery/pkg/util/sets"
Expand All @@ -22,8 +23,9 @@ import (
var (
validVolumeLocalSSDInterfacesTypes = sets.New("NVME", "SCSI")

providerFldPath = field.NewPath("providerConfig")
volumeFldPath = providerFldPath.Child("volume")
providerFldPath = field.NewPath("providerConfig")
volumeFldPath = providerFldPath.Child("volume")
dataVolumeFldPath = providerFldPath.Child("dataVolume")
)

// ValidateWorkerConfig validates a WorkerConfig object.
Expand All @@ -42,6 +44,9 @@ func ValidateWorkerConfig(workerConfig *gcp.WorkerConfig, dataVolumes []core.Dat
allErrs = append(allErrs, validateDiskEncryption(workerConfig.Volume.Encryption, volumeFldPath.Child("encryption"))...)
}
allErrs = append(allErrs, validateNodeTemplate(workerConfig.NodeTemplate, providerFldPath.Child("nodeTemplate"))...)
if workerConfig.DataVolumes != nil {
allErrs = append(allErrs, validateDataVolumeNames(workerConfig.DataVolumes, dataVolumes)...)
}
}

return allErrs
Expand Down Expand Up @@ -120,22 +125,54 @@ func validateDataVolume(workerConfig *gcp.WorkerConfig, volume core.DataVolume,
allErrs = append(allErrs, field.Required(fldPath.Child("type"), "must not be empty"))
return allErrs
}
if *volume.Type == worker.VolumeTypeScratch {

allErrs = append(allErrs, validateScratchDisk(*volume.Type, workerConfig)...)

return allErrs
}

func validateScratchDisk(volumeType string, workerConfig *gcp.WorkerConfig) field.ErrorList {
Copy link
Contributor

@elankath elankath Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(apologies for delay in reviewing. was on sick leave.)

Just for confirmation: I assume this new validation routine was introduced since we didn't any validation before to enforce this section in the usage document:

"If you attach the disk with SCRATCH type, either an NVMe interface or a SCSI interface must be specified. It is only meaningful to provide this volume interface if only SCRATCH data volumes are used."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, the validation for the SCRATCH disk type was present and has now been slightly refined.
A new validation rule requires that the workerConfigDataVolume name must correspond to a dataVolume name.

allErrs := field.ErrorList{}

interfacePath := volumeFldPath.Child("interface")
encryptionPath := volumeFldPath.Child("encryption")

if volumeType == worker.VolumeTypeScratch {
if workerConfig == nil || workerConfig.Volume == nil || workerConfig.Volume.LocalSSDInterface == nil {
allErrs = append(allErrs, field.Required(volumeFldPath.Child("interface"), fmt.Sprintf("must be set when using %s volumes", worker.VolumeTypeScratch)))
allErrs = append(allErrs, field.Required(interfacePath, fmt.Sprintf("must be set when using %s volumes", worker.VolumeTypeScratch)))
} else {
if !validVolumeLocalSSDInterfacesTypes.Has(*workerConfig.Volume.LocalSSDInterface) {
allErrs = append(allErrs, field.NotSupported(volumeFldPath.Child("interface"), *workerConfig.Volume.LocalSSDInterface, validVolumeLocalSSDInterfacesTypes.UnsortedList()))
allErrs = append(allErrs, field.NotSupported(interfacePath, *workerConfig.Volume.LocalSSDInterface, validVolumeLocalSSDInterfacesTypes.UnsortedList()))
}
}
// DiskEncryption not allowed for type SCRATCH
if workerConfig != nil && workerConfig.Volume != nil && workerConfig.Volume.Encryption != nil {
allErrs = append(allErrs, field.Invalid(volumeFldPath.Child("encryption"), *workerConfig.Volume.Encryption, fmt.Sprintf("must not be set in combination with %s volumes", worker.VolumeTypeScratch)))
allErrs = append(allErrs, field.Invalid(encryptionPath, *workerConfig.Volume.Encryption, fmt.Sprintf("must not be set in combination with %s volumes", worker.VolumeTypeScratch)))
}
} else {
// LocalSSDInterface only allowed for type SCRATCH
if workerConfig != nil && workerConfig.Volume != nil && workerConfig.Volume.LocalSSDInterface != nil {
allErrs = append(allErrs, field.Invalid(volumeFldPath.Child("interface"), *workerConfig.Volume.LocalSSDInterface, fmt.Sprintf("is only allowed for type %s", worker.VolumeTypeScratch)))
allErrs = append(allErrs, field.Invalid(encryptionPath, *workerConfig.Volume.LocalSSDInterface, fmt.Sprintf("is only allowed for type %s", worker.VolumeTypeScratch)))
}
}
return allErrs
}

func validateDataVolumeNames(workerConfigDataVolumes []gcp.DataVolume, dataVolumes []core.DataVolume) field.ErrorList {
allErrs := field.ErrorList{}

// Extracting a dataVolume names
var dataVolumeNames []string
for _, dv := range dataVolumes {
dataVolumeNames = append(dataVolumeNames, dv.Name)
}

for _, configDataVolume := range workerConfigDataVolumes {
if !slices.Contains(dataVolumeNames, configDataVolume.Name) {
Copy link
Contributor

@elankath elankath Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Minor: instead of constructing dataVolumeNames slice can shorten by just using slices.ContainsFunc

		if slices.ContainsFunc(workerConfigDataVolumes, func(volume gcp.DataVolume) bool {
			return volume.Name ==  configDataVolume.Name
		}) {
			continue
		} 
		allErrs = append(allErrs, field.Invalid(
				dataVolumeFldPath,
				configDataVolume.Name,
				fmt.Sprintf("could not find dataVolume with name %s", configDataVolume.Name)))

allErrs = append(allErrs, field.Invalid(
dataVolumeFldPath,
configDataVolume.Name,
fmt.Sprintf("could not find dataVolume with name %s", configDataVolume.Name)))
}
}

Expand Down
24 changes: 24 additions & 0 deletions pkg/apis/gcp/validation/worker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ var _ = Describe("#ValidateWorkers", func() {
},
DataVolumes: []core.DataVolume{
{
Name: "foo",
Type: ptr.To("Volume"),
VolumeSize: "30G",
},
Expand Down Expand Up @@ -280,6 +281,29 @@ var _ = Describe("#ValidateWorkers", func() {
Expect(errorList).To(BeEmpty())
})

It("should allow valid dataVolume name", func() {
errorList := validateWorkerConfig([]core.Worker{workers[0]}, &gcp.WorkerConfig{
DataVolumes: []gcp.DataVolume{{
Name: "foo",
}},
})
Expect(errorList).To(BeEmpty())
})

It("should forbid invalid dataVolume name", func() {
errorList := validateWorkerConfig([]core.Worker{workers[0]}, &gcp.WorkerConfig{
DataVolumes: []gcp.DataVolume{{
Name: "foo-invalid",
}},
})
Expect(errorList).To(ConsistOf(
PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeInvalid),
"Field": Equal("providerConfig.dataVolume"),
})),
))
})

Describe("#Volume type SCRATCH", func() {
It("should pass because worker config is configured correctly", func() {
workers[0].DataVolumes[0].Type = ptr.To(worker.VolumeTypeScratch)
Expand Down
Loading
Loading