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

⚠️ Use k8s v1.27.0 in quickstart docs and CAPD #8518

Conversation

furkatgofurov7
Copy link
Member

@furkatgofurov7 furkatgofurov7 commented Apr 12, 2023

Signed-off by: Furkat Gofurov ([email protected])

What this PR does / why we need it:
Bumps k8s version to v1.27.0 in Quickstart docs and CAPD.

NOTE:

  • etcd version is bumped to 3.5.7-0 following the kubeadm
  • COREDNS_VERSION is bumped to v1.10.1 following the kubeadm
  • new field called: WorkloadKubernetesVersion was introduced to ClusterctlUpgradeSpecInput

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Part of: #8459

cc @kubernetes-sigs/cluster-api-release-team

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 12, 2023
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 12, 2023
@furkatgofurov7
Copy link
Member Author

/hold

Manually verifying quickstart is pending

@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 Apr 12, 2023
@furkatgofurov7 furkatgofurov7 force-pushed the update-quickstart-docs-capd-new-k8s-version branch from ffa36ee to 290f49e Compare April 12, 2023 20:42
@furkatgofurov7
Copy link
Member Author

/hold

Manually verifying quickstart is pending

Done and all good.

/hold cancel

@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 Apr 12, 2023
@furkatgofurov7
Copy link
Member Author

/assign @sbueringer

@furkatgofurov7
Copy link
Member Author

GH rate limit issue

/test pull-cluster-api-e2e-informing-ipv6-main

test/e2e/config/docker.yaml Outdated Show resolved Hide resolved
@furkatgofurov7 furkatgofurov7 force-pushed the update-quickstart-docs-capd-new-k8s-version branch 2 times, most recently from b6153f9 to c4e425e Compare April 13, 2023 12:26
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Just a few nits

test/e2e/clusterctl_upgrade_test.go Outdated Show resolved Hide resolved
test/e2e/clusterctl_upgrade_test.go Outdated Show resolved Hide resolved
test/e2e/clusterctl_upgrade_test.go Show resolved Hide resolved
@sbueringer
Copy link
Member

/test pull-cluster-api-e2e-full-main

^^ this test has to be green before merge

@sbueringer
Copy link
Member

/retest

@killianmuldoon
Copy link
Contributor

/retest

possibly a flake

@sbueringer
Copy link
Member

/retest

@sbueringer
Copy link
Member

@furkatgofurov7 @killianmuldoon Let's wait with test-infra PR merges until this PR is green

@killianmuldoon
Copy link
Contributor

Definitely consistent failure in the 0.3 upgrade test 🤔. I'll have a look certainly by tomorrow some time (could have been a good candidate for the hacking session 😄)

@furkatgofurov7 furkatgofurov7 force-pushed the update-quickstart-docs-capd-new-k8s-version branch from c4e425e to d1e17ef Compare April 13, 2023 20:58
@furkatgofurov7
Copy link
Member Author

/test pull-cluster-api-e2e-full-main

@furkatgofurov7
Copy link
Member Author

/retest

@sbueringer
Copy link
Member

sbueringer commented Apr 14, 2023

Ups, is this test actually trying to create a 1.27 workload cluster with CAPI v0.3?

INFO: Creating the workload cluster with name "clusterctl-upgrade-eq86nx" using the "(default)" template (Kubernetes v1.27.0, 1 control-plane machines, 1 worker machines)

If I see this correctly we should introduce a new parameter to the test spec for the test cluster kubernetes version

@sbueringer
Copy link
Member

sbueringer commented Apr 14, 2023

Context:

  • This test creates a separate management cluster with Kubernetes version: InitWithKubernetesVersion
  • Then the old providers are deployed on this cluster
  • Then we create a test workload cluster via the old providers

Proposal:

  • I would suggest to introduce a new field to ClusterctlUpgradeSpecInput called: WorkloadKubernetesVersion
  • This field should be set to the maximumg workload cluster version supported by the old providers

So we end up with the following:

Test InitWithKubernetesVersion WorkloadKubernetesVersion
v0.3 v1.21.14 v1.22.17
v0.4 v1.23.17 v1.23.17
v1.0 v1.23.17 v1.23.17
v1.3 v1.26.3 v1.26.3
v1.4 v1.27.0 v1.27.0

InitWithKubernetesVersion is the version we deploy the old provider on
WorkloadKubernetesVersion is the version of the workload cluster the old provider creates

Both those version should be the highest the old provider supports (this makes it very likely that the main version also supports those versions, and as of today that always works)

Note: patch version should be the highest we have corresponding kindest/node image for on dockerhub. I looked them up and "bumped" them a bit. We don't have to continuously keep them up-to-date, but maybe let's do it now as we have to introduce WorkloadKubernetesVersion anyway

@furkatgofurov7 @killianmuldoon @fabriziopandini @ykakarap Opinions?

@furkatgofurov7
Copy link
Member Author

Ups, is this test actually trying to create a 1.27 workload cluster with CAPI v0.3?

yeah, but that is the case with 0.4, as well no? And I saw that went fine 🤔

@sbueringer
Copy link
Member

sbueringer commented Apr 14, 2023

Ups, is this test actually trying to create a 1.27 workload cluster with CAPI v0.3?

yeah, but that is the case with 0.4, as well no? And I saw that went fine thinking

I could imagine that v0.4 is for some reason compatible with v1.27 (e.g. we use a newer client-go, controller-runtime version).

But it should be relatively easy to see by running the failing test locally and just looking why the Machines are not coming up (I just don't have the time for it right now).

I think we should definitely find out why the test is actually failing.

I would do the suggested change in any case btw, even if we don't need it for this specific PR. It might just be the solution we need if we find out that v0.3 is simply not compatible with Kubernetes v1.27

@furkatgofurov7 furkatgofurov7 force-pushed the update-quickstart-docs-capd-new-k8s-version branch from 45ef435 to 077d303 Compare April 17, 2023 06:59
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2023
@furkatgofurov7
Copy link
Member Author

Running it once again to be sure nothing new was introduced while rebasing:

/test pull-cluster-api-e2e-full-main

Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks for getting this one over the line - was a bit more complex than usual 🙂

/lgtm

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

LGTM label has been added.

Git tree hash: ea2f0bacea383ae5e42a5af387f2a412defa62f7

@sbueringer
Copy link
Member

Thank you very much!!

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2023
@k8s-ci-robot k8s-ci-robot merged commit a9d4775 into kubernetes-sigs:main Apr 17, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.5 milestone Apr 17, 2023
@furkatgofurov7
Copy link
Member Author

@killianmuldoon @sbueringer thx folks, do we need to back-port it to release-1.4 branch?

@furkatgofurov7 furkatgofurov7 deleted the update-quickstart-docs-capd-new-k8s-version branch April 17, 2023 11:44
@sbueringer
Copy link
Member

sbueringer commented Apr 17, 2023

@killianmuldoon @sbueringer thx folks, do we need to back-port it to release-1.4 branch?

Yes, please!

I think we can & should definitely prepare the PR already. Then we have to decide if we want to "officially" support 1.27 already in 1.4.2 or merge it intentionally afterwards to get more time in CI

@furkatgofurov7
Copy link
Member Author

Yes, please!

I think we can & should definitely prepare the PR already. Then we have to decide if we want to "officially" support 1.27 already in 1.4.2 or merge it intentionally afterwards to get more time in CI

ack, let me try the cherry-picker and see if it can do it otherwise will manually cherry pick it:

/cherry-pick release-1.4

@k8s-infra-cherrypick-robot

@furkatgofurov7: #8518 failed to apply on top of branch "release-1.4":

Applying: Bump k8s version to v1.27.0 in quickstart docs and Tiltfile
Applying: Use k8s v1.27.0 in CAPD and tests
Using index info to reconstruct a base tree...
M	test/e2e/clusterctl_upgrade_test.go
M	test/e2e/config/docker.yaml
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/config/docker.yaml
Auto-merging test/e2e/clusterctl_upgrade_test.go
Applying: Introduce WorkloadKubernetesVersion field to ClusterctlUpgradeSpecInput
.git/rebase-apply/patch:44: trailing whitespace.
- Providers running CAPI release-0.3 clusterctl upgrade tests should set `WorkloadKubernetesVersion` field to the maximum workload cluster kubernetes version supported by the old providers in `ClusterctlUpgradeSpecInput`. For more information, please see: https://github.com/kubernetes-sigs/cluster-api/pull/8518#issuecomment-1508064859 
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
A	docs/book/src/developer/providers/migrations/v1.3-to-v1.4.md
A	docs/book/src/developer/providers/migrations/v1.4-to-v1.5.md
M	docs/release/release-tasks.md
M	test/e2e/clusterctl_upgrade_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/clusterctl_upgrade_test.go
CONFLICT (content): Merge conflict in test/e2e/clusterctl_upgrade_test.go
Auto-merging docs/release/release-tasks.md
Auto-merging docs/book/src/developer/providers/v1.3-to-v1.4.md
CONFLICT (modify/delete): docs/book/src/developer/providers/migrations/v1.4-to-v1.5.md deleted in HEAD and modified in Introduce WorkloadKubernetesVersion field to ClusterctlUpgradeSpecInput. Version Introduce WorkloadKubernetesVersion field to ClusterctlUpgradeSpecInput of docs/book/src/developer/providers/migrations/v1.4-to-v1.5.md left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 Introduce WorkloadKubernetesVersion field to ClusterctlUpgradeSpecInput
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

Yes, please!

I think we can & should definitely prepare the PR already. Then we have to decide if we want to "officially" support 1.27 already in 1.4.2 or merge it intentionally afterwards to get more time in CI

ack, let me try the cherry-picker and see if it can do it otherwise will manually cherry pick it:

/cherry-pick release-1.4

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.

@sbueringer
Copy link
Member

No chance with the migration doc :D

@furkatgofurov7
Copy link
Member Author

No chance with the migration doc :D

Here: #8538

@johannesfrey
Copy link
Contributor

/area provider/infrastructure-docker e2e-testing documentation

@k8s-ci-robot k8s-ci-robot added area/provider/infrastructure-docker Issues or PRs related to the docker infrastructure provider area/e2e-testing Issues or PRs related to e2e testing area/documentation Issues or PRs related to documentation labels Jun 16, 2023
@killianmuldoon killianmuldoon removed area/documentation Issues or PRs related to documentation area/e2e-testing Issues or PRs related to e2e testing labels Jun 27, 2023
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/provider/infrastructure-docker Issues or PRs related to the docker infrastructure provider 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants