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
Ability to set up additional, bigger nodes during tests #83352
Conversation
Hi @jkaniuk. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
/ok-to-test |
/hold |
4b80388
to
9bcc848
Compare
/hold cancel |
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.
Some minor comments, but overall LGTM
cluster/gce/util.sh
Outdated
"${ENABLE_IP_ALIASES:-}" \ | ||
"${IP_ALIAS_SIZE:-}") | ||
|
||
for ((i=1; i<=${NUM_ADDITIONAL_NODES}; i++)); do |
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.
An alternative would be to create a new instance-template from the already existing one and override only machine type and then create a MIG, but I'm not sure if it's possible to create an instance-template from another one.
Another advantage is that MIG will scale better if someone wants to have a big number of additional nodes (e.g. 500).
But overall, this looks good to me, it should work for our use case, so it's a good start. We can improve in future.
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.
I actually think that we should create MIGs.
Also, I would really like to avoid such huge code duplication. Can't we reuse: "create-nodes-template" and "create-*-nodes" functions?
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.
Rewritten to use MIGs
3e62e04
to
69455a4
Compare
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.
/hold
cluster/gce/util.sh
Outdated
"${ENABLE_IP_ALIASES:-}" \ | ||
"${IP_ALIAS_SIZE:-}") | ||
|
||
for ((i=1; i<=${NUM_ADDITIONAL_NODES}; i++)); do |
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.
I actually think that we should create MIGs.
Also, I would really like to avoid such huge code duplication. Can't we reuse: "create-nodes-template" and "create-*-nodes" functions?
69455a4
to
0d12e9e
Compare
0d12e9e
to
a1b5578
Compare
/test pull-kubernetes-kubemark-e2e-gce-big |
cluster/gce/util.sh
Outdated
fi | ||
|
||
if [[ -n "${ADDITIONAL_MACHINE_TYPE:-}" && "${NUM_ADDITIONAL_NODES:-}" -gt 0 ]]; then | ||
echo "Creating ${NUM_ADDITIONAL_NODES} special nodes with machine-type ${ADDITIONAL_MACHINE_TYPE}" |
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.
nit: what if NUM_ADDITIONAL_NODES > NUM_NODES ?
/hold cancel One minor comment |
a1b5578
to
46e7a14
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jkaniuk, wojtek-t 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 |
Network policy provider pods requests more than 1 core in 100+ nodes clusters.
kubernetes/test-infra#13709
/sig scalability
/cc mm4tt
/cc wojtek-t
What type of PR is this?
/kind feature
What this PR does / why we need it:
Network policy provider pods requests more than 1 core in 100+ nodes clusters.
Does this PR introduce a user-facing change?: