Data volume source image #751

hebelsan · 2024-04-30T13:45:04Z

How to categorize this PR?

/area usability
/kind enhancement
/platform gcp

What this PR does / why we need it:

Add support for specifying data volume sourceImage in the workerConfig

Which issue(s) this PR fixes:
Fixes #695

Special notes for your reviewer:

Release note:

Add support for specifying data volume sourceImage in the workerConfig

kon-angelo · 2024-05-08T12:27:54Z

lgtm

/hold additional testing

elankath · 2024-05-08T12:58:15Z

@hebelsan @kon-angelo

Just a little confused. Don't we need to add a DataVolume struct similar to what we have done in gep-aws and also add a DataVolumes field in the WorkerConfig struct ? aws gep WorkerConfig has this.

Basically now the GEP for aws and gcp now appear to have non similar behaviours.

kon-angelo · 2024-05-08T13:23:41Z

@elankath I think you are correct. The way it is setup now all dataVolumes would use the same sourceImage, which is clearly not what was intended.

kon-angelo · 2024-06-03T08:14:39Z

/needs rebase

hebelsan · 2024-06-03T09:33:44Z

@elankath Could you please review again? :)

elankath · 2024-06-14T04:54:46Z

pkg/apis/gcp/validation/worker.go

+	return allErrs
+}
+
+func validateScratchDisk(volumeType string, workerConfig *gcp.WorkerConfig) field.ErrorList {


(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."

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.

elankath · 2024-06-14T05:28:26Z

pkg/controller/worker/machines.go

@@ -296,8 +305,8 @@ func createDiskSpec(size string, boot bool, machineImage, volumeType *string, wo
 		disk["labels"] = labels
 	}

-	if machineImage != nil {
-		disk["image"] = *machineImage
+	if image != nil {


Do we need any changes in machine-controller-manager-provider-gcp or just setting image is good enough ? Just wondering why we decided to choose dataVolume.sourceImage instead of simply dataVolume.image ?

yes, nothing to change in the mcm.
we went for sourceImage because that's how it is referred to by gcp client.

elankath · 2024-06-14T12:37:52Z

pkg/apis/gcp/validation/worker.go

+	}
+
+	for _, configDataVolume := range workerConfigDataVolumes {
+		if !slices.Contains(dataVolumeNames, configDataVolume.Name) {


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)))

elankath

thanks for the PR.

kon-angelo

/lgtm

kon-angelo · 2024-06-20T08:13:51Z

/unhold

hebelsan requested review from a team as code owners April 30, 2024 13:45

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 30, 2024

gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 30, 2024

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 30, 2024

gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label May 8, 2024

gardener-robot added size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) and removed size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels May 10, 2024

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 10, 2024

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 10, 2024

hebelsan mentioned this pull request May 10, 2024

Support for hyperdisk params #743

Merged

gardener-robot added the needs/rebase Needs git rebase label Jun 3, 2024

hebelsan added 3 commits June 3, 2024 11:16

Support worker config volume source image

fd1e856

Make generate

c00c822

Add docu for worker config sourceImage

648306b

hebelsan added 3 commits June 3, 2024 11:16

Make generate

43cf20d

Move imageSource to DataVolume config

9802b76

Adjust docu

bfc9b2e

hebelsan force-pushed the data-vol-image branch from 23be46b to bfc9b2e Compare June 3, 2024 09:22

gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 3, 2024

elankath reviewed Jun 14, 2024

View reviewed changes

elankath approved these changes Jun 14, 2024

View reviewed changes

Use slice.ContainsFunc in validation

3dccc37

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 14, 2024

kon-angelo approved these changes Jun 18, 2024

View reviewed changes

gardener-robot added reviewed/lgtm Has approval for merging and removed needs/rebase Needs git rebase needs/review Needs review needs/second-opinion Needs second review by someone else labels Jun 18, 2024

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 18, 2024

gardener-robot removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Jun 20, 2024

kon-angelo merged commit 68d51c8 into gardener:master Jun 20, 2024
15 checks passed

gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jun 20, 2024

hebelsan deleted the data-vol-image branch June 26, 2024 07:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data volume source image #751

Data volume source image #751

hebelsan commented Apr 30, 2024

kon-angelo commented May 8, 2024

elankath commented May 8, 2024

kon-angelo commented May 8, 2024 •

edited

Loading

kon-angelo commented Jun 3, 2024

hebelsan commented Jun 3, 2024

elankath Jun 14, 2024 •

edited

Loading

hebelsan Jun 14, 2024

elankath Jun 14, 2024

hebelsan Jun 14, 2024

elankath Jun 14, 2024 •

edited

Loading

elankath left a comment

kon-angelo left a comment

kon-angelo commented Jun 20, 2024

Data volume source image #751

Data volume source image #751

Conversation

hebelsan commented Apr 30, 2024

kon-angelo commented May 8, 2024

elankath commented May 8, 2024

kon-angelo commented May 8, 2024 • edited Loading

kon-angelo commented Jun 3, 2024

hebelsan commented Jun 3, 2024

elankath Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

hebelsan Jun 14, 2024

Choose a reason for hiding this comment

elankath Jun 14, 2024

Choose a reason for hiding this comment

hebelsan Jun 14, 2024

Choose a reason for hiding this comment

elankath Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

elankath left a comment

Choose a reason for hiding this comment

kon-angelo left a comment

Choose a reason for hiding this comment

kon-angelo commented Jun 20, 2024

kon-angelo commented May 8, 2024 •

edited

Loading

elankath Jun 14, 2024 •

edited

Loading

elankath Jun 14, 2024 •

edited

Loading