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

[1.9] chore: skip push event for branches of dependabot. #4976

Merged

Conversation

liangyuanpeng
Copy link
Contributor

What type of PR is this?

/kind cleanup
What this PR does / why we need it:

cherry pick #4762 to release-1.9, after #4932, dependabot will create PR for release branchs, and we need to skip some push event for dependabot.

releated:

/assign @RainbowMango

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@karmada-bot karmada-bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 23, 2024
@liangyuanpeng liangyuanpeng changed the title chore: skip push event for branches of dependabot. [1.9] chore: skip push event for branches of dependabot. May 23, 2024
@karmada-bot karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 23, 2024
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Just reminder, you can use hack/cherry_pick_pull.sh to do this work, probably easier.

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label May 23, 2024
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 23, 2024
@RainbowMango
Copy link
Member

The test seems unrelated:

I0523 07:13:57.402043   29341 check.go:42] Waiting for APIService(v1alpha1.cluster.karmada.io) condition(Available), will try
I0523 07:13:58.402499   29341 check.go:42] Waiting for APIService(v1alpha1.cluster.karmada.io) condition(Available), will try
I0523 07:13:58.404562   29341 check.go:42] Waiting for APIService(v1alpha1.cluster.karmada.io) condition(Available), will try
F0523 07:13:58.404578   29341 deploy.go:121] timed out waiting for the condition

/retest

@liangyuanpeng
Copy link
Contributor Author

use hack/cherry_pick_pull.sh to do this work, probably easier.

Yes, i run this scripts first and it's failed, have not see any conflict, so i skip cherry pick with the scripts.

...
Applying: chore: skip push event for branches of dependabot.
Using index info to reconstruct a base tree...
M       .github/workflows/ci.yml
M       .github/workflows/cli.yaml
error: patch failed: .github/workflows/ci-image-scanning.yaml:1
error: .github/workflows/ci-image-scanning.yaml: patch does not apply
error: Did you hand edit your patch?
...

@liangyuanpeng
Copy link
Contributor Author

Tested on kubernetes 1.29 and karmada-aggregated-apiserver have some error about APIPriorityAndFairness

Error: invalid argument "APIPriorityAndFairness=false" for "--feature-gates" flag: cannot set feature gate APIPriorityAndFairness to false, feature is locked to true

@liangyuanpeng
Copy link
Contributor Author

try again
/retest

@liangyuanpeng
Copy link
Contributor Author

liangyuanpeng commented May 23, 2024

seems like karmada-aggregated-apiserver is using the latest image and not the local build image from release-1.9

...
Image: "docker.io/karmada/karmada-aggregated-apiserver:latest" with ID "sha256:c520dd5ad28b2c00e23a2864b5491d6a242ed70f5cc0e56282b16f4265fa6a2e" not yet present on node "karmada-host-control-plane", loading...
Start init karmada control plane...
...
$ docker exec karmada-host-control-plane crictl images --digests
IMAGE                                            TAG                  DIGEST              IMAGE ID            SIZE
docker.io/karmada/karmada-aggregated-apiserver   latest               91dfe34aa3cef       41ae560c9c94d       43MB
docker.io/library/import-2024-05-23              <none>               7ff1525d0a91a       c520dd5ad28b2       101MB

docker.io/karmada/karmada-aggregated-apiserver:latest (91dfe34aa3cef ) is the latest image and docker.io/library/import-2024-05-23(7ff1525d0a91a) is the local build image from release-1.9, have not find the reason for what happen. (why only happing on release-1.9 )

@liangyuanpeng
Copy link
Contributor Author

liangyuanpeng commented May 23, 2024

