Skip to content
This repository was archived by the owner on Jun 14, 2019. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all 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
13 changes: 10 additions & 3 deletions pkg/steps/artifacts.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@ import (
"sync"
"time"

"k8s.io/apimachinery/pkg/util/sets"
"github.com/golang/glog"

coreapi "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
meta "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
kerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/client-go/kubernetes/scheme"
coreclientset "k8s.io/client-go/kubernetes/typed/core/v1"
"k8s.io/client-go/rest"
Expand Down Expand Up @@ -182,7 +183,7 @@ type PodClient interface {
}

func copyArtifacts(podClient PodClient, into, ns, name, containerName string, paths []string) error {
log.Printf("Copying artifacts from %s into %s", name, into)
glog.V(4).Infof("Copying artifacts from %s into %s", name, into)
var args []string
for _, s := range paths {
args = append(args, "-C", s, ".")
Expand Down Expand Up @@ -253,7 +254,10 @@ func copyArtifacts(podClient PodClient, into, ns, name, containerName string, pa
size += h.Size
}

if size > 0 {
// If we're updating a substantial amount of artifacts, let the user know as a way to
// indicate why the step took a long amount of time. Conversely, if we just got a small
// number of files this is just noise and can be omitted to not distract from other steps.
if size > 1*1000*1000 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? Document tpls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduce pointless output. Will comment. The only reason to print it is when we upload lots of content and the job just sits there.

log.Printf("Copied %0.2fMi of artifacts from %s to %s", float64(size)/1000000, name, into)
}

Expand Down Expand Up @@ -651,6 +655,9 @@ func gatherContainerLogsOutput(podClient PodClient, artifactDir, namespace, podN
if err != nil {
return fmt.Errorf("could not list pod: %v", err)
}
if len(list.Items) == 0 {
return nil
}
pod := &list.Items[0]

if pod.Annotations["ci-operator.openshift.io/save-container-logs"] != "true" {
Expand Down
29 changes: 25 additions & 4 deletions pkg/steps/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,15 @@ import (
const testSecretName = "test-secret"
const testSecretDefaultPath = "/usr/test-secrets"

// PodStepConfiguration allows other steps to reuse the pod launching and monitoring
// behavior without reimplementing function. It also enforces conventions like naming,
// directory structure, and input image format. More sophisticated reuse of launching
// pods should use RunPod which is more limited.
type PodStepConfiguration struct {
// SkipLogs instructs the step to omit informational logs, such as when the pod is
// part of a larger step like release creation where displaying pod specific info
// is confusing to an end user. Failure logs are still printed.
SkipLogs bool
As string
From api.ImageStreamTagReference
Commands string
Expand All @@ -48,8 +56,9 @@ func (s *podStep) Inputs(ctx context.Context, dry bool) (api.InputDefinition, er
}

func (s *podStep) Run(ctx context.Context, dry bool) error {
log.Printf("Executing %s %s", s.name, s.config.As)

if !s.config.SkipLogs {
Copy link
Contributor

Choose a reason for hiding this comment

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

This smells bad, why isn't this going to make for inscrutable logs that admins hate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are pod steps called within jobs (where you want artifacts). The log output when you call these nested logs is more confusing - the step for "build release image" doesn't need to show executing pod release-latest because it already said what it is doing. We still print pod logs when they fail which is 95% of all debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The import from initial needs to run two pods, but the pods should never fail and the output of the nested pod within the import step is very confusing when reading it. Only the top most step matters in terms of you needing to understand what is happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a long godoc on PodStepConfiguration to explain this, let me know if that helps clarify.

log.Printf("Executing %s %s", s.name, s.config.As)
}
containerResources, err := resourcesFor(s.resources.RequirementsForStep(s.config.As))
if err != nil {
return fmt.Errorf("unable to calculate %s pod resources for %s: %s", s.name, s.config.As, err)
Expand Down Expand Up @@ -107,8 +116,8 @@ func (s *podStep) Run(ctx context.Context, dry bool) error {
s.subTests = testCaseNotifier.SubTests(s.Description() + " - ")
}()

if err := waitForPodCompletion(s.podClient.Pods(s.jobSpec.Namespace), pod.Name, testCaseNotifier); err != nil {
return fmt.Errorf("test %q failed: %v", pod.Name, err)
if err := waitForPodCompletion(s.podClient.Pods(s.jobSpec.Namespace), pod.Name, testCaseNotifier, s.config.SkipLogs); err != nil {
return fmt.Errorf("%s %q failed: %v", s.name, pod.Name, err)
}
return nil
}
Expand Down Expand Up @@ -267,3 +276,15 @@ func getSecretVolumeMountFromSecret(secretMountPath string) []coreapi.VolumeMoun
},
}
}

// RunPod may be used to run a pod to completion. Provides a simpler interface than
// PodStep and is intended for other steps that may need to run transient actions.
// This pod will not be able to gather artifacts, nor will it report log messages
// unless it fails.
func RunPod(podClient PodClient, pod *coreapi.Pod) error {
pod, err := createOrRestartPod(podClient.Pods(pod.Namespace), pod)
if err != nil {
return err
}
return waitForPodCompletion(podClient.Pods(pod.Namespace), pod.Name, nil, true)
}
Loading