-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Remove CI image pull and tagging for kubeadm injection script #6590
🌱 Remove CI image pull and tagging for kubeadm injection script #6590
Conversation
/lgtm |
lgtm |
@jsturtevant We needed the wget + import + tag of those images as they were not available in a registry (at least for CAPO and CAPA too afaik). It's interesting that for CAPZ it works with image pull, e.g. here is a successfull image pull:
Note: The download script fails in CAPZ but this doesn't fail the entire bootstrap process. Looks like the CAPO tests are currently failing since the change in image names:
In general the change looks fine, but I think we have to use the old/new registry depending on the Kubernetes version, otherwise jobs using <v1.25 will fail (if there are any). @dims Do you know why those images might be available for CAPZ but not for CAPO? (I read about the cloud-local redirects, but I don't know if that explains it) Note: The logs above show different registries, but I"m not sure if that is the actual issue (maybe just +/- redirect), given that we have kubeadm image pulls in both cases and it works in one case but not the other:
|
cc @mdbooth (for CAPO) |
@ameukam @BenTheElder can you please peek? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
/hold for the above |
something might be wrong in the configuration on openstack. Kubeadm in Azure is pulling: notice the difference between |
Yeah,
An image with a tag like this won't be in k8s.gcr.io, tags there have to be promoted in the k8s.io repo, only release tags are present. |
+1 |
Kubeadm has the ability to automatically pull the CI based images if it is a CI binary itself so we don't need to pull and tag these images but we do need to get and replace the correct binaries.
Looping in CAPA in case they are (still/also) using this script cc @pydctw @richardcase |
75aaea2
to
30ba31a
Compare
Thank you! /lgtm It would be good to wait until next week with merge if possible. Just to get feedback from CAPO/CAPA (one of the CAPO maintainers at least is out this week). Otherwise I'm fine with just iterating, CAPO tests are currently broken anyway so this won't make it worse :). |
/retitle Remove CI image pull and tagging for kubeadm injection script |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per comments in the thread above
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini, vincepri 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 |
/hold |
|
CAPA k8s-main conformance test is broken since kubernetes/kubernetes#109938 is merged, not sure how it works for CAPZ but we were using the pulled images here. I will need to further test if just changing the registry here works instead of removing it. |
For unblocking CAPZ, how about just changing k8s.gcr.io to registry.k8s.io in this PR so that CAPZ script does not fail? @jsturtevant @CecileRobertMichon |
capz isn't blocked on this, we haven't taken a dependency on this e2e helper script yet but were considering it after this merged.
This is what I did in the initial version of this PR but it was discover that the logic for pull/tagging images is not required so switch to removing it #6590 (comment) |
CAPA also doesn't pin the registry to k8s.gcr.io but still it stopped working for us, need to collect the logs. |
@sbueringer follow up on #6590 (comment)
Without importing and tagging part, ci kubeadm version fails to pull images. Errors I see:
After changing the registry to the new one in this PR, it works. So looks like we still need to import and tag images. I don't know how CAPZ test does not fail with this kubeadm error. |
Looks like there is something wrong with kubeadm somehow. It works in CAPZ because kubeadm detects the CI version and it pulls images from the CI registry automatically, e.g. Looks like in CAPA kubeadm somehow tries to pull images from the (new) production registry (registry.k8s.io).
The problem is that the script then only works with >= v1.25. The older versions are using the old registry. |
I think I found it. CAPZ sets a
kubeadm is looking for the ci prefix there, then it automatically uses the test registry/imageRepository (gcr.io/k8s-staging-ci-images) |
Good find @sbueringer. @sedefsavas can CAPA add the ci/ prefix too? Then we don't need additional script lines to pull the images and it works for all versions. CAPO should also do the same. |
Good find!! We will change it in CAPA. /lgtm |
Okay I think we can go ahead and merge it. Thx everyone! /hold cancel |
What this PR does / why we need it:
After the switch from k8s.gcr.io to registry.k8s.io (see kubernetes/kubernetes#109938 for details), the CI jobs are outputting the following when trying to extract the ci images:
https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/capz-conformance-master/1532465763934277632
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Partially Fixes kubernetes-sigs/cluster-api-provider-azure#2351
other info
I found our jobs are not failing since kubeadm is smart enough because we give it the
kubernetesversion
in the job config, even without this to pull the correct images so our jobs are still passing.See https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/9e793b42f79b65cbc830e711dce63a2c1e7c82c0/templates/test/ci/cluster-template-prow-ci-version-windows-containerd-2022.yaml#L90
Is there a reason to pull these images?