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

e2e: set imageRepository to "" in yaml files #243

Merged

Conversation

weizhouapache
Copy link
Collaborator

@weizhouapache weizhouapache commented May 8, 2023

Issue #, if available:

The image repository has been changed to registry.k8s.io by commit 8c1e614

However, with registry.k8s.io, the control plane vm cannot be booted to Ready state due to error below

    ubuntu@disk-offering-gzojvb-control-plane-gwbnt:~$ tail -f /var/log/cloud-init-output.log
    [2023-05-08 11:09:23] [preflight] You can also perform this action in beforehand using 'kubeadm config images pull'
    [2023-05-08 11:09:28] error execution phase preflight: [preflight] Some fatal errors occurred:
    [2023-05-08 11:09:28]   [ERROR ImagePull]: failed to pull image registry.k8s.io/coredns:v1.8.4: output: time="2023-05-08T11:09:28Z" level=fatal msg="pulling image: rpc error: code = NotFound desc = failed to pull and unpack image \"registry.k8s.io/coredns:v1.8.4\": failed to resolve reference \"registry.k8s.io/coredns:v1.8.4\": registry.k8s.io/coredns:v1.8.4: not found"
    [2023-05-08 11:09:28] , error: exit status 1

this is same as kubernetes/kubeadm#2761
The new registry should be supported in k8s 1.25+. However, we still use 1.22/1.23/1.24 templates, so we need to use k8s.gcr.io

Description of changes:

setting the image respository to "" so that capi/kubeadm will determine the default repository by kubernetes version.

Testing performed:

e2e tests look ok now.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 8, 2023
@netlify
Copy link

netlify bot commented May 8, 2023

Deploy Preview for kubernetes-sigs-cluster-api-cloudstack ready!

Name Link
🔨 Latest commit 5685e99
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-cloudstack/deploys/645b44a7f029680008c26304
😎 Deploy Preview https://deploy-preview-243--kubernetes-sigs-cluster-api-cloudstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 8, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @weizhouapache. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 8, 2023
@hrak
Copy link
Contributor

hrak commented May 8, 2023

I'm not sure about this change, for one, we depend on k8s 1.25 now. Two is that Kubernetes <= 1.24 is EOL. Three is that the Kubernetes project is pretty clear about changing to registry.k8s.io ASAP.

@weizhouapache
Copy link
Collaborator Author

I'm not sure about this change, for one, we depend on k8s 1.25 now. Two is that Kubernetes <= 1.24 is EOL. Three is that the Kubernetes project is pretty clear about changing to registry.k8s.io ASAP.

@hrak
the VM templates use kubernetes 1.22/1.23/1.24
https://github.com/kubernetes-sigs/cluster-api-provider-cloudstack#operating-system-images

the templates are built using image-builder and packer. the default k8s version is v1.24.11 now.
Refer to https://github.com/kubernetes-sigs/image-builder/blob/master/images/capi/packer/config/kubernetes.json

@hrak
Copy link
Contributor

hrak commented May 8, 2023

I'm not sure whether the fact that the Shapeblue provided templates haven't updated to the latest supported versions of Kubernetes (1.25, 1.26 and 1.27) should determine what this project supports, but i might be wrong. My thought is that the aim should be to be in line with the latest supported k8s releases.

image-builder also has the tendency to lag behind, i am frequently filing PR's there to update dependencies.

@rohityadavcloud
Copy link
Member

Hi @hrak I can't comment on all the technical details of this PR and your comments, perhaps @davidjumani or @vishesh92 can advise?

The image-builder project tries to stay on one version behind the latest k8s release https://github.com/kubernetes-sigs/image-builder/blob/master/images/capi/packer/config/kubernetes.json, which of course anybody can tune/update for their use-case and build custom templates. I would incline towards this approach as well, of course we shouldn't support EOL k8s releases but we should support k8s releases that are stable (latest tends to be breaking many a times until a few minor releases are seen).

@rohityadavcloud
Copy link
Member

@weizhouapache @vishesh92 @davidjumani perhaps we should look at and update the capc templates?
/cc @jweite-amazon @g-gaston @chrisdoherty4 - what k8s version is preferred/used these days for your use-case?

@g-gaston
Copy link
Contributor

g-gaston commented May 8, 2023

@weizhouapache @vishesh92 @davidjumani perhaps we should look at and update the capc templates? /cc @jweite-amazon @g-gaston @chrisdoherty4 - what k8s version is preferred/used these days for your use-case?

@rohityadavcloud

Ideally we would test the versions supported by the latest version of cluster-api
So in this case:

👌 Kubernetes version support

  • Management Cluster: v1.23.x -> v1.27.x [1]
  • Workload Cluster: v1.21.x -> v1.27.x

As of what to do with versions < 1.25, I would suggest checking what they did in the capi repo. They had to face this same issue since there are tests for 1.21 -> 1.24

@weizhouapache
Copy link
Collaborator Author

I'm not sure whether the fact that the Shapeblue provided templates haven't updated to the latest supported versions of Kubernetes (1.25, 1.26 and 1.27) should determine what this project supports, but i might be wrong. My thought is that the aim should be to be in line with the latest supported k8s releases.

image-builder also has the tendency to lag behind, i am frequently filing PR's there to update dependencies.

@hrak @rohityadavcloud
we could build capc templates with latest k8s versions (1.25/1.26/1.27).
However, I think we should support not only these versions , but also older versions which might be still used by some users or tests.

PS:
(1) k8s 1.24 is not EOL yet.
https://kubernetes.io/releases/#release-v1-24
(2) this provider is compatible with k8s 1.22/1.23/1.24
https://github.com/kubernetes-sigs/cluster-api-provider-cloudstack#compatibility-with-cluster-api-and-kubernetes-versions

@weizhouapache
Copy link
Collaborator Author

@weizhouapache @vishesh92 @davidjumani perhaps we should look at and update the capc templates? /cc @jweite-amazon @g-gaston @chrisdoherty4 - what k8s version is preferred/used these days for your use-case?

@rohityadavcloud

Ideally we would test the versions supported by the latest version of cluster-api So in this case:

ok_hand Kubernetes version support

  • Management Cluster: v1.23.x -> v1.27.x [1]
  • Workload Cluster: v1.21.x -> v1.27.x

As of what to do with versions < 1.25, I would suggest checking what they did in the capi repo. They had to face this same issue since there are tests for 1.21 -> 1.24

thanks @g-gaston . good suggestion

I checked the capi code just now. It does have a method to determine the registry by kubernetes version.
https://github.com/kubernetes-sigs/cluster-api/blob/v1.2.12/internal/util/kubeadm/kubeadm.go

maybe the imageRepository setting should be removed in yaml files.
let me test it.

@weizhouapache weizhouapache changed the title e2e: revert to k8s.gcr.io from registry.k8s.io e2e: remove imageRepository in yaml files May 8, 2023
@weizhouapache
Copy link
Collaborator Author

weizhouapache commented May 8, 2023

@weizhouapache @vishesh92 @davidjumani perhaps we should look at and update the capc templates? /cc @jweite-amazon @g-gaston @chrisdoherty4 - what k8s version is preferred/used these days for your use-case?

@rohityadavcloud
Ideally we would test the versions supported by the latest version of cluster-api So in this case:

ok_hand Kubernetes version support

  • Management Cluster: v1.23.x -> v1.27.x [1]
  • Workload Cluster: v1.21.x -> v1.27.x

As of what to do with versions < 1.25, I would suggest checking what they did in the capi repo. They had to face this same issue since there are tests for 1.21 -> 1.24

thanks @g-gaston . good suggestion

I checked the capi code just now. It does have a method to determine the registry by kubernetes version. https://github.com/kubernetes-sigs/cluster-api/blob/v1.2.12/internal/util/kubeadm/kubeadm.go

maybe the imageRepository setting should be removed in yaml files. let me test it.

verified OK with K8S 1.22/1.23/1.24.

I have updated this PR to remove all imageRepository setting in yaml files for e2e testing.
cc @rohityadavcloud @g-gaston @hrak

@hrak
Copy link
Contributor

hrak commented May 8, 2023

Nice, thanks for checking @weizhouapache, good solution 👍

@g-gaston
Copy link
Contributor

g-gaston commented May 8, 2023

Love this!
/lgtm

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

g-gaston commented May 8, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 8, 2023
@weizhouapache weizhouapache changed the title e2e: remove imageRepository in yaml files e2e: set imageRepository to null in yaml files May 8, 2023
@weizhouapache
Copy link
Collaborator Author

Update:
set imageRepository to null in yaml files, so it will be much easier if we use private registry in CI/CD testing.

@g-gaston
Copy link
Contributor

g-gaston commented May 8, 2023

Update: set imageRepository to null in yaml files, so it will be much easier if we use private registry in CI/CD testing.

@weizhouapache why is it easier this way? tbh I prefer not setting the field 😄

@weizhouapache
Copy link
Collaborator Author

Update: set imageRepository to null in yaml files, so it will be much easier if we use private registry in CI/CD testing.

@weizhouapache why is it easier this way? tbh I prefer not setting the field smile

@g-gaston
I prefer to use private registry in CI/CD testing, because of

  • download speed from public registry
  • (possible) connection limit

then I just need to run a shell command
sed -i "s/imageRepository:/imageRepository: <private registry domain>/g" ***.yaml

if the clusterConfiguration/imageRepository block is removed, it will be a bit complicated.
maybe it can also be done by other configurations (kubeadmin or clusterctl, etc), I have not figured out yet.

@g-gaston
Copy link
Contributor

g-gaston commented May 9, 2023

I feel like a different tool will be better for that, like kustomize.
Also, are we using a private registry right now or is this a future usecase?

If using sed makes it easier in the short term, could we do this:

   clusterConfiguration:
      # Setting an empty repository lets cluster-api pick the right registry for the k8s version.
      imageRepository: ""

@weizhouapache weizhouapache changed the title e2e: set imageRepository to null in yaml files e2e: set imageRepository to "" in yaml files May 10, 2023
The image repository has been changed to registry.k8s.io by commit 8c1e614

However, with registry.k8s.io, the control plane vm cannot be booted to Ready state due to error below
```
    ubuntu@disk-offering-gzojvb-control-plane-gwbnt:~$ tail -f /var/log/cloud-init-output.log
    [2023-05-08 11:09:23] [preflight] You can also perform this action in beforehand using 'kubeadm config images pull'
    [2023-05-08 11:09:28] error execution phase preflight: [preflight] Some fatal errors occurred:
    [2023-05-08 11:09:28]   [ERROR ImagePull]: failed to pull image registry.k8s.io/coredns:v1.8.4: output: time="2023-05-08T11:09:28Z" level=fatal msg="pulling image: rpc error: code = NotFound desc = failed to pull and unpack image \"registry.k8s.io/coredns:v1.8.4\": failed to resolve reference \"registry.k8s.io/coredns:v1.8.4\": registry.k8s.io/coredns:v1.8.4: not found"
    [2023-05-08 11:09:28] , error: exit status 1
```

this is same as kubernetes/kubeadm#2761
The new registry should be supported in k8s 1.25+. However, we still use 1.22/1.23/1.24 templates, so we need to use k8s.gcr.io

setting the image respository to "" so that capi/kubeadm will determine the default repository by kubernetes version.
@weizhouapache weizhouapache force-pushed the e2e-fix-control-plane-vm branch from c197642 to 5685e99 Compare May 10, 2023 07:15
@weizhouapache
Copy link
Collaborator Author

I feel like a different tool will be better for that, like kustomize. Also, are we using a private registry right now or is this a future usecase?

@g-gaston
not right now. just an idea

If using sed makes it easier in the short term, could we do this:

   clusterConfiguration:
      # Setting an empty repository lets cluster-api pick the right registry for the k8s version.
      imageRepository: ""

good point, updated and squashed this PR. thanks

@chrisdoherty4
Copy link
Member

Am I right to say this requires the user to run some sort of replacement command before they can run the E2E tests (at least for newer Kubernetes versions)?

what k8s version is preferred/used these days for your use-case?

@rohityadavcloud as @g-gaston said, we should test everything CAPI supports. I don't think this project should focus on testing consumer combinations.

@g-gaston
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2023
@weizhouapache
Copy link
Collaborator Author

/lgtm

@g-gaston can you approve it as well ? thanks

@g-gaston
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: g-gaston, weizhouapache

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 May 11, 2023
@k8s-ci-robot k8s-ci-robot merged commit d24306e into kubernetes-sigs:main May 11, 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. 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants