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.

Sign up for GitHub

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

Jump to bottom

Update Kind to 1.29.1 #73

Merged
merged 1 commit into from
Feb 14, 2024
Merged

Update Kind to 1.29.1 #73

merged 1 commit into from
Feb 14, 2024

Conversation

elezar
Copy link
Member

@elezar elezar commented Feb 12, 2024 •
edited
Loading

This change updates the Kind cluster definition to v1.29.1.

This also removes the need to use a custom kind base image if the version is already available.

@elezar elezar self-assigned this Feb 12, 2024
@elezar elezar requested a review from klueska February 12, 2024 14:59
klueska
klueska reviewed Feb 12, 2024
View reviewed changes
demo/clusters/kind/scripts/build-kind-image.sh Outdated
@@ -44,4 +48,4 @@ KIND_K8S_DIR="${TMP_DIR}/kubernetes-${KIND_K8S_TAG}"
git clone --depth 1 --branch ${KIND_K8S_TAG} ${KIND_K8S_REPO} ${KIND_K8S_DIR}

# Build the kind base image
kind build node-image --base-image "${KIND_IMAGE_BASE}" --image "${KIND_IMAGE}" "${KIND_K8S_DIR}"
kind build node-image ${KIND_IMAGE_BASE:+--base-image "${KIND_IMAGE_BASE}"} --image "${KIND_IMAGE}" "${KIND_K8S_DIR}"fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the fi at the end of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah. Yes, I had this in a separate conditional block instead of quick returning above. Will update.

klueska
klueska reviewed Feb 12, 2024
View reviewed changes
demo/clusters/kind/scripts/build-kind-image.sh Outdated
Comment on lines 32 to 36
if [[ x"${KIND_IMAGE_BASE}" == x"" && x"${KIND_K8S_TAG}" == x"" ]]; then
exit 0
fi

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ia tehre a reason to use the double [[ syntax and the x before the strings here? Mostly because it is inconsistent with the rest of the bash scripts here.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not really. I'm just used to the double syntax. Will update.

klueska
klueska reviewed Feb 12, 2024
View reviewed changes
demo/clusters/kind/scripts/common.sh Outdated
: ${KIND_IMAGE_BASE_TAG:="v20230515-01914134-containerd_v1.7.1"}
: ${KIND_IMAGE_BASE:="gcr.io/k8s-staging-kind/base:${KIND_IMAGE_BASE_TAG}"}
: ${KIND_IMAGE:="kindest/node:${KIND_K8S_TAG}-${KIND_IMAGE_BASE_TAG}"}
${NVIDIA_DRIVER_ROOT:+--set nvidiaDriverRoot=${NVIDIA_DRIVER_ROOT}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does the :+ yntax do exactly? Where is this variable used later on and why wasn't it needed in v1.27.1?

Copy link
Member Author

Choose a reason for hiding this comment

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

:+ only expands the section that follows it if the variable is defined.

For example:

$ export FOO=baz
$ echo "some/thing/possibly${FOO:+-${FOO}}"
some/thing/possibly-bar
$ unset FOO
$ echo "some/thing/possibly${FOO:+-${FOO}}"
some/thing/possibly

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, now I see the confusion. The NVIDIA_DRIVER_ROOT is not required. I had pasted the syntax here as a reminder while working on the usage of :+ below but never removed it.

Removed now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It still seems to be there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I had forgotten to push.

@elezar elezar changed the title Update Kind to 1.28.6 Update Kind to 1.29.1 Feb 12, 2024
@elezar elezar requested a review from klueska February 12, 2024 15:46
klueska
klueska reviewed Feb 14, 2024
View reviewed changes
demo/clusters/kind/scripts/build-kind-image.sh Outdated
@@ -29,6 +29,11 @@ if [ "${EXISTING_IMAGE_ID}" != "" ]; then
exit 0
fi

PREBUILT_IMAGE="$( ( docker manifest inspect ${KIND_IMAGE} > /dev/null && echo "available" ) || ( echo "not-available" ) )"
if [ "${PREBUILT_IMAGE}" = "available" ] && [ x"${KIND_IMAGE_BASE}" = x"" ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: remove the 'x's in [ x"${KIND_IMAGE_BASE}" = x"" ].

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be updated now.

demo/clusters/kind/scripts/common.sh Outdated
Comment on lines 52 to 54
## KIND_IMAGE_BASE_TAG can be specified to build specific requirements into a kind image.
## This should in general not be required.
# : ${KIND_IMAGE_BASE_TAG:="v20230515-01914134-containerd_v1.7.1"}
: ${KIND_IMAGE_BASE:="${KIND_IMAGE_BASE_TAG:+gcr.io/k8s-staging-kind/base:${KIND_IMAGE_BASE_TAG}}"}
: ${KIND_IMAGE:="kindest/node:${KIND_K8S_TAG}${KIND_IMAGE_BASE_TAG:+-${KIND_IMAGE_BASE_TAG}}"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This section is confusing to me. Why do we have KIND_IMAGE_BASE_TAG but it is commented out. We then clearly set a KIND_IMAGE_BASE (though I guess it equals "" since KIND_IMAGE_BASE_TAG is commented out?). Then in KIND_IMAGE we only conditionally include -${KIND_IMAGE_BASE_TAG} if KIND_IMAGE_BASE_TAG is set (which according to the logic above it never is).

The only thing I can figure is that this is designed to support someone setting KIND_IMAGE_BASE_TAG on the command line, which I don't think we want to do.

Can we simplify this to remove all of this inline cnditional logic (maybe leaving a separate comment block to show what would be necessary if we ever need to support a KIND_IMAGE_BASE_TAG in the future again).

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I have cleaned this up a bit. We only specify the full kind image here and then the note on how to use a custom base image is included where we build the image.

I think it's less likely that we will need custom base images since we don't specifically need runtime features for this to work.

demo/clusters/kind/scripts/common.sh Outdated
: ${KIND_IMAGE_BASE_TAG:="v20230515-01914134-containerd_v1.7.1"}
: ${KIND_IMAGE_BASE:="gcr.io/k8s-staging-kind/base:${KIND_IMAGE_BASE_TAG}"}
: ${KIND_IMAGE:="kindest/node:${KIND_K8S_TAG}-${KIND_IMAGE_BASE_TAG}"}
${NVIDIA_DRIVER_ROOT:+--set nvidiaDriverRoot=${NVIDIA_DRIVER_ROOT}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It still seems to be there.

@elezar elezar force-pushed the bump-kind-test-to-1.28.6 branch 2 times, most recently from c739617 to 716a822 Compare February 14, 2024 10:44
@elezar
Update Kind to 1.29.1
e7d9271 
Signed-off-by: Evan Lezar <[email protected]>
klueska
klueska approved these changes Feb 14, 2024
View reviewed changes
Copy link
Collaborator

@klueska klueska left a comment

Choose a reason for hiding this comment

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

Much clearer now thanks.

@elezar elezar merged commit ba45c06 into NVIDIA:main Feb 14, 2024
5 checks passed
@elezar elezar deleted the bump-kind-test-to-1.28.6 branch February 14, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klueska klueska klueska approved these changes

Assignees

@elezar elezar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@elezar @klueska