kubernetes-anywhere: make sure GCP project is allocated #9897
Conversation
k8s-ci-robot
added
area/kubetest
kind/bug
|
/priority critical-urgent |
k8s-ci-robot
added
the
priority/critical-urgent
|
/assign @krzyzacy |
kubetest/main.go
Outdated
| if err := prepareGcp(o); err != nil { | ||
| return err | ||
| } | ||
| case "aws": | ||
| if err := prepareAws(o); err != nil { | ||
| return err | ||
| } | ||
| // kubernetes-anywhere jobs are treated as --provider=local | ||
| case "local": |
There was a problem hiding this comment.
this feels a bit odd but I'm not sure if we have a better option currently
There was a problem hiding this comment.
i agree. ideas are welcome, of course.
There was a problem hiding this comment.
I feel this is odd that provider local will need to set up gcp :-/ why provider is not kubernetes-anywhere here?
There was a problem hiding this comment.
it only does if the deployer is set though.
It needs to because the tests do not support arbitrary providers 😬
There was a problem hiding this comment.
alternatively the test code should select a no-op provider when it does not know the provider flag value? this would be closer to previous behavior..
There was a problem hiding this comment.
The problem with that approach is that genuine typos (like --provider=gec instead of --provider=gce) then do not get detected.
before they created a warning in the logs.
Can't you use skeleton?
Well yes, the problem is other things are aware of which provider you're using... eg the log dump script in k/k's cluster/ dir. Here it's using """local""" but needs to also act like GCP because it's doing kubernetes-anywhere on GCE but not necessarily using the GCE provider.
There was a problem hiding this comment.
^ @krzyzacy what do you think about calling prepareGcp() in getDeployer() for the k-a case?
There was a problem hiding this comment.
let me just update this PR.
(might hide the above conversation).
There was a problem hiding this comment.
if all k-a jobs are going to use local provider... why not just add something outside of the provider switch?
if o.deployment == "kubernetes-anywhere" {
if err := prepareGcp(o); err != nil {
return err
}
}
There was a problem hiding this comment.
ok, makes sense. will update in a minute.
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
With moving k-a to "--provider=local", the kubetest function prepareGcp() is never called. Fix that by calling it in the case of a k-a deployer.
neolit123
force-pushed
the
kubeadm-e2e
branch
from
c0bfd9b
to
05c16f6
Compare
|
@krzyzacy @BenTheElder |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 6c6514745a2721ebf1911ad66cbc09e250786429
|
|
/this-is-fine |
|
In response to this:
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. |
k8s-ci-robot
added
the
approved
|
/hold |
|
/approve @BenTheElder - what's the hold for? |
|
|
Yep, hold is for that. If we unhold this we also need to bump kubetest |
|
things are P0 at this point. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, krzyzacy, neolit123, timothysc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
[discussed at kubeadm office hours, going forward with this] |
|
@BenTheElder @krzyzacy thanks for the help! |
|
... actually getting to that bump now, apologies. |
With moving k-a to "--provider=local", the kubetest
function prepareGcp() is never called. Fix that by
handling "local" in prepare().
previous PR:
ref #9884
failing test:
ref kubernetes/kubernetes#70058
/area kubetest
/kind bug
/assign @BenTheElder @timothysc
cc @jberkus