[profile-controller] Move away from using namePrefix in kustomization

[zijianjoy]

Higher KF 1.3 distribution issue: kubeflow/manifests#1798

Currently profile controller uses profiles- in its namePrefix to apply this prefix to all resources. See https://github.com/kubeflow/manifests/blob/master/apps/profiles/upstream/default/kustomization.yaml#L9.

It creates difficulty and confusion to downstream kustomization, because the downstream patches will need to use the name without prefix while patching and using such resource, but the generated resource has name with prefix. The namePrefix notation is hidden in upstream manifest, and isolated from the resource definition, which creates difficulty when we want to search manifest using the complete resource name.

Example

Patch: See https://github.com/zijianjoy/gcp-blueprints/blob/match-upstream-share/apps/profiles/kustomization.yaml#L56. I want to replace the profiles-config configMap, but I need to call it config instead. Because the upstream has profiles- as namePrefix.

Usage: Same as Patch, but it is even harder to locate the target resource because the usage itself is not kustomization.yaml, but namePrefix is in ancestor's kustomization.yaml, see PR: zijianjoy/gcp-blueprints@ef32634

Solution

Move away from using namePrefix, or apply profiles- prefix manually to all resources in the profile-controller manifests. Clear naming helps with downstream kustomization and resource keyword searching.

cc @yanniszark @Bobgy

[Bobgy]

Shall we move this issue to kubeflow/kubeflow repo, because profile controller code is there?

but anyway, I feel namePrefix is generally bad, will manifest WG be interested in enforcing everyone to not use it.
/cc @kubeflow/wg-manifests-leads

[zijianjoy]

It makes sense, let me move this issue to kubeflow/kubeflow repo.

I agree that namePrefix is generally creating difficulty in terms of down stream kustomization. However, the scope of the issue will become too big if we want to fix everything https://github.com/kubeflow/manifests/search?p=1&q=namePrefix in KF 1.3 release. Therefore we will focus our scope to blocking item (profile controller) in this issue. And following up with other working group later on.

[thesuperzapper]

Tagging the leads of Notebooks (who own profile controller) @kubeflow/wg-notebooks-leads

[yanniszark]

@zijianjoy @Bobgy thanks for the report! I tried to reproduce such an issue locally, but couldn't.
Here is what I did:

• I tried prefixing a Pod and then patching it with the prefixed name. It worked.
• I tried creating a ConfigMap with the ConfigMapGenerator and then merging it with another ConfigMapGenerator in an overlay, by referring to the prefixed name. It also worked.

Here is my example: https://github.com/yanniszark/tmp/tree/feature-kustomize-nameprefix
I tested with kustomize 3.2.0 nad 4.0.5. Can you please provide a minimal example showcasing the problem?

[zijianjoy]

Thank you @yanniszark for trying the patching! This issue happens under our existing profile manifest structure, so it doesn't necessary happen in a simple case. I would like to share minimal example with explanation about scenario of this issue.

You can pull the minimal example here:
https://github.com/zijianjoy/tmp/tree/jamxl-profiles/profiles

Command to build (kustomize v4):
kustomize build --load-restrictor LoadRestrictionsNone -o .build/ profiles

Error:
Error: merging from generator &{0xc000dd62d0 { map[] map[]} {{ profiles-config replace {[admin= gcp-sa=] [] [] } <nil>}}}: id resid.ResId{Gvk:resid.Gvk{Group:"", Version:"v1", Kind:"ConfigMap"}, Name:"profiles-config", Namespace:""} does not exist; cannot merge or replace

If I change https://github.com/zijianjoy/tmp/blob/jamxl-profiles/profiles/kustomization.yaml#L19 from profiles-config to config, the issue is gone.

Explanation

My guess is that the namespaces are different in multiple overlay levels (kubeflow vs profiles-system namespace), which cause kustomize to act strangely to find the configMap target with namePrefix.

Suggestion

The fact that we need a minimal version to debug this issue also indicates that current profile manifest is complicated to debug and kustomize. Also note from the issue description that having namePrefix increase complexity to search the target resource in IDE, not to mention there are different namespaces in the overlay. Therefore avoid using namePrefix is beneficial.

[davidspek]

Just wanted to mention that namePrefix is also used in the manifests for the Jupyter Web App here, and the notebook-controller here.

This indicates that there are 2 situations that would need addressing if namePrefixs should be removed from the manifests:

1. Remove namePrefix from manifests that belong to web apps or the central dashboard
2. Any namePrefix in the manifests of controllers are likely generated by KubeBuilder. If that is the case, the appropriate flag will need to be set so KubeBuilder doesn't add the namePrefix to the manifests it generates.

I'm willing to put in the effort to do this if we agree on the solution.

[yanniszark]

@zijianjoy thanks for providing an example! I will try to reproduce. For 1.3, I don't think it's a blocking issue, would you agree?
The first thing I'd like to do is to run your example, confirm the unexpected behavior and then open a good upstream issue.
I'd prefer to get upstream's opinion on this, as Kubeflow is a huge use case for kustomize, so I think that having good communication will help both projects tremendously.

So in total two things:

• Agree that this is not a 1.3 blocking issue.
• Agree on the next actions, in order to do this the right way (minimal reproducer, explain why it hinders our workflows, let upstream kustomize know)

[zijianjoy]

Thank you Yannis! I agree it is not a blocking issue for Kubeflow 1.3. We should follow up on the right behavior later on.

[jbottum]

/priority p2
/area manifests
/platform gcp

[google-oss-robot]

@jbottum: The label(s) area/manifests cannot be applied, because the repository doesn't have them.

In response to this:

/priority p2
/area manifests
/platform gcp

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 kubernetes/test-infra repository.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[andreyvelich]

/transfer dashboard