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

Support development version of Kustomize in install script #428

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

janwytze
Copy link
Contributor

Closes #427

Motivation

Kustomize version (devel) is not supported by the install script.

Modifications

Changed the regex so (devel) is a supported version.

Result

Install script now works when Kustomize is installed from https://kubectl.docs.kubernetes.io/installation/kustomize/source/

Copy link
Member

@rafvasq rafvasq left a comment

Choose a reason for hiding this comment

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

Hi @janwytze, thanks for the PR. Along with testing the load restrictor logic below, other scripts would need to include your update as well e.g. /scripts/setup_user_namespaces.sh and /scripts/delete.sh.

@@ -378,7 +378,7 @@ wait_for_pods_ready "-l control-plane=modelmesh-controller"

# Older versions of kustomize have different load restrictor flag formats.
# Can be removed once Kubeflow installation stops requiring v3.2.
kustomize_version=$(kustomize version --short | grep -o -E "[0-9]\.[0-9]\.[0-9]")
kustomize_version=$(kustomize version --short | grep -o -E "[0-9]\.[0-9]\.[0-9]|\(devel\)")
Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm that the logic below for the kustomize_load_restrictor_arg continues to work with the change? Seems to me like with kustomize version (devel), I hit Error: unknown flag: --load_restrictor none.

Copy link
Contributor Author

@janwytze janwytze Sep 12, 2023

Choose a reason for hiding this comment

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

@rafvasq The output of kustomize version --short on my machine is:

Flag --short has been deprecated, and will be removed in the future.
{(devel)  unknown   }

This causes $kustomize_load_restrictor_arg to be set to:

--load-restrictor LoadRestrictionsNone

I believe this is the correct behavior. Both the install and delete scripts succeed for me after this PR.

Copy link
Member

Choose a reason for hiding this comment

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

My output is

Flag --short has been deprecated, and will be removed in the future.
{(devel)  2023-09-11T18:05:37Z   }

and results in $kustomize_load_restrictor_arg to be --load_restrictor none.

I'm not sure if it's a variation in how string comparisons are handled across Bash versions, locale settings, or something else, but regardless it I think the logic could be hardened.

@@ -378,7 +378,7 @@ wait_for_pods_ready "-l control-plane=modelmesh-controller"

# Older versions of kustomize have different load restrictor flag formats.
# Can be removed once Kubeflow installation stops requiring v3.2.
kustomize_version=$(kustomize version --short | grep -o -E "[0-9]\.[0-9]\.[0-9]")
kustomize_version=$(kustomize version --short | grep -o -E "[0-9]\.[0-9]\.[0-9]|\(devel\)")
Copy link
Member

Choose a reason for hiding this comment

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

My output is

Flag --short has been deprecated, and will be removed in the future.
{(devel)  2023-09-11T18:05:37Z   }

and results in $kustomize_load_restrictor_arg to be --load_restrictor none.

I'm not sure if it's a variation in how string comparisons are handled across Bash versions, locale settings, or something else, but regardless it I think the logic could be hardened.

@ckadner

This comment was marked as duplicate.

ckadner

This comment was marked as outdated.

@ckadner ckadner dismissed their stale review September 13, 2023 21:21

based on incomplete facts

@ckadner
Copy link
Member

ckadner commented Sep 13, 2023

I removed my earlier review after doing a bit more testing. Will update shortly.

Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

Instead of trying to capture the kustomize CLI version, we could check for the spelling and parameter value variation of the supported load restrictor flag

load_restrictor_flag=$( kustomize build --help | grep -o -E "\-\-load.restrictor[^,]+" | sed -E "s/(--load.restrictor).+'(.*none)'/\1 \2/I" )
...
kustomize build runtimes $load_restrictor_flag

Quick test:

# shell function to set kustomize version, download CLI to user local bin if it doesn't exists
set-kustomize-version() {

  # check and process function parameters
  [ -z "${1}" ] && echo "Usage: '${FUNCNAME[0]} <version>'" && return 1
  [[ ! "$1" =~ ^[0-9]+(\.[0-9]+){2}$ ]] && echo "<version> should be of format 'x.y.z'" && return 1
  VERSION="$1"

  # https://kubectl.docs.kubernetes.io/installation/kustomize/binaries/
  INSTALL_SCRIPT_URL="https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh"

  # create user-local kustomize install dir 
  INSTALL_DIR=~/bin/kustomize/${VERSION}
  mkdir -p ${INSTALL_DIR}

  # download the requested kustomize CLI, if not done before
  [ -f ${INSTALL_DIR}/kustomize ] || curl -s ${INSTALL_SCRIPT_URL} | bash -s -- ${VERSION} ${INSTALL_DIR}

  # prepend the session PATH with the folder of the desired kustomize CLI version
  export PATH=${INSTALL_DIR}:$PATH

  # report the set kustomize CLI version to the user
  echo -n "Kustomize version: "
  (kustomize version --short 2> /dev/null || kustomize version) | grep -o -E "[0-9]\.[0-9]\.[0-9]"
}

# specify key kustomize versions
kustomize_versions=("3.3.0" "4.0.0" "4.1.0")

# navigate to the "config" folder inside modelmesh-serving repo
cd config

# for each of the kustomize versions, try to build the runtimes with the correct load-restrictor flag
for v in ${kustomize_versions[@]}; do
  set-kustomize-version $v

  load_restrictor_flag=$( kustomize build --help | grep -o -E "\-\-load.restrictor[^,]+" | sed -E "s/(--load.restrictor).+'(.*none)'/\1 \2/I" )
  echo "$load_restrictor_flag"

  kustomize build runtimes $load_restrictor_flag 2>&1 > /dev/null \
    && echo "SUCCESS" \
    || echo "FAILED"

  echo
done

Output on MacOS and Linux:

Kustomize version: 3.3.0
--load_restrictor none
SUCCESS

Kustomize version: 4.0.0
--load_restrictor LoadRestrictionsNone
SUCCESS

Kustomize version: 4.1.0
--load-restrictor LoadRestrictionsNone
SUCCESS

Copy link
Member

@rafvasq rafvasq left a comment

Choose a reason for hiding this comment

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

Tested with versions 3.2.0, 5.1.1, and (devel). Thanks @janwytze!

setup_user_namespaces scripts

Signed-off-by: Jan Wytze Zuidema <[email protected]>
Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

/lgtm

@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ckadner, janwytze, rafvasq
To complete the pull request process, please assign njhill after the PR has been reviewed.
You can assign the PR to them by writing /assign @njhill in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@ckadner ckadner merged commit f47cd7b into kserve:main Sep 19, 2023
5 checks passed
@ckadner ckadner assigned janwytze and unassigned ckadner Oct 2, 2023
@ckadner ckadner added this to the v0.11.1 milestone Oct 2, 2023
@ckadner ckadner self-assigned this Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Install script not working on kustimize version (devel)
4 participants