Delete all firewall rules (and optionally network) on GCE/GKE cluster teardown #34577

ixdy · 2016-10-11T23:54:22Z

Not entirely ready for review yet; I want to see what Jenkins thinks of this.

This change is

ixdy · 2016-10-12T03:04:55Z

k8s-ci-robot · 2016-10-12T03:29:11Z

Jenkins unit/integration failed for commit ef309fc118d82bf823af1385d67da4be418442d7. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

ixdy · 2016-10-12T21:53:34Z

I think this is ready for review. Anyone in @kubernetes/test-infra-maintainers want to take a look?

krzyzacy · 2016-10-12T22:04:28Z

cluster/gce/util.sh

@@ -1373,7 +1385,7 @@ function kube-down() {
 function get-replica-name() {
  echo $(gcloud compute instances list \
    --project "${PROJECT}" \
-    --zone "${ZONE}" \
+    --zones "${ZONE}" \


Assume the flag is renamed? I saw other scripts still using --zone and might need to be renamed

gcloud has been pitching a warning for a while now, yeah.

Yeah, most gcloud compute list calls use --zones now instead of --zone. #29851 fixed most of these, but missed this one (which was added by #29995).

I left this change as a separate commit to make that clear.

zmerlynn · 2016-10-12T22:06:52Z

cluster/gce/util.sh

@@ -1373,7 +1385,7 @@ function kube-down() {
 function get-replica-name() {
  echo $(gcloud compute instances list \
    --project "${PROJECT}" \
-    --zone "${ZONE}" \
+    --zones "${ZONE}" \


gcloud has been pitching a warning for a while now, yeah.

zmerlynn · 2016-10-12T22:10:41Z

cluster/gke/util.sh

+
+  # It's unfortunate that the $FIREWALL_SSH rule and network are created in
+  # kube-up, but we can only really delete them in test-teardown. So much for
+  # symmetry.


I don't understand why this statement is true. Actually, I don't understand why the gke provider structures it this way, and I'm scraping my memory for this at all. It's really awkward looking.

That one is at least partially your fault. 😀

#26664

Basically, I guess firewall deletion happens faster if the cluster has already been turned down, so we do that first. I'd prefer to do the network deletion in kube-down (since it's created in kube-up), but we can'd do that unless we delete the firewalls first, and some of those firewalls are created by test-setup, and so should logically only be deleted in test-teardown.

I don't really understand why we have both test-setup/kube-up and test-teardown/kube-down everywhere. But addressing that is possibly a larger task.

I just parallelized them there. We actually don't want to delete the firewall rules until after the container clusters delete, but I can't tell why we don't just delete them in kube-down after the call to container clusters delete, except that they're test-only. I suppose this is mostly irrelevant, though, because the GKE provider is really a synthetic provider for test.

zmerlynn · 2016-10-12T22:12:39Z

cluster/gce/config-default.sh

@@ -35,6 +35,7 @@ REGISTER_MASTER_KUBELET=${REGISTER_MASTER:-true}
 PREEMPTIBLE_NODE=${PREEMPTIBLE_NODE:-false}
 PREEMPTIBLE_MASTER=${PREEMPTIBLE_MASTER:-false}
 KUBE_DELETE_NODES=${KUBE_DELETE_NODES:-true}
+KUBE_DELETE_NETWORK=${KUBE_DELETE_NETWORK:-true}


Is this the right default for config-default.sh (vs config-test.sh?) (Same question for GKE provider.)

I could leave these as false for the default case. I'm not sure if there was a strong reason for leaking the firewalls/networks by default besides laziness (@thockin's word, not mine. :)

+1 for cleaning up after ourselves by default unless there is a good reason to do otherwise.

I can't think of a good reason to intentionally leak, except MAYBE that we don't know everything we leaked, and we don't want to nuke things accidentally...

switched to false in config-default.sh

alternately this could be false if $NETWORK is "default", true otherwise, but I don't know how much logic we want to embed here.

thockin · 2016-10-12T22:34:58Z

cluster/gce/util.sh

+  if [[ -n $(gcloud compute networks --project "${PROJECT}" describe "${NETWORK}" --format='value(name)' 2>/dev/null || true) ]]; then
+    if ! gcloud compute networks delete --project "${PROJECT}" --quiet "${NETWORK}"; then
+      echo "Failed to delete network '${NETWORK}'. Listing firewall-rules:"
+      gcloud compute firewall-rules --project "${PROJECT}" list --filter="network=${NETWORK}"


the command is gcloud compute firewall-rules, so I'm not sure what your comment means.

thockin · 2016-10-12T22:36:12Z

cluster/gce/util.sh

+
+function delete-network() {
+  if [[ -n $(gcloud compute networks --project "${PROJECT}" describe "${NETWORK}" --format='value(name)' 2>/dev/null || true) ]]; then
+    if ! gcloud compute networks delete --project "${PROJECT}" --quiet "${NETWORK}"; then


oh, this is a lot more heavy-handed than I expected. I do NOT think this should be teh default for non-test, because the default network is "default", and many people launch into an existing project. This PR will totally nuke my project, if I understand it.

ugh, misreading, nevermind

no, I was right - this will delete the default net. Taht would be bad

it'd probably fail to delete the network, assuming there are other things using the network (e.g. firewall rules with a different prefix).

in any case, you're right that this is a bit scary, so I've updated this so we only delete the network by default for test. (I still have this deleting the cluster firewall rules by default)

It won't, typically, because it takes all the dependencies being gone before the default network can be deleted using gcloud (unlike the webconsole, gcloud is just trying to delete this single object). That's not usually going to be the case because default has other fw rules in it to start (the default-allow rules, etc.).

That said, as I commented above, I don't think it should be the default for config-default.sh, simply because it'll usually error.

And I was commenting on a non-refreshed copy, apparently, so this thread looks all jacked up. LGTM.

spxtr · 2016-10-12T22:56:23Z

kubernetes/test-infra#812

thockin · 2016-10-12T23:01:20Z

nevermind, didn't read the echo

On Wed, Oct 12, 2016 at 3:47 PM, Jeff Grafton notifications@github.com
wrote:

@ixdy commented on this pull request.

In cluster/gce/util.sh
#34577:
+function delete-firewall-rules() {
for fw in $@; do

if [[ -n $(gcloud compute firewall-rules --project "${PROJECT}" describe "${fw}" --format='value(name)' 2>/dev/null || true) ]]; then
 gcloud compute firewall-rules delete --project "${PROJECT}" --quiet "${fw}" &
fi

done

kube::util::wait-for-jobs || {

echo -e "${color_red}Failed to delete firewall rules.${color_norm}" >&2

}
+}
+function delete-network() {
if [[ -n $(gcloud compute networks --project "${PROJECT}" describe "${NETWORK}" --format='value(name)' 2>/dev/null || true) ]]; then

if ! gcloud compute networks delete --project "${PROJECT}" --quiet "${NETWORK}"; then
 echo "Failed to delete network '${NETWORK}'. Listing firewall-rules:"
 gcloud compute firewall-rules --project "${PROJECT}" list --filter="network=${NETWORK}"
the command is gcloud compute firewall-rules, so I'm not sure what your
comment means.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34577, or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVDrFMfOO4iJCWxkq2cZytEOdhJY8ks5qzWN1gaJpZM4KUOgw
.

thockin · 2016-10-12T23:01:50Z

I will not have time to circle back to this - thanks for addressing my
concerns :)

On Wed, Oct 12, 2016 at 4:00 PM, Tim Hockin thockin@google.com wrote:

nevermind, didn't read the echo

On Wed, Oct 12, 2016 at 3:47 PM, Jeff Grafton notifications@github.com
wrote:
@ixdy commented on this pull request.

In cluster/gce/util.sh
#34577:
+function delete-firewall-rules() {
for fw in $@; do

if [[ -n $(gcloud compute firewall-rules --project "${PROJECT}" describe "${fw}" --format='value(name)' 2>/dev/null || true) ]]; then
 gcloud compute firewall-rules delete --project "${PROJECT}" --quiet "${fw}" &
fi

done

kube::util::wait-for-jobs || {

echo -e "${color_red}Failed to delete firewall rules.${color_norm}" >&2

}
+}
+function delete-network() {
if [[ -n $(gcloud compute networks --project "${PROJECT}" describe "${NETWORK}" --format='value(name)' 2>/dev/null || true) ]]; then

if ! gcloud compute networks delete --project "${PROJECT}" --quiet "${NETWORK}"; then
 echo "Failed to delete network '${NETWORK}'. Listing firewall-rules:"
 gcloud compute firewall-rules --project "${PROJECT}" list --filter="network=${NETWORK}"
the command is gcloud compute firewall-rules, so I'm not sure what your
comment means.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34577, or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVDrFMfOO4iJCWxkq2cZytEOdhJY8ks5qzWN1gaJpZM4KUOgw
.

ixdy · 2016-10-12T23:09:16Z

@k8s-bot test this please

k8s-ci-robot · 2016-10-12T23:44:32Z

Jenkins GCI GCE e2e failed for commit ee6e924. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-github-robot · 2016-10-13T17:08:19Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-10-13T17:48:34Z

Automatic merge from submit-queue

…77-origin-release-1.4 Automated cherry pick of #34577

wonderfly · 2016-10-28T18:25:30Z

@fabioy Any plan to cherrypick this to 1.3 too?

…ck-of-#34577-origin-release-1.4 Automated cherry pick of kubernetes#34577

ixdy added the area/test label Oct 11, 2016

ixdy self-assigned this Oct 11, 2016

googlebot added the cla: yes label Oct 11, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Oct 11, 2016

ixdy force-pushed the cleanup-network branch from ef309fc to 45549b2 Compare October 12, 2016 21:39

ixdy removed their assignment Oct 12, 2016

ixdy force-pushed the cleanup-network branch 2 times, most recently from a9f3225 to f1fa21c Compare October 12, 2016 21:56

k8s-github-robot assigned zmerlynn Oct 12, 2016

ixdy force-pushed the cleanup-network branch from f1fa21c to a0b1413 Compare October 12, 2016 21:57

krzyzacy reviewed Oct 12, 2016

View reviewed changes

ixdy force-pushed the cleanup-network branch from a0b1413 to 687ab51 Compare October 12, 2016 22:04

ixdy added release-note-none Denotes a PR that doesn't merit a release note. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 12, 2016

zmerlynn reviewed Oct 12, 2016

View reviewed changes

thockin reviewed Oct 12, 2016

View reviewed changes

ixdy added 2 commits October 12, 2016 15:46

Delete all firewall rules and network on GCE/GKE test cluster teardown

661d6bd

gcloud compute instances list --zone -> --zones

ee6e924

ixdy force-pushed the cleanup-network branch from 687ab51 to ee6e924 Compare October 12, 2016 22:46

ixdy changed the title ~~Delete all firewall rules and network on GCE/GKE cluster teardown~~ Delete all firewall rules (and optionally network) on GCE/GKE cluster teardown Oct 12, 2016

zmerlynn added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2016

This was referenced Oct 13, 2016

Enable etcd3 tests on every PR kubernetes/test-infra#807

Closed

Test runs leak resources in case of cluster-startup partial failure. #33867

Closed

deads2k mentioned this pull request Oct 13, 2016

split genericapiserver configuration apart so that you can run withou… #34556

Merged

ixdy added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Oct 13, 2016

k8s-github-robot merged commit 721f4be into kubernetes:master Oct 13, 2016

k8s-github-robot mentioned this pull request Oct 13, 2016

[k8s.io] EmptyDir wrapper volumes should not conflict {Kubernetes e2e suite} #32467

Closed

mml mentioned this pull request Oct 13, 2016

gke kube-down error blows up whole test run :'( #34747

Closed

This was referenced Oct 27, 2016

All test suite kubernetes-e2e-gce-*-release-1.4 failed #35654

Closed

Automated cherry pick of #34577 #35659

Merged

jessfraz added a commit that referenced this pull request Oct 27, 2016

Merge pull request #35659 from jessfraz/automated-cherry-pick-of-#345…

ee421db

…77-origin-release-1.4 Automated cherry pick of #34577

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016

Merge pull request kubernetes#35659 from jessfraz/automated-cherry-pi…

62cd12a

…ck-of-#34577-origin-release-1.4 Automated cherry pick of kubernetes#34577

ixdy mentioned this pull request Jun 16, 2017

cluster/{gce,gke}/util.sh test-setup creates network, but test-teardown doesn't delete it #21426

Closed

Delete all firewall rules (and optionally network) on GCE/GKE cluster teardown #34577

Delete all firewall rules (and optionally network) on GCE/GKE cluster teardown #34577

Conversation

ixdy commented Oct 11, 2016 • edited by k8s-oncall Loading

ixdy commented Oct 12, 2016

k8s-ci-robot commented Oct 12, 2016

ixdy commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spxtr commented Oct 12, 2016

thockin commented Oct 12, 2016

@ixdy commented on this pull request.

thockin commented Oct 12, 2016

@ixdy commented on this pull request.

ixdy commented Oct 12, 2016

k8s-ci-robot commented Oct 12, 2016

k8s-github-robot commented Oct 13, 2016

k8s-github-robot commented Oct 13, 2016

wonderfly commented Oct 28, 2016

ixdy commented Oct 11, 2016 •

edited by k8s-oncall

Loading