Unify common build functionality between gce and gke #80

amwat · 2021-01-13T09:34:40Z

Also extend make build support for both and start staging with the new krel API.

MushuEE · 2021-01-13T22:02:17Z

kubetest2-gce/deployer/options/build.go

@@ -0,0 +1,40 @@
+/*
+Copyright 2020 The Kubernetes Authors.


Suggested change

Copyright 2020 The Kubernetes Authors.

Copyright 2021 The Kubernetes Authors.

MushuEE · 2021-01-13T22:27:54Z

pkg/build/make.go

 	if err != nil {
 		return "", fmt.Errorf("failed to get version: %v", err)
 	}
-	cmd := exec.Command("make", "-C", src, target)
+	cmd := exec.Command("make", "-C", m.RepoRoot, target)


Just checking, the Make file exists at Root?

yup, https://github.com/kubernetes/kubernetes/blob/master/Makefile

it's actually a symlink, but yes 🙃
the real file is in ./build/root/Makefile

MushuEE · 2021-01-13T22:30:10Z

pkg/build/release_push_build.go

 	if mat == nil || len(mat) < 4 {
-		return fmt.Errorf("invalid stage location: %v. Use gs://bucket/ci/optional-suffix", rpb.Location)
+		return fmt.Errorf("invalid stage location: %v. Use gs://bucket/ci/optional-suffix", rpb.StageLocation)


Suggested change

return fmt.Errorf("invalid stage location: %v. Use gs://bucket/ci/optional-suffix", rpb.StageLocation)

return fmt.Errorf("invalid stage location: %v. Use form gs://<bucket>/<ci>/<optional-suffix>", rpb.StageLocation)

MushuEE · 2021-01-13T22:31:13Z

pkg/build/release_push_build.go

+		return fmt.Errorf("invalid stage location: %v. Use gs://bucket/ci/optional-suffix", rpb.StageLocation)
+	}
+
+	// force indicate staging of extra gce configuration files


Suggested change

// force indicate staging of extra gce configuration files

// Force indicate staging of extra gce configuration files

MushuEE · 2021-01-13T22:33:44Z

kubetest2-gke/deployer/options/build.go

 }

+var _ build.Builder = &BuildOptions{}


Curious what the propose of this pattern is? Just type enforcement?

yes, this essentially is a compile time type assertion

adding: this is a common enough pattern in go, I'm not sure if it's strictly compile time or not though.

technically I bet at minimum it also ensures that the package defining the interface is valid to import even if it otherwise wouldn't be, and importing a package can have side effects (namely packages have init() / globals in a binary IFF they are imported somewhere).

spiffxp

tiny suggestions otherwise LGTM

spiffxp · 2021-01-15T21:08:49Z

kubetest2-gce/deployer/build.go

-		exec.InheritOutput(cmd)
-		cmd.SetDir(d.RepoRoot)
-		err := cmd.Run()
+		klog.V(2).Info("starting the legacy k/k build")


Suggested change

klog.V(2).Info("starting the legacy k/k build")

klog.V(2).Info("starting the legacy kubernetes/kubernetes build")

spiffxp · 2021-01-15T21:13:20Z

kubetest2-gce/deployer/build.go

+		version += "+" + d.commonOptions.RunID()
+		if d.BuildOptions.CommonBuildOptions.StageLocation != "" {


nit

Suggested change

version += "+" + d.commonOptions.RunID()

if d.BuildOptions.CommonBuildOptions.StageLocation != "" {

version += "+" + d.commonOptions.RunID()

// stage build if requested

if d.BuildOptions.CommonBuildOptions.StageLocation != "" {

I'm a little curious why stage needs to be done here, seems like a side effect unrelated to building, but won't block

yeah stage is optional here for gce, it's not needed if running build and up together. but needed if we want to build and then up a cluster later.

spiffxp · 2021-01-15T21:21:15Z

kubetest2-gke/deployer/build.go

@@ -55,7 +67,7 @@ func (d *deployer) Build() error {
 	}
 	// append the kubetest2 run id
 	version += "+" + d.commonOptions.RunID()
-	if d.BuildOptions.StageLocation != "" {
+	if d.BuildOptions.CommonBuildOptions.StageLocation != "" {


Suggested change

if d.BuildOptions.CommonBuildOptions.StageLocation != "" {

// stage build if requested

if d.BuildOptions.CommonBuildOptions.StageLocation != "" {

done, I think?

spiffxp · 2021-01-15T21:23:23Z

pkg/build/options.go

+)
+
+type Options struct {
+	Strategy           string `flag:"~strategy" desc:"Determines the build strategy to use either make or bazel."`


nit: seems like this could be its own type, but won't block

They are separate types:

kubetest2/pkg/build/make.go

Lines 25 to 27 in 03dbeda

type MakeBuilder struct {

RepoRoot string

}

kubetest2/pkg/build/bazel.go

Lines 27 to 31 in 03dbeda

type Bazel struct {

RepoRoot string

StageLocation string

ImageLocation string

}

the string here is to expose the available strategies as a flag.

most flag libraries allow you to use the real type instead of a string /shrug

the upside to doing so is that you can then pass around this struct as the real parsed values

we are using the real type which is BuildOptions and has the actual options like RepoRoot , but at some point you need to specify whether it's bazel or make :)

exposing the individual implementation directly would mean weirdly exposing individual flags like --make-repo-root and --bazel-repo-root which looks worse /shrug

I guess I could have been clearer and suggested a string-as-enum approach (type BuildStrategy string etc.) but as-is is fine, like I said, won't block.

BenTheElder · 2021-01-20T07:05:23Z

kubetest2-gke/deployer/build.go

@@ -55,7 +67,7 @@ func (d *deployer) Build() error {
 	}
 	// append the kubetest2 run id
 	version += "+" + d.commonOptions.RunID()
-	if d.BuildOptions.StageLocation != "" {
+	if d.BuildOptions.CommonBuildOptions.StageLocation != "" {


done, I think?

BenTheElder · 2021-01-20T07:07:49Z

kubetest2-gke/deployer/options/build.go

 }

+var _ build.Builder = &BuildOptions{}


adding: this is a common enough pattern in go, I'm not sure if it's strictly compile time or not though.

technically I bet at minimum it also ensures that the package defining the interface is valid to import even if it otherwise wouldn't be, and importing a package can have side effects (namely packages have init() / globals in a binary IFF they are imported somewhere).

BenTheElder · 2021-01-20T07:09:09Z

kubetest2-gke/deployer/options/build.go

 func (bo *BuildOptions) Validate() error {
-	return bo.implementationFromStrategy()
+	// force extra GCP files to be staged


nit: if we're always going to force this, IMHO this should be in Build() or really anywhere other than here, Validate should normally be only reading data, not writing; this fails the principle of least surprise.

Done. also added a check to ensure stage is used with build and up since that's required for gke.

BenTheElder · 2021-01-20T07:10:26Z

pkg/build/options.go

+)
+
+type Options struct {
+	Strategy           string `flag:"~strategy" desc:"Determines the build strategy to use either make or bazel."`


most flag libraries allow you to use the real type instead of a string /shrug

BenTheElder · 2021-01-20T07:10:52Z

pkg/build/options.go

+)
+
+type Options struct {
+	Strategy           string `flag:"~strategy" desc:"Determines the build strategy to use either make or bazel."`


the upside to doing so is that you can then pass around this struct as the real parsed values

amwat · 2021-01-28T22:48:50Z

ping

spiffxp

/approve
/lgtm

spiffxp · 2021-01-29T01:44:30Z

kubetest2-gke/deployer/build.go

@@ -39,9 +40,20 @@ var (
 	// $1 == prefix
 	// $2 == suffix
 	buildPrefix = regexp.MustCompile(`^(\d\.\d+\.\d+)([+-].*)?$`)
+
+	defaultImageTag = "gke.gcr.io"


nit (can be done as followup), seems like defaultImageRegistry would be a better name

spiffxp · 2021-01-29T01:45:27Z

kubetest2-gke/deployer/build.go

+	}
+
+	klog.V(2).Infof("setting KUBE_DOCKER_REGISTRY to %s for tagging images", imageTag)
+	if err := os.Setenv("KUBE_DOCKER_REGISTRY", imageTag); err != nil {


ditto for imageRegistry instead

spiffxp · 2021-01-29T01:49:04Z

pkg/build/options.go

+)
+
+type Options struct {
+	Strategy           string `flag:"~strategy" desc:"Determines the build strategy to use either make or bazel."`


I guess I could have been clearer and suggested a string-as-enum approach (type BuildStrategy string etc.) but as-is is fine, like I said, won't block.

k8s-ci-robot · 2021-01-29T01:49:54Z

[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:

~~OWNERS~~ [amwat,spiffxp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spiffxp · 2021-01-29T02:03:46Z

/test gce-build-up-down

 - The zone 'projects/k8s-e2e-gce-alpha1-5/zones/us-central1-b' does not have enough resources available to fulfill the request.  Try a different zone, or try again later.

Neat.

spiffxp · 2021-01-29T14:42:38Z

/retest

liggitt · 2021-01-29T20:37:09Z

kubetest2-gce/deployer/build.go

+			return err
+		}
+		// append the kubetest2 run id
+		version += "+" + d.commonOptions.RunID()


if version already contains "+", this should append "+" + d.commonOptions.RunID(), otherwise we end up with invalid docker tags

this is the GCE deployer not GKE, we might want similar changes here too though. yet to test this out for upstream jobs.

the double-plus version results in an invalid docker tag regardless of deployer

k8s-ci-robot requested review from BenTheElder, MushuEE and spiffxp January 13, 2021 09:34

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 13, 2021

amwat mentioned this pull request Jan 13, 2021

Remove unused code paths kubernetes/release#1841

Closed

MushuEE reviewed Jan 13, 2021

View reviewed changes

amwat force-pushed the gce_build branch 2 times, most recently from 184de2a to 9f67607 Compare January 14, 2021 03:27

spiffxp suggested changes Jan 15, 2021

View reviewed changes

amwat force-pushed the gce_build branch from 9f67607 to 03dbeda Compare January 20, 2021 06:27

BenTheElder reviewed Jan 20, 2021

View reviewed changes

amwat added 8 commits January 26, 2021 11:19

Move out common build functionality.

67cdc40

Use common build functionality.

5e3ec61

Simplify make build

9a57e04

fix make build working directory.

2a55503

move out image tagging specifically into the gke deployer.

e5bb6e1

force staging of extra gce configuration files.

cb637a5

pickup new changes for staging extra gcp files.

343f65c

ensure stage is specified with build and up

30cf15f

amwat force-pushed the gce_build branch from ff7c7e1 to 30cf15f Compare January 26, 2021 19:23

spiffxp approved these changes Jan 29, 2021

View reviewed changes

k8s-ci-robot assigned spiffxp Jan 29, 2021

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

k8s-ci-robot merged commit da7b071 into kubernetes-sigs:master Jan 29, 2021

amwat deleted the gce_build branch January 29, 2021 19:12

liggitt reviewed Jan 29, 2021

View reviewed changes

amwat mentioned this pull request Feb 2, 2021

Setup working directory correctly for bazel staging #89

Merged

	Copyright 2020 The Kubernetes Authors.
	Copyright 2021 The Kubernetes Authors.

	return fmt.Errorf("invalid stage location: %v. Use gs://bucket/ci/optional-suffix", rpb.StageLocation)
	return fmt.Errorf("invalid stage location: %v. Use form gs://<bucket>/<ci>/<optional-suffix>", rpb.StageLocation)

	// force indicate staging of extra gce configuration files
	// Force indicate staging of extra gce configuration files

	klog.V(2).Info("starting the legacy k/k build")
	klog.V(2).Info("starting the legacy kubernetes/kubernetes build")

		version += "+" + d.commonOptions.RunID()
		if d.BuildOptions.CommonBuildOptions.StageLocation != "" {

	if d.BuildOptions.CommonBuildOptions.StageLocation != "" {
	// stage build if requested
	if d.BuildOptions.CommonBuildOptions.StageLocation != "" {

	type Bazel struct {
	RepoRoot string
	StageLocation string
	ImageLocation string
	}

Unify common build functionality between gce and gke #80

Unify common build functionality between gce and gke #80

Conversation

amwat commented Jan 13, 2021

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

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

spiffxp left a 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

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

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

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

amwat commented Jan 28, 2021

spiffxp left a 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

k8s-ci-robot commented Jan 29, 2021

spiffxp commented Jan 29, 2021

spiffxp commented Jan 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment