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

kubeadm: enable join dryrun e2e tests #3116

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

SataQiu
Copy link
Member

@SataQiu SataQiu commented Oct 26, 2024

kubeadm: enable join dryrun e2e tests without a real control plane

Ref: #2653

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 26, 2024
@k8s-ci-robot k8s-ci-robot added the area/kinder Issues to track work in the kinder tool label Oct 26, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 26, 2024
@SataQiu
Copy link
Member Author

SataQiu commented Oct 26, 2024

/hold for review

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2024
@pacoxu
Copy link
Member

pacoxu commented Oct 28, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2024
@carlory
Copy link
Member

carlory commented Oct 28, 2024

/lgtm

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

i had this PR planned but i could not get to it. so thanks for sending it.

i think we should do the following:

rename init-dry -> dry-run-without-cluster
perform the required_version check there
if versions is 1.32, run all these before a cluster is created:

  • init dry run
  • join control plane dry run
  • join worker (too maybe?)
  • upgrade apply dry run
  • upgrade node dry run
  • reset dry run

then continue testing the commands after the init node was created as it is now.

@@ -60,6 +60,22 @@ tasks:
docker exec {{ .vars.clusterName }}-control-plane-1 kubeadm init --ignore-preflight-errors={{ .vars.kubeadmIgnorePreflightErrors }} --kubernetes-version={{ .vars.kubernetesVersion }} --dry-run=true --v={{ .vars.kubeadmVerbosity }} || exit 1
docker exec {{ .vars.clusterName }}-control-plane-1 kubeadm init --ignore-preflight-errors={{ .vars.kubeadmIgnorePreflightErrors }} --kubernetes-version={{ .vars.kubernetesVersion }} --upload-certs=true --dry-run=true --v={{ .vars.kubeadmVerbosity }} || exit 1
Copy link
Member

@neolit123 neolit123 Oct 28, 2024

Choose a reason for hiding this comment

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

i think the upload certs is no longer required? (or we can only call this variant)

Comment on lines 70 to 67
required_version="1.32" # join dry-run without real control plane is available since v1.32
current_version=$(echo "{{ .vars.kubernetesVersion }}" | grep -oP '\d+\.\d+')
if awk "BEGIN {exit !($current_version < $required_version)}"; then
echo "Join dry-run without real control plane is not available for Kubernetes version {{ .vars.kubernetesVersion }}. Skipping."
exit 0
fi
Copy link
Member

Choose a reason for hiding this comment

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

i like this check. it avoids us having to write updates to the kinder config (new params) or adding more workflow files.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2024
@SataQiu
Copy link
Member Author

SataQiu commented Oct 30, 2024

kubeadm upgrade dryrun without cluster is still not supported yet.

# kubeadm upgrade apply -f v1.32.0-alpha.3.47+daef8c2419a638 --ignore-preflight-errors=Swap,SystemVerification,FileContent--proc-sys-net-bridge-bridge-nf-call-iptables --allow-release-candidate-upgrades=true --allow-experimental-upgrades=true --dry-run --v=6

open /etc/kubernetes/admin.conf: no such file or directory
failed to load kubeconfig
k8s.io/kubernetes/cmd/kubeadm/app/util/apiclient.(*DryRun).WithKubeConfigFile
        k8s.io/kubernetes/cmd/kubeadm/app/util/apiclient/dryrun.go:79
k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade.getClient
        k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade/common.go:195
k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade.newApplyData
        k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade/apply.go:226
k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade.newCmdApply.func2
        k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade/apply.go:146
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow.(*Runner).InitData
        k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow/runner.go:185
k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade.newCmdApply.func1
        k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade/apply.go:99
github.com/spf13/cobra.(*Command).execute
        github.com/spf13/[email protected]/command.go:985
github.com/spf13/cobra.(*Command).ExecuteC
        github.com/spf13/[email protected]/command.go:1117
github.com/spf13/cobra.(*Command).Execute
        github.com/spf13/[email protected]/command.go:1041
k8s.io/kubernetes/cmd/kubeadm/app.Run
        k8s.io/kubernetes/cmd/kubeadm/app/kubeadm.go:47
main.main
        k8s.io/kubernetes/cmd/kubeadm/kubeadm.go:25
runtime.main
        runtime/proc.go:272
runtime.goexit
        runtime/asm_amd64.s:1700
couldn't create a Kubernetes client from file "/etc/kubernetes/admin.conf"
k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade.newApplyData
        k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade/apply.go:228
k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade.newCmdApply.func2
        k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade/apply.go:146
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow.(*Runner).InitData
        k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow/runner.go:185