it's successed when i add ImagePullPolicy PullIfNotPresent for karmada-aggregated-apiserver

		Containers: []corev1.Container{
			{
				Name:            karmadaAggregatedAPIServerDeploymentAndServiceName,
				Image:           i.karmadaAggregatedAPIServerImage(),
				Command:         command,
				ImagePullPolicy: corev1.PullIfNotPresent,
...

@karmada-bot karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label May 23, 2024
@RainbowMango
Copy link
Member

seems like karmada-aggregated-apiserver is using the latest image and not the local build image from release-1.9

Oh? I'm afraid there is something wrong.

it's successed when i add ImagePullPolicy PullIfNotPresent for karmada-aggregated-apiserver

Can you explain it a little bit? I don't get how this fixed the previous problem.

@zhzhuang-zju
Copy link
Contributor

Can you explain it a little bit? I don't get how this fixed the previous problem.

This is actually two questions:

  • why karmada-aggregated-apiserver is using the latest image
    Refer to

    # make images
    export VERSION="latest"
    export REGISTRY="docker.io/karmada"
    make images GOOS="linux" --directory="${REPO_ROOT}"
    ${BUILD_PATH}/karmadactl init --kubeconfig=${KUBECONFIG_PATH}/${HOST_CLUSTER_NAME}.config \
    --karmada-controller-manager-image="${REGISTRY}/karmada-controller-manager:${VERSION}" \
    --karmada-scheduler-image="${REGISTRY}/karmada-scheduler:${VERSION}" \
    --karmada-webhook-image="${REGISTRY}/karmada-webhook:${VERSION}" \
    --karmada-aggregated-apiserver-image="${REGISTRY}/karmada-aggregated-apiserver:${VERSION}" \
    --karmada-data=${HOME}/karmada \
    --karmada-pki=${HOME}/karmada/pki \
    --crds=./crds.tar.gz
    and
    flags.StringVarP(&opts.KarmadaAggregatedAPIServerImage, "karmada-aggregated-apiserver-image", "", kubernetes.DefaultKarmadaAggregatedAPIServerImage, "Karmada aggregated apiserver image")
    ,
    the value of KarmadaAggregatedAPIServerImage will be docker.io/karmada/karmada-aggregated-apiserver:latest.
    Refer to
    DefaultKarmadaAggregatedAPIServerImage = fmt.Sprintf("docker.io/karmada/karmada-aggregated-apiserver:%s", releaseVer.ReleaseVersion())
    and
    func (i *CommandInitOption) karmadaAggregatedAPIServerImage() string {
    if i.ImageRegistry != "" && i.KarmadaAggregatedAPIServerImage == DefaultKarmadaAggregatedAPIServerImage {
    return i.ImageRegistry + "/karmada-aggregated-apiserver:" + karmadaRelease
    }
    return i.KarmadaAggregatedAPIServerImage
    ,
    since i.KarmadaAggregatedAPIServerImage != DefaultKarmadaAggregatedAPIServerImage, the imageRef of karmada-aggregated-apiserver will be docker.io/karmada/karmada-aggregated-apiserver:latest, rather than docker.io/karmada/karmada-aggregated-apiserver:release-1.9.
    When the tag of image is latest, ImagePullPolicy will be default to always, that means Kubernetes will always pull the latest version of the image from the container registry. So the e2e environment doesn't actually use the locally built image.

  • why adding ImagePullPolicy IfNotPresent for karmada-aggregated-apiserver works?
    when the ImagePullPolicy of karmada-aggregated-apiserver is IfNotPresent, the e2e environment will use the local image built from release-1.9 branch code.

Besides, we should add imagePullPolicy IfNotPresent for all karmada components, not just component karmada-aggregated-apiserver. They both have the same problem, except that the component "karmada-agregated-apiserver" was exposed due to a breaking change.

Refer to #4815, changes have been made in the master branch, we just need to cherry-pick them to release-1.9.
cc @RainbowMango @liangyuanpeng

@liangyuanpeng
Copy link
Contributor Author

liangyuanpeng commented May 27, 2024

Thanks for your time, i have not into it again yet, actually i believe that i advocated not using the latest image tag, but i have not found the history.I think a better approach is to update the tags used by the test image rather than backport a new features. WDYT?

like use tag of test or e2etest and not latest.
@zhzhuang-zju

@RainbowMango
Copy link
Member

Given that we will fix the CI issue on the release branch, I am supposed to fix it the way we did on the master branch.

That is backporting the #4815 to release-1.9. But as @liangyuanpeng mentioned, we usually don't backport features onto release branches, so, I think we can backport part of #4815, just change the default ImagePullPolicy to PullIfNotPresent(not bring the new flag --image-pull-policy) for all of the components not only the karmada-aggregate-apiserver.

@zhzhuang-zju
Copy link
Contributor

I think we can backport part of #4815, just change the default ImagePullPolicy to PullIfNotPresent(not bring the new flag --image-pull-policy) for all of the components not only the karmada-aggregate-apiserver.

get it

@RainbowMango
Copy link
Member

Hi @liangyuanpeng, you can rebase this PR now.

Co-authored-by: zhzhuang-zju <[email protected]>
Co-authored-by: RainbowMango <[email protected]>
Signed-off-by: Lan Liang <[email protected]>
@liangyuanpeng liangyuanpeng force-pushed the cherrypick_1.9_4762 branch from 44d61a2 to 021f00b Compare May 27, 2024 06:38
@RainbowMango
Copy link
Member

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label May 27, 2024
@karmada-bot karmada-bot merged commit 93a403a into karmada-io:release-1.9 May 27, 2024
12 checks passed
@pulsarsigs-bot pulsarsigs-bot deleted the cherrypick_1.9_4762 branch May 27, 2024 07:28
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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. 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.

4 participants