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.

Sign up for GitHub

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

Jump to bottom

test: Set num-nodes flag #16176

Merged
merged 1 commit into from
Jan 20, 2024
Merged

test: Set num-nodes flag #16176

merged 1 commit into from
Jan 20, 2024

Conversation

upodroid
Copy link
Member

There is a bug in the test framework where the -num-nodes flag must be explicitly set to avoid tests being skipped. It should be autocalculated but it is not working as intended.

image

Also, node-os-distro should be set to skip various tests that are OS specific in the kubernetes e2e test suite.

@dims

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 15, 2023
@upodroid
Copy link
Member Author

/cc @hakman

@upodroid
Copy link
Member Author

/retest

this is ready to be merged

rifelpet
rifelpet reviewed Dec 21, 2023
View reviewed changes
tests/e2e/pkg/tester/tester.go
@@ -203,6 +199,22 @@ func (t *Tester) addNodeIG() error {
ig = v
}
}
numNodes := int(*ig.Spec.MaxSize) // we assume that MinSize = Maxsize, this is true for e2e testing
Copy link
Member

Choose a reason for hiding this comment

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

This will be the size of the last nodes InstanceGroup. Should we instead sum the max size of all nodes InstanceGroups?

Copy link
Member Author

@upodroid upodroid Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No, the ginkgo wants the number of all nodes except the master/controplane IG. When we are using kops for e2e testing we assume there are exactly 2 IGs(one master and one worker). If we add the master ig to the count, you'll have an extra node that you can't schedule workloads onto without tweaking manifests to tolerate control plane taints

Copy link
Member

Choose a reason for hiding this comment

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

Ah I was thinking in the case of there being multiple nodes IGs. This tester code can be used in cluster configurations that have multiple node IGs so I don't know if your assumption is always true.

hakman
hakman requested changes Dec 21, 2023
View reviewed changes
tests/e2e/kubetest2-kops/deployer/up.go Outdated Show resolved Hide resolved
upodroid
upodroid commented Dec 21, 2023
View reviewed changes
tests/e2e/pkg/tester/skip_regex.go
@@ -26,7 +26,7 @@ import (
)

const (
skipRegexBase = "\\[Slow\\]|\\[Serial\\]|\\[Disruptive\\]|\\[Flaky\\]|\\[Feature:.+\\]|nfs|NFS|Gluster"
skipRegexBase = "\\[Slow\\]|\\[Serial\\]|\\[Disruptive\\]|\\[Flaky\\]|\\[Feature:.+\\]|nfs|NFS|Gluster|NodeProblemDetector"
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to adjust the prow job generator and it caused a big mess. We run ubuntu by default so it would need to be running most of the time. It is easier to skip, will open a PR in k/k now to park this behind a Feature tag

@upodroid upodroid mentioned this pull request Dec 21, 2023
Closed
@upodroid
Copy link
Member Author

/retest

We merged a PR in k/k to stop running NPD tests, the skip is needed for release branches

hakman
hakman reviewed Dec 23, 2023
View reviewed changes
tests/e2e/pkg/tester/tester.go Outdated
distro := CalculateDistroFromIG(ig)
if distro != "" {
klog.Infof("Setting --node-os-distro=%s", distro)
t.TestArgs += " --node-os-distro=" + distro
Copy link
Member

Choose a reason for hiding this comment

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

Don't remember, did we say we will move this to the generator for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

This is ready to merged

@upodroid upodroid changed the title set node-os-distro and num-nodes flags set num-nodes flags Jan 20, 2024
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 20, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2024
@hakman hakman changed the title set num-nodes flags test: Set num-nodes flag Jan 20, 2024
@hakman hakman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 20, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2024
@hakman
Copy link
Member

hakman commented Jan 20, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2024
@k8s-ci-robot k8s-ci-robot merged commit 2cd7993 into kubernetes:master Jan 20, 2024
21 of 22 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Jan 20, 2024
zetaab pushed a commit to zetaab/kops that referenced this pull request Jan 22, 2024
@upodroid @zetaab
set num-nodes flag automatically (kubernetes#16176)
83a1fdd 
Co-authored-by: upodroid <upodroid@users.noreply.github.com>
This was referenced Feb 5, 2024
Merged
Closed
@BrewTestBot BrewTestBot mentioned this pull request May 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rifelpet rifelpet rifelpet left review comments

@hakman hakman hakman approved these changes

@johngmyers johngmyers Awaiting requested review from johngmyers

@zetaab zetaab Awaiting requested review from zetaab

Assignees

@hakman hakman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.
4 participants
@upodroid @k8s-ci-robot @hakman @rifelpet