Skip to content
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

[GCE] Explicitly set KUBE_GCE_NETWORK #100

Merged
merged 3 commits into from
Mar 3, 2021

Conversation

amwat
Copy link
Contributor

@amwat amwat commented Mar 2, 2021

fixes: #99

propagates to

https://github.com/kubernetes/kubernetes/blob/0ced9d2854174778163b791038c0a757103455b6/cluster/gce/config-test.sh#L134

Also use a run specific prefix instead of a static one to easily be able to tie back to a run.

xref: kubernetes/enhancements#2464

Tested locally with

kubetest2 gce --gcp-project=<my_project> --gcp-zone=<my_zone> --repo-root=<repo_root> --up --test=ginkgo -- --focus-regex='Services.should.be.able.to.create.a.functioning.NodePort.service' --use-built-binaries

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 2, 2021
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 2, 2021
@amwat
Copy link
Contributor Author

amwat commented Mar 2, 2021

/cc @BenTheElder @spiffxp

env = append(env, fmt.Sprintf("KUBE_GCE_INSTANCE_PREFIX=%s", d.instancePrefix))

// kube-up and kube-down get this as a default ("default" / "e2e-test-${USER}")
// but log-dump does not, set it explicitly here for maximum consistency
Copy link
Member

Choose a reason for hiding this comment

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

neat. did kubetest do this? are there more of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did kubetest do this ?

I don't think so it just relied on the defaults.

are there more of these?

Hopefully not 😅

// both KUBETEST2_RUN_ID and PROW_JOB_ID uuids are generated
// following RFC 4122 https://tools.ietf.org/html/rfc4122
// e.g. 09a2565a-7ac6-11eb-a603-2218f636630c
// extract the first 13 characters (09a2565a-7ac6) as they are the ones that depend on
Copy link
Member

@BenTheElder BenTheElder Mar 2, 2021

Choose a reason for hiding this comment

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

if we're constrained to 13/36 characters, perhaps we should skip the fixed -?

Copy link
Member

Choose a reason for hiding this comment

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

we also probably don't want purely timestamp bits if we can avoid it, else concurrent runs can conflict ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean currently it's a fixed name for all so it's no worse

@amwat
Copy link
Contributor Author

amwat commented Mar 2, 2021

@spiffxp how are we managing ACLs for the build cluster?

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kubetest2/100/gce-build-up-down/1366553371506380800#1:build-log.txt%3A735

Activated service account credentials for: [pr-kubekins@kubernetes-jenkins-pull.iam.gserviceaccount.com]
ERROR: (gcloud.compute.instances.add-metadata) Could not fetch resource:
 - Required 'compute.instances.get' permission for 'projects/k8s-prow-builds/zones/us-central1-b/instances/kt2-5a1d0484-7af2-master'

@amwat
Copy link
Contributor Author

amwat commented Mar 2, 2021

Anyway the actual error is

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kubetest2/100/gce-build-up-down/1366553371506380800#1:build-log.txt%3A893

ERROR: (gcloud.compute.firewall-rules.delete) Could not fetch resource:
 - The resource 'projects/k8s-jkns-e2e-gce-slow-1-2/global/firewalls/kt2-5a1d0484-7af2-minion-nodeports' was not found

because it was already deleted by kube-down (now that we plumb through the right network)
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kubetest2/100/gce-build-up-down/1366553371506380800#1:build-log.txt%3A882

Deleting firewall rules remaining in network kt2-5a1d0484-7af2: kt2-5a1d0484-7af2-minion-nodeports
Deleted [https://www.googleapis.com/compute/v1/projects/k8s-jkns-e2e-gce-slow-1-2/global/firewalls/kt2-5a1d0484-7af2-minion-nodeports].

https://github.com/kubernetes/kubernetes/blob/6d64dfea641c8cc8ade0d5402abbc5685ba6f70c/cluster/gce/util.sh#L2473

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 2, 2021
Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

/approve
/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 3, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amwat, spiffxp

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit b6e52f6 into kubernetes-sigs:master Mar 3, 2021
@amwat amwat deleted the gce_network branch March 3, 2021 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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.

[GCE] [Failing Test] conformance nodePort tests
4 participants