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

Add exclued node flag for load image command #3688

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Monokaix
Copy link

When using kind load command I want to exclude some nodes,for example,I want to load to all nodes except master node,because it has taint and is just a control plane and I don't want load image to this node,so add a new flag just exclude the specific node would be helpful.

Copy link

linux-foundation-easycla bot commented Jul 18, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Monokaix / name: Marty Mcfly (08092b1)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Monokaix
Once this PR has been reviewed and has the lgtm label, please assign bentheelder 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 k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 18, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @Monokaix!

It looks like this is your first PR to kubernetes-sigs/kind 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kind has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@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 Jul 18, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @Monokaix. 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 the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 18, 2024
Signed-off-by: Monokaix <changxuzheng@huawei.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 18, 2024
if !ok {
return fmt.Errorf("unknown node: %q", name)
}
candidateNodes = nodeutils.RemoveNode(name, candidateNodes)
Copy link
Member

Choose a reason for hiding this comment

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

the end result candidateNodes is processed with too many loops.
instead try using maps or a smarter way to process the excluded nodes.

for example, only add a node to candiateNodes if it's not in the excluded nodes.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your reply! Will update later.

if !ok {
return fmt.Errorf("unknown node: %q", name)
}
selectedNodes = nodeutils.RemoveNode(name, selectedNodes)
Copy link
Member

Choose a reason for hiding this comment

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

same problem here.

@aojea
Copy link
Contributor

aojea commented Jul 18, 2024

you can already specify the ones you want to load the image, why to use the negative too?

@BenTheElder
Copy link
Member

Yeah ... not a fan of adding exclusive / confusing flag pairs, you can currently specify the set of nodes to load to with an existing flag

@Monokaix
Copy link
Author

you can already specify the ones you want to load the image, why to use the negative too?

Because when there are many kind nodes and I just want not loading to a few nodes, without this flag I have to specify most nodes, if we can support exclude semantics, it would be very convenient: )

@BenTheElder
Copy link
Member

We generally ask that features like this be discussed as a design first, so these discussions are resolved up front.

The problem with adding every convenient little feature is bloat and poor test coverage / support ability.

https://kind.sigs.k8s.io/docs/contributing/getting-started/

How many nodes is "many nodes"? We generally don't expect kind clusters to have many, in nearly all cases they should have one. For some specific use cases they might have a few.

@Monokaix
Copy link
Author

We generally ask that features like this be discussed as a design first, so these discussions are resolved up front.

The problem with adding every convenient little feature is bloat and poor test coverage / support ability.

https://kind.sigs.k8s.io/docs/contributing/getting-started/

How many nodes is "many nodes"? We generally don't expect kind clusters to have many, in nearly all cases they should have one. For some specific use cases they might have a few.

I can truly understand that, there are three cases I have to create a kind cluster with not only one node.

  1. In some benchmark case, we need hundreds or thousands of nodes to do performance check.

  2. A cloud native scheduler project's e2e test needs at least four work nodes and one control plane.

  3. I deployed kubeflow with kind, because kubeflow has many components which occupied lots of resources, I have to create six nodes to meet the resource requirements.

As kind becomes more and more popular, users use kind to deploy more than one node should be considered as a normal case:)

@stmcginnis
Copy link
Contributor

deploy more than one node should be considered as a normal case

That is quite normal, and is used regularly for testing. Usually that is a small number of nodes though, and anything scaling beyond that starts to be something better done with other tools than kind.

It sounds like you've been able to get an impressive amount of scale for something not intended to scale that far. It would be interested to see how you've gotten around some of the limitations like inotify, etc. But I think type of setup would still be a very rare thing, so adding a (potentially) confusing option that is the inverse of an existing option may not be worth the maintenance and confusion it would cause.

It may be better to add a step to query the cluster for the nodes you care about, then use that as input for the --nodes argument.

@BenTheElder
Copy link
Member

BenTheElder commented Jul 22, 2024

In some benchmark case, we need hundreds or thousands of nodes to do performance check.

You're running kind with 100s or 1000s of nodes to do benchmarks?? That's a first 😅

Depending on what you're doing, you might also want to consider alternatives, I'd love to hear more about this 😅

Most users cannot get anywhere near this scale to work because it consumes a lot of resources on the host machine to do this, on many dimensions. What sort of environment are you running this in?

Do you actually need real nodes? Would kwok / kubemark /... make more sense?

As kind becomes more and more popular, users use kind to deploy more than one node should be considered as a normal case:)

Well, it's quite common, but in many many cases it's not actually a very good fit for the task (e.g. kubemark / kwok may make more sense), and a lot of the time we find users pre-maturely adopting multiple nodes for misguided reasons and being frustrated with the experience, so I usually check if a single node or something else would be a better fit for their needs.

Case in point:

I deployed kubeflow with kind, because kubeflow has many components which occupied lots of resources, I have to create six nodes to meet the resource requirements.

That's not actually an appropriate use of kind nodes, because adding kind nodes DOES NOT actually add hardware resources, it only over-reports them. You should either adjust the resource requests for the workloads or run kind on a larger underlying host.

Multiple kind nodes is a solution to testing multi-node behaviors in distributed systems (e.g. kubernetes networking implementations, some of the core project), not a solution to needing more resources.

@Monokaix
Copy link
Author

Autually when I deploy a cluster with one control plane and four workers node to do e2e test about scheduler's behavior,I have to specify four nodes after --nodes option, if we support exlude node, only one node name specified is ok,I think this would be very convenient.

@aojea
Copy link
Contributor

aojea commented Jul 23, 2024

exclude node

$ kind get nodes | grep -v kind-control-plane
kind-worker
kind-worker2

convert to comma separated list to pass to kind command

$ kind get nodes | grep -v kind-control-plane | paste -sd, -
kind-worker,kind-worker2

voila

$ kind load docker-image busybox:stable --nodes $(kind get nodes | grep -v kind-control-plane | paste -sd, -)
Image with ID: sha256:65ad0d468eb1c558bf7f4e64e790f586e9eda649ee9f130cd0e835b292bbc5ac already present on the node kind-worker but is missing the tag docker.io/library/busybox:stable. re-tagging...
Image with ID: sha256:65ad0d468eb1c558bf7f4e64e790f586e9eda649ee9f130cd0e835b292bbc5ac already present on the node kind-worker2 but is missing the tag docker.io/library/busybox:stable. re-tagging...

@BenTheElder
Copy link
Member

Autually when I deploy a cluster with one control plane and four workers node to do e2e test about scheduler's behavior,I have to specify four nodes after --nodes option, if we support exlude node, only one node name specified is ok,I think this would be very convenient.

It sounds like your actually use case is to exclude by role?

(Again, this is why we prefer to discuss features on a issue before implementation PRs per our contribution guide)

In the meantime you can make that snippet portable across clusters and shorter by grep -v control-plane

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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants