Switch away from gcloud deprecated flags in compute resource listings #50398

pci · 2017-08-09T21:33:05Z

What is fixed

Remove deprecated gcloud compute flags, see linked issue.

Which issue this PR fixes:

Special notes for your reviewer:

The change in gcloudComputeResourceList in test/e2e/framework/ingress_utils.go isn't strictly needed as currently no affected resources are called on within that file, however the function has the potential to access affected resources so I covered it as well. Happy to change if deemed unnecessary.

Release note:

NONE

k8s-ci-robot · 2017-08-09T21:33:12Z

Hi @pci. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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. I understand the commands that are listed here.

liggitt · 2017-08-10T02:16:36Z

/unassign
/assign @cjcullen

ncdc · 2017-08-10T13:27:29Z

/unassign

dims · 2017-08-17T11:05:11Z

/ok-to-test

cblecker · 2017-08-17T17:27:53Z

/cc @shyamjvs

cblecker · 2017-08-17T17:28:12Z

/assign @ixdy

shyamjvs · 2017-08-17T17:37:58Z

cluster/gce/list-resources.sh

@@ -75,10 +75,10 @@ echo "Provider: ${KUBERNETES_PROVIDER:-}"
 # provided.
 gcloud-compute-list instance-templates --regexp="${INSTANCE_PREFIX}.*"
 gcloud-compute-list instance-groups ${ZONE:+"--zones=${ZONE}"} --regexp="${INSTANCE_PREFIX}.*"
-gcloud-compute-list instances ${ZONE:+"--zones=${ZONE}"} --regexp="${INSTANCE_PREFIX}.*"
+gcloud-compute-list instances --filter="${ZONE:+"zone=(${ZONE}) AND"} name ~ '${INSTANCE_PREFIX}.*'"


Are you sure this works with the first double apostrophe closed by the immediate next one?

Here are the tests I ran:

$ test="${ZONE:+"zone=(${ZONE}) AND"} name ~ '${INSTANCE_PREFIX}.*'"; echo $test name ~ '.*' $ ZONE=foo; test="${ZONE:+"zone=(${ZONE}) AND"} name ~ '${INSTANCE_PREFIX}.*'"; echo $test zone=(foo) AND name ~ '.*' $ INSTANCE_PREFIX=bar; test="${ZONE:+"zone=(${ZONE}) AND"} name ~ '${INSTANCE_PREFIX}.*'"; echo $test name ~ 'bar.*' $ ZONE=foo;INSTANCE_PREFIX=bar; test="${ZONE:+"zone=(${ZONE}) AND"} name ~ '${INSTANCE_PREFIX}.*'"; echo $test zone=(foo) AND name ~ 'bar.*'

run on my local machine:

GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin16) Copyright (C) 2007 Free Software Foundation, Inc.

shyamjvs · 2017-08-17T17:39:43Z

test/e2e/framework/ingress_utils.go

