test/e2e/karmadactl_test.go: test taint command

mohamedawnallah commented 2 months ago

Description

In this commit, we add e2e tests for the karmadactl taint command to ensure it handles various scenarios correctly. The tests include:

Applying taints with different effects (NoSchedule, NoExecute) to a cluster.
Reapplying taints without the --overwrite flag to ensure an error is returned.
Overwriting existing taints using the --overwrite flag.
Removing taints from a cluster.
Running the taint command in --dry-run mode to ensure no actual changes are made.
Verifying that taints were not applied during --dry-run mode.
Ensuring the command returns an error for an invalid flag.
Ensuring the command returns an error when attempting to taint a non-existent cluster.

What type of PR is this?

/kind failing-test

Which issue(s) this PR fixes:

Part of #4544

Does this PR introduce a user-facing change?:

NONE

codecov-commenter commented 2 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 28.35%. Comparing base (721495d) to head (37af811). Report is 54 commits behind head on master.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #5244 +/- ## ========================================== + Coverage 28.26% 28.35% +0.09% ========================================== Files 632 632 Lines 43732 43774 +42 ========================================== + Hits 12359 12412 +53 + Misses 30472 30461 -11 Partials 901 901 ``` | [Flag](https://app.codecov.io/gh/karmada-io/karmada/pull/5244/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=karmada-io) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/karmada-io/karmada/pull/5244/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=karmada-io) | `28.35% <ø> (+0.09%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=karmada-io#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

XiShanYongYe-Chang commented 2 months ago

/cc @zhzhuang-zju @Vacant2333

mohamedawnallah commented 2 months ago

Thanks a lot, @zhzhuang-zju, for the feedback. It is much better now! 🙏 I will ask for a review when the GitHub CI tests finish.

mohamedawnallah commented 2 months ago

/retest

karmada-bot commented 2 months ago

@mohamedawnallah: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to [this](https://github.com/karmada-io/karmada/pull/5244#issuecomment-2248624901): >/retest Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.

zhzhuang-zju commented 2 months ago

seems like there's something wrong with the e2e test, I'll take a look at it first

zhzhuang-zju commented 2 months ago

@mohamedawnallah The modification should fix the previous e2e failure. However, I was wondering if it would be possible to create a new cluster and test karmadactl taint on the newly created cluster, which would have the following benefits:

No need to limit to serial, so we can reduce the e2e test time.
There will be no impact at all on existing environments and other e2e tests. Currently, If the e2e is run locally, it may have an impact on the local environment during the test.
There is no need to ensure that the taint is eventually removed. Currently if e2e test failed before ginkgo.By(fmt.Sprintf("Removing %s taint from the cluster", taint), func() {, taint may be left behind. After creating a new cluster, you just need to remove the cluster at the end.

mohamedawnallah commented 2 months ago

Thanks again @zhzhuang-zju for the feedback! I used the new cluster approach you suggested. I will ask for review when GitHub CI tests finish :)

zhzhuang-zju commented 2 months ago

@mohamedawnallah Good job~ cc @XiShanYongYe-Chang @Vacant2333

zhzhuang-zju commented 2 months ago

~~/lgtm~~ /lgtm cancel

mohamedawnallah commented 2 months ago

@zhzhuang-zju Apologies for the delayed response. I was busy submitting the proposal for the Karmada: Enhance Test Coverage for Search, Operator, and Webhook Components LFX project. I've incorporated your feedback and will request a review once the GitHub CI tests are complete :)

mohamedawnallah commented 2 months ago

I'm wondering if we should go to the previous approach since creating a cluster still require it to be run serially?

zhzhuang-zju commented 2 months ago

@zhzhuang-zju Apologies for the delayed response. I was busy submitting the proposal for the Karmada: Enhance Test Coverage for Search, Operator, and Webhook Components LFX project.

@mohamedawnallah No problem, just proceed at your own pace.

I'm wondering if we should go to the previous approach since creating a cluster still require it to be run serially?

Do you mean there is no need to create a new cluster? The karmadactl taint command can actually affect existing clusters, such as evicting pods. If I run e2e tests locally, it might disrupt my local member cluster, which is something we want to avoid.

BTW, you missed a modification, refer to https://github.com/karmada-io/karmada/pull/5244#discussion_r1699771953

mohamedawnallah commented 2 months ago

BTW, you missed a modification, refer to #5244 (comment)

Thanks for pointing that out. I missed it initially but have now addressed it. Will ask for review when CI tests finish!

mohamedawnallah commented 2 months ago

This may be off topic, but I usually run this command to execute a specific e2e test case. It used to run only the specified test, but now it's running all specs, which is quite annoying 😅.

Is there something I'm missing to ensure that this e2e test case only runs locally?

karmada@dev:~/Desktop/karmada/test/e2e$ ginkgo -focus "Karmadactl taint testing"
=== RUN   TestE2E
Running Suite: E2E Suite - /home/karmada/Desktop/karmada/test/e2e
=================================================================
Random Seed: 1722834546

Will run 226 of 226 specs

zhzhuang-zju commented 2 months ago

/retest

Is there something I'm missing to ensure that this e2e test case only runs locally?

that's weird, the output for the same command on my end is as follows

✗ ginkgo -focus "Karmadactl taint testing"
Running Suite: E2E Suite - /root/home/workspace/code/karmada/test/e2e
=====================================================================
Random Seed: 1722838703

Will run 1 of 227 specs

mohamedawnallah commented 2 months ago

Yeah found the issue ginkgo was installed 1.x.x release upgraded the major version to 2.x.x and it worked 👍

zhzhuang-zju commented 2 months ago

/lgtm cc @XiShanYongYe-Chang @Vacant2333

karmada-bot commented 2 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: XiShanYongYe-Chang

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: - ~~[test/OWNERS](https://github.com/karmada-io/karmada/blob/master/test/OWNERS)~~ [XiShanYongYe-Chang] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

karmada-io / karmada

test/e2e/karmadactl_test.go: test taint command #5244

Codecov Report