Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kubeflow manifests and kustomize 5 #2388

Closed
kimwnasptd opened this issue Feb 10, 2023 · 14 comments
Closed

Kubeflow manifests and kustomize 5 #2388

kimwnasptd opened this issue Feb 10, 2023 · 14 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@kimwnasptd
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

kimwnasptd commented Feb 22, 2023

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link

@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.

@google-oss-prow google-oss-prow bot added the help wanted Extra attention is needed label Feb 22, 2023
@juliusvonkohout
Copy link
Member

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
Copy link

jbottum commented Feb 23, 2023

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
Copy link
Member

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
Copy link
Member Author

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

@yingding
Copy link

yingding commented Mar 31, 2023

I can confirm with Kustomize 5.0.1, the following one liner works for KF 1.7.0 manifests with my on-prem k8s

while ! kustomize build example | kubectl apply -f -; do echo "Retrying to apply resources"; sleep 10; done

@midhun1998
Copy link
Member

I was trying to install 1.8.0-rc1 with Kustomize 5.1.1 and I still see the following warnings

# Warning: 'vars' is deprecated. Please use 'replacements' instead. [EXPERIMENTAL] Run 'kustomize edit fix' to update your Kustomization automatically.
# Warning: 'patchesStrategicMerge' is deprecated. Please use 'patches' instead. 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.

Is the effort still on to remove the warning? Any helping hand needed? ✋🏻

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Oct 1, 2023

@midhun1998 we just fixed the istio and knative warnings in the last weeks. You can create a similar PR for any other components directly maintained by the manifests WG. If they are synchronized as for example KFP, you might have to fix it at the source repository. Feel free to join our manifest WG meetings to discuss such things.

@juliusvonkohout
Copy link
Member

We support Kustomize 5.2.1+ only now so lets close. If there are warnings lets create follow up PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants