Remove linkage to cloud providers in persistent volume label controller

[rrati]

/kind feature

#44680 introduced the persistent volume label controller to the cloud-controller-manager. The first revision of the controller was basically a transplant of the admission controller, but to truly function as is intended the linkage to the EC2 and GCE volumes needs to be removed.

[k8s-github-robot]

@rrati
There are no sig labels on this issue. Please add a sig label by:

1. mentioning a sig: @kubernetes/sig-<group-name>-<group-suffix>
   e.g., @kubernetes/sig-contributor-experience-<group-suffix> to notify the contributor experience sig, OR

2. specifying the label manually: /sig <label>
   e.g., /sig scalability to apply the sig/scalability label

Note: Method 1 will trigger an email to the group. You can find the group list here and label list here.
The <group-suffix> in the method 1 has to be replaced with one of these: bugs, feature-requests, pr-reviews, test-failures, proposals

[luxas]

@cheftako @rrati @wlan0 Should we fix this in time for v1.8?

That would probably make sense to me; current state is kind of a bug, but let's ask @jdumars as well if he thinks that would be a bug fix or not

[jdumars]

I would lean toward this being a bug as well. ETA on possibly fixing this?

[cheftako]

1.8 code freezes today. I'll defer to @wlan0 but my guess is we aren't getting a fix that quickly. We definitely should fix it for 1.9.

[wlan0]

Please punt it to 1.9.

How do we approach this @rrati? Would we add an interface in the cloudprovider for volume info?

Should this volume info interface, if we implement, pave the way for creating out-of-tree EBS/PD controllers in the future? Please advise @thockin @luxas

[luxas]

@wlan0 I think doing this in the most simple way right now is just fine; i.e. moving the imports from the generic package into an other cloud-specific package; and then passing a cloud implementation struct to the generic code taking a generic interface makes a lot of sense, and is very low-volume code-wise and risk-wise. This would remove the hard-coding we have right now

That is; move this code

func (pvlc *PersistentVolumeLabelController) findAWSEBSLabels(volume *v1.PersistentVolume) (map[string]string, error) {
	// Ignore any volumes that are being provisioned
	if volume.Spec.AWSElasticBlockStore.VolumeID == vol.ProvisionedVolumeName {
		return nil, nil
	}
	ebsVolumes, err := pvlc.getEBSVolumes()
	if err != nil {
		return nil, err
	}

	// TODO: GetVolumeLabels is actually a method on the Volumes interface
	// If that gets standardized we can refactor to reduce code duplication
	spec := aws.KubernetesVolumeID(volume.Spec.AWSElasticBlockStore.VolumeID)
	labels, err := ebsVolumes.GetVolumeLabels(spec)
	if err != nil {
		return nil, err
	}

	return labels, nil
}

// getEBSVolumes returns the AWS Volumes interface for ebs
func (pvlc *PersistentVolumeLabelController) getEBSVolumes() (aws.Volumes, error) {
	pvlc.mutex.Lock()
	defer pvlc.mutex.Unlock()

	if pvlc.ebsVolumes == nil {
		awsCloudProvider := pvlc.cloud.(*aws.Cloud)
		awsCloudProvider, ok := pvlc.cloud.(*aws.Cloud)
		if !ok {
			// GetCloudProvider has gone very wrong
			return nil, fmt.Errorf("error retrieving AWS cloud provider")
		}
		pvlc.ebsVolumes = awsCloudProvider
	}
	return pvlc.ebsVolumes, nil
}

func (pvlc *PersistentVolumeLabelController) findGCEPDLabels(volume *v1.PersistentVolume) (map[string]string, error) {
	// Ignore any volumes that are being provisioned
	if volume.Spec.GCEPersistentDisk.PDName == vol.ProvisionedVolumeName {
		return nil, nil
	}

	provider, err := pvlc.getGCECloudProvider()
	if err != nil {
		return nil, err
	}

	// If the zone is already labeled, honor the hint
	zone := volume.Labels[kubeletapis.LabelZoneFailureDomain]

	labels, err := provider.GetAutoLabelsForPD(volume.Spec.GCEPersistentDisk.PDName, zone)
	if err != nil {
		return nil, err
	}

	return labels, nil
}