k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade.newCmdApply.func1
        k8s.io/kubernetes/cmd/kubeadm/app/cmd/upgrade/apply.go:99
github.com/spf13/cobra.(*Command).execute
        github.com/spf13/[email protected]/command.go:985
github.com/spf13/cobra.(*Command).ExecuteC
        github.com/spf13/[email protected]/command.go:1117
github.com/spf13/cobra.(*Command).Execute
        github.com/spf13/[email protected]/command.go:1041
k8s.io/kubernetes/cmd/kubeadm/app.Run
        k8s.io/kubernetes/cmd/kubeadm/app/kubeadm.go:47
main.main
        k8s.io/kubernetes/cmd/kubeadm/kubeadm.go:25
runtime.main
        runtime/proc.go:272
runtime.goexit
        runtime/asm_amd64.s:1700

@SataQiu SataQiu force-pushed the join-dryrun-20241026 branch 2 times, most recently from ed957ec to b6fd229 Compare October 30, 2024 12:22
@SataQiu SataQiu changed the title kubeadm: enable join dryrun e2e tests [WIP]kubeadm: enable join dryrun e2e tests Oct 30, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2024
@neolit123
Copy link
Member

kubeadm upgrade dryrun without cluster is still not supported yet.

i think it used to work. perhaps we broke it with upgrade apply phases.
checking..

@neolit123
Copy link
Member

kubeadm upgrade dryrun without cluster is still not supported yet.

i think it used to work. perhaps we broke it with upgrade apply phases. checking..

no, seems like upgrade does need the admin.conf
https://github.com/kubernetes/kubernetes/pull/126776/files#diff-657870bffd956e3e3553db7dbb5cb725ea53922da959fcf869c2e31880df72c7R195

but i can try to make it work without it.

@neolit123
Copy link
Member

kubeadm upgrade dryrun without cluster is still not supported yet.

i think it used to work. perhaps we broke it with upgrade apply phases. checking..

no, seems like upgrade does need the admin.conf https://github.com/kubernetes/kubernetes/pull/126776/files#diff-657870bffd956e3e3553db7dbb5cb725ea53922da959fcf869c2e31880df72c7R195

but i can try to make it work without it

@SataQiu SataQiu changed the title [WIP]kubeadm: enable join dryrun e2e tests kubeadm: enable join dryrun e2e tests Nov 13, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2024
@SataQiu
Copy link
Member Author

SataQiu commented Nov 13, 2024

this is ready for more review, I think we can merge it now.
@neolit123

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

LGTM, two minor nits.

set -x
required_version="1.32" # dryrun without cluster is available since v1.32
current_version=$(echo "{{ .vars.kubernetesVersion }}" | grep -oP 'v\K\d+\.\d+')
if awk "BEGIN {exit !($current_version < $required_version)}"; then
Copy link
Member

Choose a reason for hiding this comment

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

i think this would work too

Suggested change
if awk "BEGIN {exit !($current_version < $required_version)}"; then
if [ "$current_version" \< "$required_version" ]; then

tested in a local bash.

#!/bin/bash

required_version="1.32"
current_version=$(echo "v1.34.0" | grep -oP 'v\K\d+\.\d+')

if [ "$current_version" \< "$required_version" ]; then
    echo "less"
else
    echo "supported"
fi

echo "Full dryrun support without cluster is not available for Kubernetes version {{ .vars.kubernetesVersion }}, only init-dryrun will be executed for this task."
docker exec {{ .vars.clusterName }}-control-plane-1 bash -c "until crictl ps &> /dev/null; do echo 'Waiting for the container runtime to be running ...'; sleep 1; done"
docker exec {{ .vars.clusterName }}-control-plane-1 kubeadm init --ignore-preflight-errors={{ .vars.kubeadmIgnorePreflightErrors }} --kubernetes-version={{ .vars.kubernetesVersion }} --dry-run --v={{ .vars.kubeadmVerbosity }} || exit 1
else
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
else
exit 0
fi

just so that we don't indent the else part.
also added one extra line after the fi.

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/lgtm
/apporove
thanks @SataQiu

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlory, neolit123, pacoxu, SataQiu

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [SataQiu,neolit123,pacoxu]

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

@neolit123 neolit123 mentioned this pull request Nov 13, 2024
3 tasks
@SataQiu
Copy link
Member Author

SataQiu commented Nov 13, 2024

/hold cancel

Let's see if it works

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2024
@k8s-ci-robot k8s-ci-robot merged commit 3a2bb82 into kubernetes:main Nov 13, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kinder Issues to track work in the kinder tool cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants