Kubeflow manifests and kustomize 5

[kimwnasptd]

Kustomize 5 has just been released, with the fix we wanted for ordering kubernetes-sigs/kustomize#3794 #1797.

Will use this issue to expose the current status of installing the Kubeflow manifests with Kustomize 5.

/cc @DomFleischmann @annajung @juliusvonkohout

[kimwnasptd]

The situation with Kustomize 5 is that:

1. The manifests can be built, and we can avoid the ordering issue with the fix from Kustomize resource ordering regression kubernetes-sigs/kustomize#3794
2. We can't use the same kustomization.yaml for both kustomize 5 and earlier versions, since the sortOptions field is supported only by Kustomize 5
3. Because of a regression in Kustomize 5 we'll need to slightly adjust some of our commands to work around it Well-defined vars never replaced changed output stream in 5.0.0 kubernetes-sigs/kustomize#5039
4. While the warnings we see are not blockers, for now, we'll need to some manifests work to mitigate them

I'll expose more technical details in the following messages, but I propose that we do the switch and just go with Kustomize 5

[kimwnasptd]

Specifically, if we try to build the example we'll get the following warnings (multiple times):

# Warning: 'patchesStrategicMerge' is deprecated. Please use 'patches' instead. Run 'kustomize edit fix' to update your Kustomization automatically.
# Warning: 'vars' is deprecated. Please use 'replacements' instead. [EXPERIMENTAL] Run 'kustomize edit fix' to update your Kustomization automatically.
# Warning: 'bases' is deprecated. Please use 'resources' instead. Run 'kustomize edit fix' to update your Kustomization automatically.
# Warning: 'patchesJson6902' is deprecated. Please use 'patches' instead. Run 'kustomize edit fix' to update your Kustomization automatically.

We could try to do kustomize edit fix, but there's a good chance we'll need to do manual changes as well

[kimwnasptd]

Next up, if we'd try to apply these manifests we'll see that the generated manifests look like this:

2023/02/22 15:47:37 well-defined vars that were never replaced: kfp-app-name,kfp-app-version
apiVersion: v1
kind: Namespace
metadata:
  name: auth
...

The problem here is that the line 2023/02/22 15:47:37 well-defined vars that were never replaced: kfp-app-name,kfp-app-version should have been printed in stderr and not stdout. This is a regression in kustomize 5 kubernetes-sigs/kustomize#5039. A PR for that is merged so there's a good chance this will be resolved in a next minor patch in Kustomize.

To get around this we can just parse the output and remove such lines with something like awk '!/well-defined/'. So the one-liner would now become:

 while ! kustomize-5 build example | awk '!/well-defined/' | kubectl apply -f -; do echo "Retrying to apply resources"; sleep 10; done

[kimwnasptd]

Lastly, in order to solve the initial issue we had with ordering we'll need to use the sortOptions to modify the ordering. Specifically we'll need this section:

sortOptions:
  order: legacy
  legacySortOptions:
    orderFirst:
    - Namespace
    - ResourceQuota
    - StorageClass
    - CustomResourceDefinition
    - MutatingWebhookConfiguration
    - ServiceAccount
    - PodSecurityPolicy
    - Role
    - ClusterRole
    - RoleBinding
    - ClusterRoleBinding
    - ConfigMap
    - Secret
    - Endpoints
    - Service
    - LimitRange
    - PriorityClass
    - PersistentVolume
    - PersistentVolumeClaim
    - Deployment
    - StatefulSet
    - CronJob
    - PodDisruptionBudget
    orderLast:
    - ValidatingWebhookConfiguration

Pay close attention to the MutatingWebhookConfiguration part. This list is copy-paste from the Kustomize 3.2 code
https://github.com/kubernetes-sigs/kustomize/blob/v3.2.0/pkg/gvk/gvk.go#L82-L106, which is working as expected. A note here that I'm not 100 sure if we need to also move the ValidatingWebhookConfiguration higher in the list, but I'll leave this as is for now.

In later versions of kustomize this order was changed, and is what produced the ordering issue described in #1797
https://github.com/kubernetes-sigs/kustomize/blob/kustomize/v4.3.0/kyaml/resid/gvk.go#L136-L163

The only issue with the above though is that users can't use it with earlier versions of Kustomize, since only Kustomize 5 can recognize the sortOptions property.

[kimwnasptd]

I propose that we just go to Kustomize 5 altogether. We'll have to user the awk command for now, but I'm expecting to move away from it pretty soon (since the kustomize fix for it is merged).

So KF 1.7 will support the latest and greatest Kustomize 5.

Then from our side we'll need to do the work to update the KF manifests to remove the warnings. Any takers?

/help

[google-oss-prow]

@kimwnasptd:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

I propose that we just go to Kustomize 5 altogether. We'll have to user the awk command for now, but I'm expecting to move away from it pretty soon (since the kustomize fix for it is merged).

So KF 1.7 will support the latest and greatest Kustomize 5.

Then from our side we'll need to do the work to update the KF manifests to remove the warnings. Any takers?

/help

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.

[juliusvonkohout]

I propose that we just go to Kustomize 5 altogether. We'll have to user the awk command for now, but I'm expecting to move away from it pretty soon (since the kustomize fix for it is merged).

So KF 1.7 will support the latest and greatest Kustomize 5.

Then from our side we'll need to do the work to update the KF manifests to remove the warnings. Any takers?

/help

yes please kustomize 5.0 only. @akgraner anyone from the community outreach who could do that?

[jbottum]

Kustomize 5.0 seems like the right move, we have been on 3.2 for a while, I can put this issue on the Community Meeting Agenda. I appreciate the "help wanted" request. I suspect the qualified number of candidates will include those who have infra, which might mean that likely contributors would be at the distributions or perhaps large users. Still time is tight if we want this in 1.7.

[annajung]

Thanks Kimonas for the detailed explanation of the issues and solutions. I support the upgrade of 5.0 for the manifest repo, however, am concerned with the timing to be included in the 1.7.

This would mean that any distributions / end users would need to modify their existing kustomize version and test their custom manifest works as expected. With the issues that you have mentioned above + knowing that it is a breaking change, is there a strong reason to include it in the 1.7 when we're already in the middle of distribution testing and very close to the release date?

Would it be better to include the update in the main branch but not cherry pick for the release and give manifest wg time to fix the warning and other issues that might come up and everyone else more time to make changes needed to prepare for the breaking changes introduced by kustomize5?

[kimwnasptd]

Small update that Kustomize has released a 5.0.1 version that includes the fix for the regression discussed in #2388 (comment).

We can update the the instructions to propose that version to be used instead. But considering that we are very close to release I'm not sure on whether we'd like to do this change now. Reminder that we have the awk command (in the one-liner install) to mitigate the above regression.

cc @DomFleischmann

https://github.com/kubernetes-sigs/kustomize/releases/tag/kustomize%2Fv5.0.1