// getGCECloudProvider returns the GCE cloud provider, for use for querying volume labels
func (pvlc *PersistentVolumeLabelController) getGCECloudProvider() (*gce.GCECloud, error) {
	pvlc.mutex.Lock()
	defer pvlc.mutex.Unlock()

	if pvlc.gceCloudProvider == nil {
		gceCloudProvider, ok := pvlc.cloud.(*gce.GCECloud)
		if !ok {
			// GetCloudProvider has gone very wrong
			return nil, fmt.Errorf("error retrieving GCE cloud provider")
		}
		pvlc.gceCloudProvider = gceCloudProvider
	}
	return pvlc.gceCloudProvider, nil
}

into a sub-package or something specific for aws and gce (in separate dirs, but)

Create an interface something like this:

type PVLabeler interface {
    GetLabelsForVolume(pv *v1.PersistentVolume) (map[string]string, error)
}

that the generic controller then consumes; instead of

func (pvlc *PersistentVolumeLabelController) addLabelsToVolume(vol *v1.PersistentVolume) error {
	var volumeLabels map[string]string

	// Only add labels if in the list of initializers
	if needsInitialization(vol.Initializers, initializerName) {
		if vol.Spec.AWSElasticBlockStore != nil {
			labels, err := pvlc.findAWSEBSLabels(vol)
			if err != nil {
				return fmt.Errorf("error querying AWS EBS volume %s: %v", vol.Spec.AWSElasticBlockStore.VolumeID, err)
			}
			volumeLabels = labels
		}
		if vol.Spec.GCEPersistentDisk != nil {
			labels, err := pvlc.findGCEPDLabels(vol)
			if err != nil {
				return fmt.Errorf("error querying GCE PD volume %s: %v", vol.Spec.GCEPersistentDisk.PDName, err)
			}
			volumeLabels = labels
		}
		return pvlc.updateVolume(vol, volumeLabels)
	}

	return nil
}

we'd have something like

func (pvlc *PersistentVolumeLabelController) addLabelsToVolume(vol *v1.PersistentVolume) error {

	// Only add labels if in the list of initializers
	if needsInitialization(vol.Initializers, initializerName) {
		volumeLabels, err := pvlc.GetLabelsForVolume(vol)
		if err != nil {
			return fmt.Errorf("error querying cloud provider for volume labels: %v", err)
		}

		return pvlc.updateVolume(vol, volumeLabels)
	}

	return nil
}

which would be way cleaner.

@rrati could you please send a PR for this quickly as a bugfix?
As a side effect, any external cloud provider can easily make their own impl of the PVLabeler interface and pass that to the new controller.

[k8s-github-robot]

[MILESTONENOTIFIER] Milestone Labels Complete

@rrati

Issue label settings:

• sig/cluster-lifecycle: Issue will be escalated to these SIGs if needed.
• priority/important-longterm: Escalate to the issue owners; move out of the milestone after 1 attempt.
• kind/bug: Fixes a bug discovered during the current release.

Additional instructions available here The commands available for adding these labels are documented here

[karataliu]

+1 for fixing this.

Verified against v1.9.0-alpha.0, standalone cloud controller manager built for any cloud will have to depend on aws/gce sdk, which does not make sense.

[dims]

Will help fix one of the problems mentioned in #49402

[luxas]

@dims can I assign you this and so we can get this bugfix into v1.8?
Otherwise all ccms will depend on GCE and AWS sdks as @karataliu said...

[dims]

@luxas, am travelling all week next week (OpenStack meeting in Denver), so i won't be able to work this problem. If i get some time to look, i will assign it to myself.

[luxas]

ok, thanks anyway

[dims]

@luxas trying a tactical fix, dunno if it will work, please take a look

[rrati]

@luxas Sorry, I seem to have missed the updates to the this issue. I agree with your proposed implementation. That was along the lines I was thinking of too.