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

Fix: When creating a list of files and images with ./generate_list.sh, included ingress-nginx/kube-webhook-certgen image in the list. #11787

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

DearJey
Copy link

@DearJey DearJey commented Dec 11, 2024

What type of PR is this?
/kind bug

What this PR does / why we need it:

When list of files, images and local repogitry was created with ./generate_list.sh, ./manage-offline-files.sh and createing Kubernetes Cluster In offline construction with ingress_nginx_webhook_enabled: true, ingress-nginx-admission-* Pod of the Job created when is ImagePullBackOff and does not become READY.

  • kubectl describe po -n ingress-nginx ingress-nginx-admission-*
Events:
  Type     Reason     Age                  From               Message
  ----     ------     ----                 ----               -------
  Normal   Scheduled  2m16s                default-scheduler  Successfully assigned ingress-nginx/ingress-nginx-admission-patch-5hcxf to worker2
  Normal   Pulling    53s (x4 over 2m16s)  kubelet            Pulling image "192.168.122.155:5000/ingress-nginx/kube-webhook-certgen:v1.4.1"
  Warning  Failed     53s (x4 over 2m16s)  kubelet            Failed to pull image "192.168.122.155:5000/ingress-nginx/kube-webhook-certgen:v1.4.1": rpc error: code = NotFound desc = failed to pull and unpack image "192.168.122.155:5000/ingress-nginx/kube-webhook-certgen:v1.4.1": failed to resolve reference "192.168.122.155:5000/ingress-nginx/kube-webhook-certgen:v1.4.1": 192.168.122.155:5000/ingress-nginx/kube-webhook-certgen:v1.4.1: not found
  Warning  Failed     53s (x4 over 2m16s)  kubelet            Error: ErrImagePull
  Warning  Failed     38s (x6 over 2m16s)  kubelet            Error: ImagePullBackOff
  Normal   BackOff    23s (x7 over 2m16s)  kubelet            Back-off pulling image "192.168.122.155:5000/ingress-nginx/kube-webhook-certgen:v1.4.1"

./generate_list.sh creates an image list based on the downloads: section of /roles/kubespray-defaults/defaults/main/download.yml.

#!/bin/bash
set -eo pipefail

CURRENT_DIR=$(cd $(dirname $0); pwd)
TEMP_DIR="${CURRENT_DIR}/temp"
REPO_ROOT_DIR="${CURRENT_DIR%/contrib/offline}"

: ${DOWNLOAD_YML:="roles/kubespray-defaults/defaults/main/download.yml"}

mkdir -p ${TEMP_DIR}

# generate all download files url template
grep 'download_url:' ${REPO_ROOT_DIR}/${DOWNLOAD_YML} \
    | sed 's/^.*_url: //g;s/\"//g' > ${TEMP_DIR}/files.list.template

# generate all images list template
sed -n '/^downloads:/,/download_defaults:/p' ${REPO_ROOT_DIR}/${DOWNLOAD_YML} \
    | sed -n "s/repo: //p;s/tag: //p" | tr -d ' ' \
    | sed 'N;s#\n# #g' | tr ' ' ':' | sed 's/\"//g' > ${TEMP_DIR}/images.list.template

So, add the following to download.yml, registry.k8s.io/ingress-nginx/kube-webhook-certgen:v1.4.1 was added to the image list, and the above phenomenon was resolved.

  • vi /root/k8s-upgrade/kubespray-2.25.0/roles/kubespray-defaults/defaults/main/download.yml
downloads:
  ingress_nginx_kube_webhook_certgen:
    repo: "{{ ingress_nginx_kube_webhook_certgen_image_repo }}"
    tag: "{{ ingress_nginx_kube_webhook_certgen_image_tag }}"
    sha256: "{{ ingress_nginx_kube_webhook_certgen_digest_checksum | default(None) }}"
    groups:
      - kube_node
    when: ingress_nginx_webhook_enabled

Which issue(s) this PR fixes:

Fixes #11591

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix: When creating a list of files and images with ./generate_list.sh, `ingress-nginx/kube-webhook-certgen:v1.4.1` is also included in the list.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 11, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: DearJey
Once this PR has been reviewed and has the lgtm label, please assign yankay for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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
Copy link
Contributor

Hi @DearJey. 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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 11, 2024
@VannTen
Copy link
Contributor

VannTen commented Dec 12, 2024 via email

@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. labels Dec 12, 2024
sha256: "{{ ingress_nginx_kube_webhook_certgen_digest_checksum | default(None) }}"
groups:
- kube_node
when: ingress_nginx_webhook_enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be enabled: "{{ ingress_nginx_webhook_enabled }}" like the other in this file.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for comments!
I fixed it as descibed below.

enabled: "{{ ingress_nginx_webhook_enabled }}"

Dose this address your cocmments?
commit: d47e051

Comment on lines 1038 to 1039
groups:
- kube_node
Copy link
Contributor

Choose a reason for hiding this comment

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

This would download the webhook image on all nodes, this will slow down kubespray on large clusters.
This should be scoped to smaller subset of nodes 🤔

Copy link
Author

Choose a reason for hiding this comment

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

I've updated it as described below.

    groups:
      - "{{ ingress_nginx_nodeselector }}"

My goal was to determine the node to download based on the labell set by ingress_nginx_nodeselector in addons.yml.
Could you confirm if this task is correct?
commit: d47e051

ingress_nginx_kube_webhook_certgen:
repo: "{{ ingress_nginx_kube_webhook_certgen_image_repo }}"
tag: "{{ ingress_nginx_kube_webhook_certgen_image_tag }}"
sha256: "{{ ingress_nginx_kube_webhook_certgen_digest_checksum | default(None) }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop the d(None). We should have valid checksums for our images.

Copy link
Author

@DearJey DearJey Dec 18, 2024

Choose a reason for hiding this comment

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

I deleted (None) from this task in commit d47e051.

sha256: "{{ ingress_nginx_kube_webhook_certgen_digest_checksum }}"

@k8s-ci-robot
Copy link
Contributor

Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found 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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/contains-merge-commits kind/bug Categorizes issue or PR as related to a bug. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
3 participants