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

Do a kubectl get nodes after turning up the e2e cluster. #34128

Merged
merged 1 commit into from
Oct 13, 2016
Merged

Do a kubectl get nodes after turning up the e2e cluster. #34128

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions hack/e2e.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,20 @@ func run(deploy deployer) error {
if err := xmlWrap("Up", deploy.Up); err != nil {
return fmt.Errorf("starting e2e cluster: %s", err)
}
if *dump != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Do we maybe want to pull this out to one level higher and run it in either the case that -up or -test is selected, then just eliminate the kubectl version check for -test? i.e. move it to about line 215 in the original and just say:

if *up || *test {
  // get nodes. If there's something wrong, we need to get out of dodge after this.
  // if *dump is set, send the output to a file
}

Copy link
Member

Choose a reason for hiding this comment

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

Actually, that may not be the best for large clusters, where get nodes -oyaml might be dicey in the first place. Hrm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't consider large clusters. How bad is it?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for leaving this PR for so long. I don't think it's too terrible, but the entire yaml output for all nodes is a pretty big chunk. On the whole this change is probably worth it but might be something to watch on the large cluster projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. No worries for the delay. I'm not in a rush.

Copy link
Contributor

Choose a reason for hiding this comment

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

For large cluster it's bad. Running kubectl get nodes on 3000 Node cluster makes logs even harder to read. Is there an easy way to disable it for some suites? @spxtr

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping @spxtr @zmerlynn - can we please limit it to 10 Nodes or something. It really hurts during large cluster testing.

cc @wojtek-t

cmd := exec.Command("./cluster/kubectl.sh", "--match-server-version=false", "get", "nodes", "-oyaml")
b, err := cmd.CombinedOutput()
if *verbose {
log.Printf("kubectl get nodes:\n%s", string(b))
}
if err == nil {
if err := ioutil.WriteFile(filepath.Join(*dump, "nodes.yaml"), b, 0644); err != nil {
errs = appendError(errs, fmt.Errorf("error writing nodes.yaml: %v", err))
}
} else {
errs = appendError(errs, fmt.Errorf("error running get nodes: %v", err))
}
}
}

if *checkLeakedResources {
Expand Down