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

Upgrade kustomize version (v3.8.7 to v4.5.2) to support ppc64le/s390x #5674

Closed

Conversation

gquillar
Copy link

To support 'make generate' on both ppc64le and s390x, kustomize version
needs to be upgraded to v4.5.2.

Description of the change:
Upgrade kustomize version from v3.8.7 to v4.5.2.

Motivation for the change:
Support of both ppc64le and s390x.

…390x

To support 'make generate' on both ppc64le and s390x, kustomize version
needs to be upgraded to v4.5.2.
…390x

To support 'make generate' on both ppc64le and s390x, kustomize version
needs to be upgraded to v4.5.2.

Signed-off-by: Gilles Quillard <gquillar@redhat.com>
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ansible/Helm/Golamh plugins use kustomize/v1 plugin which is defined in the Kubebuilder project. This plugin is responsible for the kustomize files and the config/ dir.

It means, that the change ought to be in Kubebuilder and not here.
therefore, I do not think that we can do this change in the kustomize/v1 since it is a breaking change see;

kubernetes-sigs/kubebuilder#2583

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2022
kind: "change"

# Is this a breaking change?
breaking: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a breaking change

@@ -35,7 +35,7 @@ import (

const (
// kustomizeVersion is the sigs.k8s.io/kustomize version to be used in the project
kustomizeVersion = "v3.8.7"
kustomizeVersion = "v4.5.2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

who should know what the kustomize version used/support is the plugin responsible to generate/scaffolding the kustomize files since its manifests need to be compatible with. Also, it does not seem to fit under the domain of resposanbility of ansible/helm/go plugins.

Who is responsible for the kustomize is the kustomize plugin.
See that we are proposing to fix it in the PR, see:

So that, we could do dp the same fix here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since that moving to kustomize v4 requires to use the new plugin kustomize/v2-alpha which is still in development (PR-2583) and will be unstable (alpha version), I think that it is better to build locally the kustomize 3.8.7 for the not supported architectures as suggested by @natasha41575 in PR-4612.

So @camilamacedo86 for ppc64le and s390x, I propose to not move to v4.5.4 but rather use a locally built kustomize v3.8.7. If you agree, I'll do the following change in internal/plugins/ansible/v1/scaffolds/internal/templates/makefile.go and internal/plugins/helm/v1/scaffolds/internal/templates/makefile.go to generate the Makefile for ansible and helm operator samples.

.PHONY: kustomize
KUSTOMIZE = $(shell pwd)/bin/kustomize
kustomize: ## Download kustomize locally if necessary.
ifeq (,$(wildcard $(KUSTOMIZE)))
ifeq (,$(shell which kustomize 2>/dev/null))
        @{ \
        set -e ;\
        mkdir -p $(dir $(KUSTOMIZE)) ;\
-       curl -sSLo - https://github.com/kubernetes-sigs/kustomize/releases/download/kustomize/{{ .KustomizeVersion }}/kustomize_{{ .KustomizeVersion }}_$(OS)_$(ARCH).tar.gz | \
-       tar xzf - -C bin/ ;\
+       $(eval arch = $(shell uname -m)) \
+       if [ "${arch}" = "ppc64le" -o "${arch}" = "s390x" ]; then \
+               $(eval tmpDir = $(shell mktemp -d)) \
+               curl -sSLo - https://api.github.com/repos/kubernetes-sigs/kustomize/tarball/kustomize/v3.8.7 | \
+               tar xzf - -C ${tmpDir} --wildcards kubernetes-sigs-kustomize-\*/kustomize ;\
+               cd ${tmpDir}/kubernetes-sigs-kustomize-*/kustomize && GOBIN=$(dir $(KUSTOMIZE)) go install . && cd - ;\
+               rm -rf ${tmpDir} ;\
+       else \
+               curl -sSLo - https://github.com/kubernetes-sigs/kustomize/releases/download/kustomize/v3.8.7/kustomize_v3.8.7_$(OS)_$(ARCH).tar.gz | \
+               tar xzf - -C bin/ ;\
+       fi \
        }
else
KUSTOMIZE = $(shell which kustomize)
endif
endif

Copy link
Contributor

@camilamacedo86 camilamacedo86 May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can change your scaffold as please you.
However, to support these archs we need to more than only be able to have kustomize to them, such as:

  • All images scaffolds by default must support them
  • For golang kubebuilder-tools which ships what is required to use env test also must support

(ihmo) we need to address all changes in Kubebuilder.
When kubebuilder be able to support then we can update SDK.
We might be able to make ansible/helm support first but it might reduce the maintainability

@@ -32,7 +32,7 @@ import (

const (
// kustomizeVersion is the sigs.k8s.io/kustomize version to be used in the project
kustomizeVersion = "v3.8.7"
kustomizeVersion = "v4.5.2"
Copy link
Contributor

@camilamacedo86 camilamacedo86 May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(imo) We need to change add to Kubebuilder : kubernetes-sigs/kubebuilder#2583.
After that, we will need to check if we can or not upgrade the current ansible/helm plugins with or we will need alpha versions of them.
See that the kustomize version should not be in ansbile/helm/golang plugin, https://github.com/kubernetes-sigs/kubebuilder/pull/2583/files#r839541438 (it is part of the kustomize plugin domain of responsability)

@camilamacedo86 camilamacedo86 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 17, 2022
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold

I do not think that we can move with this one.
When we bump the next Kubebuilder release we will be able to check if we need to begin to use the new kustomize/v2-alpha to have the version v4. See: kubernetes-sigs/kubebuilder#2583

Also, we need to evaluate if we can make the helm/ansible plugins begin to use kustomize.v2-alpha directly or if we need new versions of these plugins as well.

c/c @rashmigottipati @ryantking @asmacdo @fabianvf @everettraven

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2022
@openshift-merge-robot
Copy link

@gquillar: PR needs rebase.

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.

@camilamacedo86
Copy link
Contributor

@gquillar I am closing this one because the changes were addressed here: #5965

However, please feel free to re-open or contact me if you see that can I help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants