Add labels to all k8s objects deployed by skaffold #644

nkubala · 2018-06-06T21:46:42Z

This adds labels to all kubernetes objects deployed by Skaffold. By default, the label deployed_with:skaffold is added to all objects, as well as a skaffold_deploy_client:<client> with the deployer used in skaffold. We can add more labels or change these as we see fit.
Each Builder, Deployer, and Tagger implementation now exposes its own set of labels to the Runner, which will aggregate all labels and send those to the deployed objects on each run.

We should consider adding labels for individual profiles as well, possibly in a follow-up PR.

Also, as a small non-functional change, I changed build.Build to build.BuildArtifact to match deploy.DeployArtifact.

Fixes #98

r2d4 · 2018-06-06T21:52:40Z

pkg/skaffold/deploy/helm.go

 	}
+	labelDeployResults(deployResults)


To avoid having to run this in each deployer, I would suggest that we change the deploy interface back to

Deploy(ctx context.Context, out io.Writer, builds []build.Build) (DeployResult, error)

That maybe looks like

type DeployResult struct { deployedArtifacts []DeployedArtifact } type DeployedArtifact struct { GVK string obj *runtime.Obj }

The high level flow would be

func run() { buildResults := builder.build() deployResults := deployer.deploy(buildResults) label(cfg, deployResults) }

I had played around with having this live in the runner, but decided to go with putting it in each deployer for two reasons:

the runner will need to know less about what happened in its individually managed steps, and

labeling should be coupled to deploying, since we can't label unless we've successfully deployed.

That said, I can see arguments for moving it to the runner as well. If we add more deployers that's more boilerplate we have to remember to add, and the runner has a lot more information about the overall skaffold run which could be useful labeling information. Since the runner has all the skaffold config info, I think that's a strong enough argument for moving it back, so I'll go ahead with it and see how it looks.

r2d4 · 2018-06-06T21:52:58Z

pkg/skaffold/deploy/deploy.go

