Generating label names

[brybacki]

What this PR does / why we need it:

Removes unused labels holding resource names. If any label is used then its name will be shortened by provided function in package naming.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Enhancement: DataVolume names are no longer restricted to 55 characters. The limit is determined by the runtime now.

[coveralls]

Coverage increased (+0.2%) to 69.597% when pulling 6b8bc90 on brybacki:generating-label-names into b98e0a8 on kubevirt:master.

[awels]

In general looks good, but can we think of any functional tests that we can do here? I don't want to merge again and having to add functional tests after the fact.

[brybacki]

Datavolume webhook still guards names (len <=55) so I can only test import using pvc for this PR.
But when #1201 is finished, we can decide if we want to lift the name length limit, and follow with tests for import/upload/clone (to test if pod/pvc/label/service names are ok)

[awels]

/test pull-containerized-data-importer-e2e-k8s-1.17-ceph

[awels]

/retest

[awels]

/retest

[brybacki]

/retest

[awels]

/retest

[awels]

/retest

[awels]

Looks like a unit test failure: https://travis-ci.org/github/kubevirt/containerized-data-importer/jobs/689329082#L7732

[brybacki]

pushed 'Focused' test by mistake, and now after turning off the focus I have found a problem to fix

[awels]

Some questions, in particular why the annotation to store/retrieve the pod name? Is the call to the naming function not deterministic based on the pvc name?

[awels]

This one really needed? Looks like we create it above when we determine that we need scratch space.

[brybacki]

see my last comment on storing names in annotations

[brybacki]

actually not, we create scratch name once, and then it is always stored in a Volume definition on the pod

[brybacki]

so I have removed this annotation, as it is not needed, and change to read from pod spec, if available

[awels]

Not sure what you mean by this comment.

[brybacki]

if r.client.Get does not wok (service error, temporary network error) then we go straight to check if pod is controlled by pvc, and the only error we can see is Pod is not owned by PVC.

	if err := r.client.Get(context.TODO(), types.NamespacedName{Name: podName, Namespace: pvc.Namespace}, pod); err != nil {
		if !k8serrors.IsNotFound(err) {
			return nil, errors.Wrapf(err, "error getting import pod %s/%s", pvc.Namespace, podName)
		}
		return nil, nil
	}

	if !metav1.IsControlledBy(pod, pvc) {
		return nil, errors.Errorf("%s pod not controlled by pvc %s", podName, pvc.Name)
	}

[brybacki]

I have actually applied the change from previous comment

[awels]

This is more a stylistic thing than a code problem, but I would write this as:

if _, ok := pvc.Annotations[AnnImportPod]; ok {
...
} else {
...
}

Second, what is the difference between initPvcPodName vs createImporterPod. I see initPvcPodName is creating the name and adding it to the annotations, but can't we do that in createImporterPod as well? I am trying to understand why this is split.

[brybacki]

This was started with a plan to change name generation logic, but now as we can see the name generation is backwards compatible, so this does not make sense/does not help, I can simplify

[awels]

So I am a little confused at this point, this function is called always with the pvc name, and the result is stored in the annotation. But why are we doing that? This should be a deterministic result based on the pvc name, so instead of storing and reading from the annotation, why not simply call this function whenever we are interested in finding the pod name based on the pvc name?

[brybacki]

So you are right, our function is now deterministic, and currently storing in annotation does not help us. So it has no value now, but might help us in the future:

• if we decide to resolve duplicate names/conflicts by trying some other name.
• if we decide to make name generation more random/unique.
• if we decide to 'change' the naming method, we might use annotation for backwards compatibility.

Now after you comment, I think, this "store in annotation" commit", and the next one, are nice to have, but really are maybe over engineering for current state.

I mean - keeping it here, and improving some tests might be beneficial (for the future needs?), but removing it - might simplify the code. So I am open for hearing arguments for one of this directions. Use annotations to store names, or do not (since naming is deterministic)

@mhenriks @awels What do you think?

[awels]

/lgtm
/approve

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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:

• OWNERS [awels]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[awels]

/test pull-containerized-data-importer-e2e-os-3.11.0-crio