Skip to content

E2E using gce first pass#31

Merged
aveshagarwal merged 1 commit intokubernetes-sigs:masterfrom
ravisantoshgudimetla:e2e_with_travis_gcloud
Mar 19, 2018
Merged

E2E using gce first pass#31
aveshagarwal merged 1 commit intokubernetes-sigs:masterfrom
ravisantoshgudimetla:e2e_with_travis_gcloud

Conversation

@ravisantoshgudimetla
Copy link
Contributor

@ravisantoshgudimetla ravisantoshgudimetla commented Oct 30, 2017

@aveshagarwal I am able to create a 3 node cluster in gce with pod being evicted for lownodeutilization case. Please review when you find time. I believe, travis ci might complain because of encrypted file(client.json.enc) which will probably not be decrypted on this repo.

Some changes:

  • Now instead of running unit-tests, travis will run unit-tests + e2e's when using make test.
  • Before running make test, we need to ensure that gcloud_create_cluster.sh has been run. I have included that in the run-tests.sh(file renamed from run-unittests.sh)

Sample Output after running e2e:

evicting pods from node %#v with usage: %#v descheduler-b148f4c6-bd94-11e7-8e84-c85b76333877 map[cpu:2264 memory:263.78542244169597 pods:81.81818181818181]

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 30, 2017
@aveshagarwal
Copy link
Contributor

@ravisantoshgudimetla thanks ravi, will look into it soon.

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the e2e_with_travis_gcloud branch 4 times, most recently from c827f92 to 897f314 Compare October 31, 2017 19:10
@ravisantoshgudimetla ravisantoshgudimetla force-pushed the e2e_with_travis_gcloud branch 3 times, most recently from 0dbc5e8 to 3f549cf Compare November 3, 2017 16:51
@aveshagarwal
Copy link
Contributor

ok to test

1 similar comment
@ravisantoshgudimetla
Copy link
Contributor Author

ok to test

@aveshagarwal
Copy link
Contributor

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 3, 2017
@ravisantoshgudimetla ravisantoshgudimetla force-pushed the e2e_with_travis_gcloud branch 3 times, most recently from 7d28eaa to 27cc1cd Compare November 3, 2017 19:29
@ravisantoshgudimetla
Copy link
Contributor Author

ok to test

@ravisantoshgudimetla
Copy link
Contributor Author

retest this please

@ravisantoshgudimetla
Copy link
Contributor Author

/test all

@ravisantoshgudimetla
Copy link
Contributor Author

/test e2e

@openshift-ci-robot
Copy link

@ravisantoshgudimetla: you can't request testing unless you are a kubernetes-incubator member.

Details

In response to this:

/test e2e

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.

@aveshagarwal
Copy link
Contributor

@ravisantoshgudimetla this PR updates vendor dir, but where are changes to glide.lock file?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 14, 2018
@ravisantoshgudimetla
Copy link
Contributor Author

@aveshagarwal I removed the dependency on those vendored files. I copied the function which I need to the test so it should run independently.

@ingvagabund
Copy link
Contributor

/test e2e

1 similar comment
@ingvagabund
Copy link
Contributor

/test e2e

gcloud compute copy-files $E2E_GCE_HOME/kubeadm_preinstall.sh descheduler-$master_uuid:/tmp --zone=us-east1-b
gcloud compute copy-files $E2E_GCE_HOME/kubeadm_install.sh descheduler-$master_uuid:/tmp --zone=us-east1-b
gcloud compute copy-files $E2E_GCE_HOME/kubeadm_preinstall.sh descheduler-$node1_uuid:/tmp --zone=us-east1-b
gcloud compute copy-files $E2E_GCE_HOME/kubeadm_preinstall.sh descheduler-$node2_uuid:/tmp --zone=us-east1-b
Copy link
Contributor

Choose a reason for hiding this comment

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

WARNING: gcloud compute copy-files is deprecated. Please use gcloud compute scp instead. Note that gcloud compute scp does not have recursive copy on by default. To turn on recursion, use the --recurse flag.

Copy link
Contributor Author

@ravisantoshgudimetla ravisantoshgudimetla Mar 16, 2018

Choose a reason for hiding this comment

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

Thanks for catching it. These were not deprecated when I wrote them back in October. I can add a TODO or update them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@ingvagabund
Copy link
Contributor

/test e2e

@ingvagabund
Copy link
Contributor

/test e2e

1 similar comment
@ingvagabund
Copy link
Contributor

/test e2e

@ingvagabund
Copy link
Contributor

CI job merged.

@aveshagarwal
Copy link
Contributor

@ingvagabund @ravisantoshgudimetla is it ready for merge?

@ravisantoshgudimetla
Copy link
Contributor Author

@ingvagabund can you give a LGTM so that @aveshagarwal can merge?

@@ -0,0 +1,9 @@
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Currently the file is not needed. Installation of the gcloud is part of the JJ [1].

[1] https://github.com/openshift/aos-cd-jobs/blob/master/sjb/config/test_cases/ci-kubernetes-descheduler-e2e-gce.yml#L12-L23

@@ -0,0 +1,8 @@
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: The script is not invoked anywhere. Maybe make it part of some documentation that describes how to setup and run the descheduler e2e tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed if someone wants to run e2e using GCE from local system. This is similar to kube-up.sh in upstream.

if err != nil {
t.Errorf("Error listing node with %v", err)
}
leastLoadedNode := nodeList.Items[2]
Copy link
Contributor

@ingvagabund ingvagabund Mar 19, 2018

Choose a reason for hiding this comment

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

Nit: We should check if the cluster has at least 3 nodes. Otherwise this will panic.

Btw. how do you know the third node in the list is actually the least loaded node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nit: We should check if the cluster has at least 3 nodes. Otherwise this will panic.

We would have failed in gcloud_cluster_create.sh if we don't have 3 nodes. I thought failing early is better.

Btw. how do you the third node in the list is actually the least loaded node?

I have added a comment here.

https://github.com/kubernetes-incubator/descheduler/pull/31/files/e0d3b2663509dde3266daed9ecd0867f6fc4fdb0#diff-6413e1dbafd6dd63b5d907ea3ae6f6f9R151

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's put the comment here. Smth like:

// Assumption: kubeadm brings all the master components onto master node. So, the last node would have least utilization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@ingvagabund
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 19, 2018
@aveshagarwal aveshagarwal merged commit 0a7f14d into kubernetes-sigs:master Mar 19, 2018
ingvagabund pushed a commit to ingvagabund/descheduler that referenced this pull request May 29, 2020
briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022
…ith_travis_gcloud

E2E using gce first pass
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants