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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions kubetest2-gce/deployer/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,14 @@ func (d *deployer) buildEnv() []string {
// the run of kubetest2 and so should be placed in the artifacts directory
env = append(env, fmt.Sprintf("KUBECONFIG=%s", d.kubeconfigPath))

// kube-up and kube-down get this as a default ("kubernetes") but log-dump
// does not. opted to set it manually here for maximum consistency
// kube-up and kube-down get this as a default ("kubernetes" / "e2e-test-${USER}")
// but log-dump does not, set it explicitly here for maximum consistency
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 😅

env = append(env, fmt.Sprintf("KUBE_GCE_NETWORK=%s", d.network))

// Pass through number of nodes and associated IP range. In the future,
// IP range will be configurable.
env = append(env, fmt.Sprintf("NUM_NODES=%d", d.NumNodes))
Expand Down
32 changes: 27 additions & 5 deletions kubetest2-gce/deployer/deployer.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,27 @@ type deployer struct {
CreateCustomNetwork bool `desc:"Sets the environment variable CREATE_CUSTOM_NETWORK=true during deployment."`
}

// pseudoUniqueSubstring returns a substring of a UUID
// that can be reasonably used in resource names
// where length is constrained
// e.g https://cloud.google.com/compute/docs/naming-resources
// but still retain as much uniqueness as possible
// also easily lets us tie it back to a run
func pseudoUniqueSubstring(uuid string) string {
// 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

// timestamp and has the best avalanche effect (https://en.wikipedia.org/wiki/Avalanche_effect)
// as compared to the other bytes
// 13 characters is also <= the no. of character being used previously
const maxResourceNamePrefixLength = 13
if len(uuid) <= maxResourceNamePrefixLength {
return uuid
}
return uuid[:maxResourceNamePrefixLength]
}

// New implements deployer.New for gce
func New(opts types.Options) (types.Deployer, *pflag.FlagSet) {
d := &deployer{
Expand All @@ -92,11 +113,12 @@ func New(opts types.Options) (types.Deployer, *pflag.FlagSet) {
Strategy: "make",
},
},
kubeconfigPath: filepath.Join(opts.RunDir(), "kubetest2-kubeconfig"),
logsDir: filepath.Join(opts.RunDir(), "cluster-logs"),
boskosHeartbeatClose: make(chan struct{}),
instancePrefix: "kubetest2",
network: "default",
kubeconfigPath: filepath.Join(opts.RunDir(), "kubetest2-kubeconfig"),
logsDir: filepath.Join(opts.RunDir(), "cluster-logs"),
boskosHeartbeatClose: make(chan struct{}),
// names need to start with an alphabet
instancePrefix: "kt2-" + pseudoUniqueSubstring(opts.RunID()),
network: "kt2-" + pseudoUniqueSubstring(opts.RunID()),
BoskosAcquireTimeoutSeconds: 5 * 60,
BoskosLocation: "http://boskos.test-pods.svc.cluster.local.",
NumNodes: 3,
Expand Down
54 changes: 54 additions & 0 deletions kubetest2-gce/deployer/deployer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
Copyright 2020 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package deployer

import "testing"

func TestPseudoUniqueSubstring(t *testing.T) {
testCases := []struct {
name string
uuid string
expectedSubstring string
}{
{
name: "actual uuid",
uuid: "09a2565a-7ac6-11eb-a603-2218f636630c",
expectedSubstring: "09a2565a-7ac6",
},
{
name: "<= 13 length uuid",
uuid: "09a2565a-7ac6",
expectedSubstring: "09a2565a-7ac6",
},
{
name: "empty string",
uuid: "",
expectedSubstring: "",
},
}

for _, tc := range testCases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
actualSubstring := pseudoUniqueSubstring(tc.uuid)
if actualSubstring != tc.expectedSubstring {
t.Errorf("invalid substring: expected %s, but got %s", tc.expectedSubstring, actualSubstring)
}
})
}
}