@@ -761,9 +761,20 @@ func (cont *GCEIngressController) deleteStaticIPs() error {
 func gcloudComputeResourceList(resource, regex, project string, out interface{}) {
 	// gcloud prints a message to stderr if it has an available update
 	// so we only look at stdout.
+	var regexFlag string


nit: s/regexFlag/regexpFlag/

Absolutely, sorry for the typo.

Also as this is my first kubernetes' PR, do reviewers prefer changes as a second commit, or squashed into the first and rebased on an up-to-date master ?

Usually it's better to add a new commit on each review round, so previous comments are not lost and also it's easier to verify if comments have been addressed.
However, it's ok to squash for simple PRs.

If an additional commit is the standard way, I'll go with that.

Update pushed, I realised filterFlag is a better name for the flag given it's the flag applying the filter and not always a regex.

shyamjvs · 2017-08-17T17:39:53Z

test/e2e/framework/ingress_utils.go

@@ -761,9 +761,20 @@ func (cont *GCEIngressController) deleteStaticIPs() error {
 func gcloudComputeResourceList(resource, regex, project string, out interface{}) {
 	// gcloud prints a message to stderr if it has an available update
 	// so we only look at stdout.
+	var regexFlag string
+
+	// --regex flag is deprecated for these resources:


👍 Same as above

shyamjvs · 2017-08-17T18:42:02Z

Also there are usages of the regexp flag in cluster/gce/util.sh. Can you fix them also?

pci · 2017-08-17T19:06:14Z

@shyamjvs How did I miss those?! My ag skills let me down. Thanks for the spot, update pushed.

Will the automated tests cover these lines or is there some manual testing I could do?

shyamjvs · 2017-08-17T19:15:47Z

The automated tests should likely cover it (though I'm not sure if they'd pass as the gcloud version currently being used for those tests has been pinned at 163.0.0).
Anyway, testing it locally shouldn't be hard - just try starting a small cluster (using cluster/kube-up.sh) and then stopping it (using cluster/kube-down.sh).

pci · 2017-08-29T21:59:20Z

@shyamjvs Ah, thanks. The logs looked like a timeout.

shyamjvs · 2017-08-29T22:37:30Z

/lgtm
@ixdy Wanna take a final look?

ixdy · 2017-08-29T22:43:01Z

/retest

pci · 2017-08-30T05:19:45Z

/test pull-kubernetes-e2e-gce-etcd3

(failed due to "Backend Error" in gcloud.compute.instance-groups.managed.create call)

ixdy

thanks for taking this on!

ixdy · 2017-08-30T05:23:50Z

cluster/gce/util.sh

-    --zones "${ZONE}" --project "${PROJECT}" \
-    --regexp "${NODE_INSTANCE_PREFIX}-.+" \
+    --project "${PROJECT}" \
+    --filter "name ~ '${NODE_INSTANCE_PREFIX}-.+' AND zone:(${ZONE})" \


why is it sometimes zone:(${ZONE}) and sometimes zone=(${ZONE})?

ah, I guess :( matches patterns, whereas =( matches exactly, per https://cloud.google.com/sdk/gcloud/reference/topic/filters.

this is probably fine, though I'm guessing =( is maybe more correct?

@ixdy the reasoning in full is here but : seems to be the way gcloud pushes as = flat out doesn't work for zones

@ixdy Very sorry - I missed one zone here I'll fix that

ok, there now shouldn't be any occurrences of zone= inside a filter

ixdy · 2017-08-30T05:29:52Z

/lgtm

ixdy · 2017-08-30T05:30:14Z

/test pull-kubernetes-e2e-gce-bazel

roberthbailey · 2017-08-30T05:32:51Z

/approve

pci · 2017-08-30T05:34:57Z

@ixdy No problem, thank you (and shyamjvs & roberthbailey) for your efforts in reviewing and moving this forward.

pci · 2017-08-30T05:45:53Z

@ixdy @shyamjvs @roberthbailey Once again my apologies - There was still a zone= filter here which would have returned zero rows for EXPECTED_NUM_NODES which isn't correct, I've updated it to use zone: as per this comment.

Famous last words but I hope that's the last one.

ixdy · 2017-08-30T05:53:34Z

ok, so zone: is correct, but so is region=?
I love gcloud so much. :)

pci · 2017-08-30T05:58:56Z

@ixdy that definitely seems to be the case, although the deprecation warning is the only documentation I've found on it! Even if they fix the behaviour of zone= in the future, zone: should continue to work so we're covered from that side.

ixdy · 2017-08-30T06:10:09Z

/lgtm

k8s-github-robot · 2017-08-30T06:10:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ixdy, pci, roberthbailey, shyamjvs

Associated issue: 49673

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cluster/OWNERS~~ [roberthbailey]
~~hack/OWNERS~~ [ixdy]
~~test/OWNERS~~ [ixdy]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

pci · 2017-08-30T07:19:03Z

/test pull-kubernetes-e2e-gce-bazel

Failed with "no such file"

ixdy · 2017-08-30T07:28:02Z

yeah. :\

the good news is that gce-bazel isn't blocking at the moment.

k8s-github-robot · 2017-08-30T08:51:29Z

Automatic merge from submit-queue (batch tested with PRs 47054, 50398, 51541, 51535, 51545)

pci · 2017-08-30T10:21:26Z

Woo! Thanks all.

I'll look to open the cherry-pick PRs to 1.7, 1.6 and 1.5 tonight (UK time), let me know if that isn't the right next step.

…stream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #50398 Cherry pick of #50398 on release-1.7. #50398: Switch away from gcloud deprecated flags in compute resource

…stream-release-1.6 Automatic merge from submit-queue Automated cherry pick of #50398 Cherry pick of #50398 on release-1.6. #50398: Switch away from gcloud deprecated flags in compute resource

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 9, 2017

k8s-github-robot assigned liggitt Aug 9, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 9, 2017

k8s-github-robot assigned ncdc Aug 9, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 9, 2017

pci force-pushed the gcloud-compute-list branch 2 times, most recently from 4fa18d5 to 60825f9 Compare August 9, 2017 21:37

liggitt added the area/platform/gce label Aug 10, 2017

k8s-ci-robot assigned cjcullen and unassigned liggitt Aug 10, 2017

k8s-ci-robot unassigned ncdc Aug 10, 2017

pci mentioned this pull request Aug 16, 2017

All release-1.5 jobs failing due to leaked resources (mainly master VM and network) #50624

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 17, 2017

k8s-ci-robot requested a review from shyamjvs August 17, 2017 17:27

k8s-ci-robot assigned ixdy Aug 17, 2017

shyamjvs reviewed Aug 17, 2017

View reviewed changes

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 17, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2017

ixdy approved these changes Aug 30, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2017

Switch away from gcloud deprecated flags in compute resource listings

697f92a

pci force-pushed the gcloud-compute-list branch from d1a0820 to 697f92a Compare August 30, 2017 05:41

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

k8s-github-robot merged commit 04bc4ec into kubernetes:master Aug 30, 2017

This was referenced Aug 30, 2017

Automated cherry pick of #50398 #51651

Merged

Automated cherry pick of #50398 #51652

Merged

Automated cherry pick of #50398 #51654

Closed

Switch away from gcloud deprecated flags in compute resource listings #50398

Switch away from gcloud deprecated flags in compute resource listings #50398

Conversation

pci commented Aug 9, 2017

k8s-ci-robot commented Aug 9, 2017

liggitt commented Aug 10, 2017

ncdc commented Aug 10, 2017

dims commented Aug 17, 2017

cblecker commented Aug 17, 2017

cblecker commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pci Aug 17, 2017 • edited

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

shyamjvs commented Aug 17, 2017

pci commented Aug 17, 2017 • edited

shyamjvs commented Aug 17, 2017

pci commented Aug 29, 2017

shyamjvs commented Aug 29, 2017

ixdy commented Aug 29, 2017

pci commented Aug 30, 2017

ixdy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pci Aug 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixdy commented Aug 30, 2017

ixdy commented Aug 30, 2017

roberthbailey commented Aug 30, 2017

pci commented Aug 30, 2017

pci commented Aug 30, 2017

ixdy commented Aug 30, 2017

pci commented Aug 30, 2017 • edited

ixdy commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

pci commented Aug 30, 2017

ixdy commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

pci commented Aug 30, 2017

pci Aug 17, 2017 •

edited

pci commented Aug 17, 2017 •

edited

pci Aug 30, 2017 •

edited

pci commented Aug 30, 2017 •

edited