+// DeployResult is a holder struct encapsulating the deployed k8s objects
+type DeployResult struct {
+	obj       *runtime.Object
+	namespace string


The namespace is part of of the runtime.Object I believe

unfortunately it isn't always, helm can't be trusted to put it where it should be, among many other things that it should do but doesn't

r2d4 · 2018-06-06T21:53:41Z

pkg/skaffold/deploy/deploy.go

+type DeployResult struct {
+	obj       *runtime.Object
+	namespace string
+	deployer  string


We can infer this from the config. I'm OK with adding a Name() string requirement to the deploy interface

Agreed as per my comment above

r2d4 · 2018-06-08T16:50:59Z

pkg/skaffold/deploy/label.go

+	return "default"
+}
+
+func updateRuntimeObject(client clientgo.Interface, labels map[string]string, res Artifact) error {


I believe the dynamic client should make this easy.

https://github.com/kubernetes/client-go/blob/master/dynamic/interface.go

Looks like client-go itself is discouraging use of the dynamic client: kubernetes/client-go#394. Even though it looks a little messy I think sticking to the typed APIs is safer and probably a better idea

I think we should use it anyways. Lets keep it out of scope for this PR though.

r2d4 · 2018-06-11T21:02:01Z

pkg/skaffold/deploy/label.go

+const tries int = 3
+const sleeptime time.Duration = 300 * time.Millisecond
+
+func LabelDeployResults(labels map[string]string, results []Artifact) {


Since labels come from the runner, this should be in the runner package

moved to runner/labels

r2d4 · 2018-06-11T21:02:32Z

pkg/skaffold/deploy/label.go

+	return "default"
+}
+
+func updateRuntimeObject(client clientgo.Interface, labels map[string]string, res Artifact) error {


Add a TODO here to get rid of this in favor of the dynamic client or something else

r2d4 · 2018-06-11T22:19:55Z

pkg/skaffold/build/tag/git_commit.go

@@ -32,6 +34,12 @@ import (
 type GitCommit struct {
 }

+func (c *GitCommit) Labels() map[string]string {
+	return map[string]string{
+		constants.Labels.TagPolicy: "git_commit",


nit: consistency gitCommit

r2d4 · 2018-06-11T22:23:50Z

pkg/skaffold/config/options.go

+func (opts *SkaffoldOptions) Labels() map[string]string {
+	labels := map[string]string{}
+	if opts.CustomTag != "" {
+		labels["custom_tag"] = "true"


I'd remove this

Add

namespace

profiles

cleanup

profiles is a little special because this will just contain the list of all profiles specified on the CLI, but not which one was actually used to create each k8s API object. was planning on opening an issue to add support for profile labeling outside this PR

r2d4 · 2018-06-11T22:34:58Z

pkg/skaffold/runner/timings.go

-	if err != nil {
-		return err
+	dRes, err := w.Deployer.Deploy(ctx, out, builds)
+	if err == nil {


this error goes unchecked

dRes, err := w.Deployer.Deploy if err != nil { return nil, err } fmt.Fprintln return dRes, nil

good catch, got the logic slightly wrong here. that's equivalent to

dRes, err := w.Deployer.Deploy if err == nil { fmt.Fprintln } return dRes, err

which is what I was trying to write.

r2d4 · 2018-06-11T22:37:23Z

pkg/skaffold/deploy/helm.go

+// Retrieve info about all releases using helm get
+// Skaffold labels will be applied to each deployed k8s object
+// Since helm isn't always consistent with retrieving results, don't return errors here
+func (h *HelmDeployer) getDeployResults(namespace string, release string) []Artifact {


can you add a test for this?

r2d4 · 2018-06-11T22:38:36Z

pkg/skaffold/deploy/deploy.go

+// Artifact contains all information about a completed deployment
+type Artifact struct {
+	obj       *runtime.Object
+	namespace string


why we need namespace here?

the namespace used to do a deployment doesn't always match the namespace in the deployer, and we need access to it when updating the API objects with the labels

r2d4 · 2018-06-11T22:39:45Z

pkg/skaffold/constants/constants.go

+	DefaultLabels map[string]string
+}{
+	DefaultLabels: map[string]string{
+		"deployed_with": "skaffold",


nit: hyphen instead of underscore

https://github.com/kubernetes/website/pull/8703/files?short_path=7b9d071#diff-7b9d071558ae546db530549c8c8542a1

r2d4 · 2018-06-11T22:54:11Z

pkg/skaffold/build/local.go

@@ -64,6 +64,12 @@ func NewLocalBuilder(cfg *v1alpha2.BuildConfig, kubeContext string) (*LocalBuild
 	return l, nil
 }

+func (l *LocalBuilder) Labels() map[string]string {
+	return map[string]string{
+		constants.Labels.Builder: "local",


nit:
add some context from the docker api

"docker-api-version": api.ServerVersion(ctx)

r2d4 · 2018-06-11T22:55:35Z

pkg/skaffold/build/container_builder.go

-func (cb *GoogleCloudBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) ([]Build, error) {
+func (cb *GoogleCloudBuilder) Labels() map[string]string {
+	return map[string]string{
+		constants.Labels.Builder: "GoogleCloudBuilder",


google-cloud-builder

nkubala · 2018-06-12T22:58:41Z

dlorenc · 2018-06-13T16:05:41Z

pkg/skaffold/deploy/helm.go

+
+func (h *HelmDeployer) getReleaseInfo(release string) (*bufio.Reader, error) {
+	var releaseInfo bytes.Buffer
+	out := bufio.NewWriter(&releaseInfo)


You shouldn't need this bufio.NewWriter, you can use a var releaseInfo bytes.Buffer directly as a reader/writer:
https://golang.org/pkg/bytes/#Buffer.Write

yep, thanks

dlorenc · 2018-06-13T16:06:23Z

pkg/skaffold/deploy/helm.go

+			break
+		}
+		if err != nil {
+			logrus.Infof("error parsing helm deployment: %s", err.Error())


should we have a continue here to avoid trying to parse an invalid or nil doc?

good call, added

dlorenc · 2018-06-13T16:06:45Z

pkg/skaffold/deploy/kubectl.go

+func parseManifestsForDeploys(manifests manifestList) ([]Artifact, error) {
+	results := []Artifact{}
+	for _, manifest := range manifests {
+		b := bufio.NewReader(bytes.NewReader(manifest))


Why do you wrap this in bufio?

the YAMLReader from apimachinery requires a bufio.Reader instead of an io.Reader: https://github.com/kubernetes/apimachinery/blob/master/pkg/util/yaml/decoder.go#L262

balopat

I see I was too slow. Added couple of nits maybe for next time.

balopat · 2018-06-13T16:04:53Z

cmd/skaffold/app/cmd/deploy.go

@@ -50,7 +51,7 @@ func NewCmdDeploy(out io.Writer) *cobra.Command {
 func runDeploy(out io.Writer, filename string) error {
 	ctx := context.Background()

-	runner, _, err := newRunner(filename)


nit: I'd think runner is short enough.

balopat · 2018-06-13T18:56:22Z

pkg/skaffold/build/tag/date_time.go

@@ -40,6 +42,12 @@ func NewDateTimeTagger(format, timezone string) Tagger {
 	}
 }

+func (tagger *dateTimeTagger) Labels() map[string]string {
+	return map[string]string{
+		constants.Labels.TagPolicy: "dateTimeTagger",


nit: this could be just dateTime to be consistent

balopat · 2018-06-13T18:56:35Z

pkg/skaffold/build/tag/env_template.go

@@ -40,8 +41,14 @@ func NewEnvTemplateTagger(t string) (Tagger, error) {
 	}, nil
 }

+func (t *envTemplateTagger) Labels() map[string]string {
+	return map[string]string{
+		constants.Labels.TagPolicy: "envTemplateTagger",


nit: this could be just envTemplate to be consistent

balopat · 2018-06-13T20:04:22Z

pkg/skaffold/runner/label/label.go

+const sleeptime time.Duration = 300 * time.Millisecond
+
+//nolint
+func LabelDeployResults(labels map[string]string, results []deploy.Artifact) {


nit: how about ApplyLabels?

nkubala requested review from dgageot, dlorenc, r2d4 and viglesiasce as code owners June 6, 2018 21:46

r2d4 reviewed Jun 6, 2018

View reviewed changes

r2d4 self-assigned this Jun 7, 2018

nkubala force-pushed the labels branch from 9ae8f81 to 869a807 Compare June 7, 2018 21:43

r2d4 reviewed Jun 8, 2018

View reviewed changes

r2d4 mentioned this pull request Jun 8, 2018

Label schema #656

Closed

r2d4 reviewed Jun 11, 2018

View reviewed changes

nkubala force-pushed the labels branch 3 times, most recently from 93106ed to a52436d Compare June 12, 2018 22:56

dlorenc reviewed Jun 13, 2018

View reviewed changes

dlorenc approved these changes Jun 13, 2018

View reviewed changes

add labels to all k8s objects deployed by skaffold

f63fcdb

nkubala force-pushed the labels branch from 0606d04 to f63fcdb Compare June 13, 2018 18:52

nkubala merged commit dade08e into GoogleContainerTools:master Jun 13, 2018

balopat reviewed Jun 13, 2018

View reviewed changes

nkubala deleted the labels branch September 24, 2018 20:19

skame mentioned this pull request Aug 12, 2019

using with 'skaffold deploy' made duplicated deployment vmware-tanzu/velero#1746

Closed

Add labels to all k8s objects deployed by skaffold #644

Add labels to all k8s objects deployed by skaffold #644

Conversation

nkubala commented Jun 6, 2018 • edited Loading

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

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

nkubala commented Jun 12, 2018

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

balopat 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

nkubala commented Jun 6, 2018 •

edited

Loading