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.
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
Swich to new gcloud API for GCE MIGs #11692
Swich to new gcloud API for GCE MIGs #11692
Conversation
GCE e2e build/test failed for commit 632f1d74443d1948d5750e9a5329db622245053e. |
# command returns, but currently it returns before the instances come up due | ||
# to gcloud's deficiency. | ||
wait-for-minions-to-run | ||
gcloud beta compute instance-groups managed wait-until-stable "${NODE_INSTANCE_PREFIX}-group" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need || true
at the end in case of errors? or some other error checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@wojtek-t why did you mark this as do not merge? |
@roberthbailey - the new API is not yet released (should be today according to MIG team from Warsaw) - this is why Jenkins tests failed. [I've tested by updating my gcloud to (still) experimental version] |
[by new API I mean support for it in gcloud] |
632f1d7
to
bc4c526
Compare
Ah. I know that the new raw API is out already; I didn't realize gcloud support hadn't rolled out yet. |
GCE e2e build/test failed for commit bc4c526a4400035d8af8812f62ba6ab524e54e27. |
bc4c526
to
247cf8f
Compare
GCE e2e build/test failed for commit 247cf8faa56debaef38bf467780cedcbc0e0e151. |
Jenkins is now failing with the following error:
How can we workaround it (and do we want?)? |
/cc @jlowdermilk |
It looks like the beta component actually is installed for shared gcloud. Maybe someone recently bumped it? @k8s-bot retest this please |
Please reassign when this is ready to merge. |
247cf8f
to
5996237
Compare
GCE e2e build/test failed for commit 59962376f6e7c77a6d0a4d11d0be0d90fb391d04. |
5996237
to
92623bc
Compare
GCE e2e build/test failed for commit 92623bcdcf4ff930315e4e97110be371c48986d6. |
92623bc
to
ef9f6c7
Compare
GCE e2e build/test failed for commit ef9f6c72de45151142061fe1f99eee61a3e840ef. |
@jlowdermilk (sorry for late response). It seems that beta is not installed - i.e. after rebasing today it's still not there. Is there any way to work around it? Is it possible to proceed with that change? |
@k8s-bot test this please |
GCE e2e build/test failed for commit ef9f6c72de45151142061fe1f99eee61a3e840ef. |
ef9f6c7
to
ff92925
Compare
GCE e2e build/test failed for commit ff9292509de6c3d03395ff909264373f24c0ea0c. |
ff92925
to
ef189bb
Compare
GCE e2e build/test failed for commit ef189bbb1cde4b961fee5fe1649c4051eee17a21. |
ef189bb
to
74a7351
Compare
GCE e2e build/test passed for commit 74a73511767e7f2e336442459436c50433ce4e58. |
This is now ready for review. |
@roberthbailey - can you please take a look (or delegate)? |
@@ -695,16 +677,16 @@ function kube-up { | |||
write-node-env | |||
create-node-instance-template | |||
|
|||
gcloud preview managed-instance-groups --zone "${ZONE}" \ | |||
gcloud beta compute instance-groups managed \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tested a similar command yesterday this was working for me without the 'beta' cmd group, so I think we can remove that from the script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - it seems to work now.
Done.
LGTM, but I think we should use |
# command returns, but currently it returns before the instances come up due | ||
# to gcloud's deficiency. | ||
wait-for-minions-to-run | ||
gcloud beta compute instance-groups managed wait-until-stable \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably need --project
here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not needed, but I've added it for consistency.
74a7351
to
2d95cd4
Compare
GCE e2e build/test passed for commit 2d95cd4. |
@roberthbailey - PTAL |
LGTM. |
@wojtek-t Please cherry pick this into the 1.0 release branch (after it's been merged) so that our release branch validation tests are using the current API for MIGs as well (I doubt the preview commands will work for much longer). |
@k8s-bot test this [testing build queue, sorry for the noise] |
GCE e2e build/test passed for commit 2d95cd4. |
Automatic merge from SubmitQueue |
Auto commit by PR queue bot
…92-upstream-release-1.0 Auto commit by PR queue bot
Fixes #11686
cc @davidopp @mbforbes @roberthbailey