Skip to content
This repository has been archived by the owner on Sep 5, 2019. It is now read-only.

Trim StepStates for implicit steps #340

Merged
merged 4 commits into from
Sep 14, 2018
Merged

Trim StepStates for implicit steps #340

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48ff6b4
checkpoint
imjasonh Sep 7, 2018
334ff58
Fixed failures, works now
imjasonh Sep 7, 2018
7974af5
Add test, move diff to buildtest
imjasonh Sep 7, 2018
85ea911
Update diff.go
imjasonh Sep 14, 2018
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
16 changes: 14 additions & 2 deletions pkg/builder/cluster/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func (op *operation) Name() string {
return op.name
}

func (op *operation) Checkpoint(status *v1alpha1.BuildStatus) error {
func (op *operation) Checkpoint(build *v1alpha1.Build, status *v1alpha1.BuildStatus) error {
status.Builder = v1alpha1.ClusterBuildProvider
if status.Cluster == nil {
status.Cluster = &v1alpha1.ClusterSpec{}
Expand All @@ -57,7 +57,19 @@ func (op *operation) Checkpoint(status *v1alpha1.BuildStatus) error {
status.StartTime = op.builder.podCreationTime
status.StepStates = nil
status.StepsCompleted = nil
for _, s := range op.statuses {

// Always ignore the first pod status, which is creds-init.
skip := 1
if build.Spec.Source != nil {
// If the build specifies source, skip another container status, which
// is the source-fetching container.
skip++
}
if skip > len(op.statuses) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there is no test coverage for this condition skip> len(op.statuses).

skip = 0
}

for _, s := range op.statuses[skip:] {
if s.State.Terminated != nil {
status.StepsCompleted = append(status.StepsCompleted, s.Name)
}
Expand Down
194 changes: 139 additions & 55 deletions pkg/builder/cluster/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,23 +60,25 @@ func TestBasicFlow(t *testing.T) {
t.Fatalf("Unexpected error executing builder.Build: %v", err)
}

var bs v1alpha1.BuildStatus
if err := op.Checkpoint(&bs); err != nil {
build := &v1alpha1.Build{
Status: v1alpha1.BuildStatus{},
}
if err := op.Checkpoint(build, &build.Status); err != nil {
t.Fatalf("Unexpected error executing op.Checkpoint: %v", err)
}
if buildercommon.IsDone(&bs) {
t.Errorf("IsDone(%v); wanted not done, got done.", bs)
if buildercommon.IsDone(&build.Status) {
t.Errorf("IsDone(%v); wanted not done, got done.", build.Status)
}
if bs.CreationTime.IsZero() {
t.Errorf("bs.CreationTime; want zero, got %v", bs.CreationTime)
if build.Status.CreationTime.IsZero() {
t.Errorf("build.Status.CreationTime; want zero, got %v", build.Status.CreationTime)
}
if !bs.CompletionTime.IsZero() {
t.Errorf("bs.CompletionTime; want zero, got %v", bs.CompletionTime)
if !build.Status.CompletionTime.IsZero() {
t.Errorf("build.Status.CompletionTime; want zero, got %v", build.Status.CompletionTime)
}
if !bs.StartTime.IsZero() {
t.Errorf("bs.StartTime; want non-zero, got %v", bs.StartTime)
if !build.Status.StartTime.IsZero() {
t.Errorf("build.Status.StartTime; want non-zero, got %v", build.Status.StartTime)
}
op, err = builder.OperationFromStatus(&bs)
op, err = builder.OperationFromStatus(&build.Status)
if err != nil {
t.Fatalf("Unexpected error executing OperationFromStatus: %v", err)
}
Expand Down Expand Up @@ -163,23 +165,25 @@ func TestNonFinalUpdateFlow(t *testing.T) {
t.Fatalf("Unexpected error executing builder.Build: %v", err)
}

var bs v1alpha1.BuildStatus
if err := op.Checkpoint(&bs); err != nil {
build := &v1alpha1.Build{
Status: v1alpha1.BuildStatus{},
}
if err := op.Checkpoint(build, &build.Status); err != nil {
t.Fatalf("Unexpected error executing op.Checkpoint: %v", err)
}
if buildercommon.IsDone(&bs) {
t.Errorf("IsDone(%v); wanted not done, got done.", bs)
if buildercommon.IsDone(&build.Status) {
t.Errorf("IsDone(%v); wanted not done, got done.", build.Status)
}
if bs.CreationTime.IsZero() {
t.Errorf("bs.CreationTime; want zero, got %v", bs.CreationTime)
if build.Status.CreationTime.IsZero() {
t.Errorf("build.Status.CreationTime; want zero, got %v", build.Status.CreationTime)
}
if !bs.CompletionTime.IsZero() {
t.Errorf("bs.CompletionTime; want zero, got %v", bs.CompletionTime)
if !build.Status.CompletionTime.IsZero() {
t.Errorf("build.Status.CompletionTime; want zero, got %v", build.Status.CompletionTime)
}
if !bs.StartTime.IsZero() {
t.Errorf("bs.StartTime; want non-zero, got %v", bs.StartTime)
if !build.Status.StartTime.IsZero() {
t.Errorf("build.Status.StartTime; want non-zero, got %v", build.Status.StartTime)
}
op, err = builder.OperationFromStatus(&bs)
op, err = builder.OperationFromStatus(&build.Status)
if err != nil {
t.Fatalf("Unexpected error executing OperationFromStatus: %v", err)
}
Expand Down Expand Up @@ -260,23 +264,25 @@ func TestFailureFlow(t *testing.T) {
t.Fatalf("Unexpected error executing builder.Build: %v", err)
}

var bs v1alpha1.BuildStatus
if err := op.Checkpoint(&bs); err != nil {
build := &v1alpha1.Build{
Status: v1alpha1.BuildStatus{},
}
if err := op.Checkpoint(build, &build.Status); err != nil {
t.Fatalf("Unexpected error executing op.Checkpoint: %v", err)
}
if buildercommon.IsDone(&bs) {
t.Errorf("IsDone(%v); wanted not done, got done.", bs)
if buildercommon.IsDone(&build.Status) {
t.Errorf("IsDone(%v); wanted not done, got done.", build.Status)
}
if bs.CreationTime.IsZero() {
t.Errorf("bs.CreationTime; want zero, got %v", bs.CreationTime)
if build.Status.CreationTime.IsZero() {
t.Errorf("build.Status.CreationTime; want zero, got %v", build.Status.CreationTime)
}
if !bs.CompletionTime.IsZero() {
t.Errorf("bs.CompletionTime; want zero, got %v", bs.CompletionTime)
if !build.Status.CompletionTime.IsZero() {
t.Errorf("build.Status.CompletionTime; want zero, got %v", build.Status.CompletionTime)
}
if !bs.StartTime.IsZero() {
t.Errorf("bs.StartTime; want non-zero, got %v", bs.StartTime)
if !build.Status.StartTime.IsZero() {
t.Errorf("build.Status.StartTime; want non-zero, got %v", build.Status.StartTime)
}
op, err = builder.OperationFromStatus(&bs)
op, err = builder.OperationFromStatus(&build.Status)
if err != nil {
t.Fatalf("Unexpected error executing OperationFromStatus: %v", err)
}
Expand Down Expand Up @@ -365,23 +371,25 @@ func TestPodPendingFlow(t *testing.T) {
t.Fatalf("Unexpected error executing builder.Build: %v", err)
}

var bs v1alpha1.BuildStatus
if err := op.Checkpoint(&bs); err != nil {
build := &v1alpha1.Build{
Status: v1alpha1.BuildStatus{},
}
if err := op.Checkpoint(build, &build.Status); err != nil {
t.Fatalf("Unexpected error executing op.Checkpoint: %v", err)
}
if buildercommon.IsDone(&bs) {
t.Errorf("IsDone(%v); wanted not done, got done.", bs)
if buildercommon.IsDone(&build.Status) {
t.Errorf("IsDone(%v); wanted not done, got done.", build.Status)
}
if bs.CreationTime.IsZero() {
t.Errorf("bs.CreationTime; want zero, got %v", bs.CreationTime)
if build.Status.CreationTime.IsZero() {
t.Errorf("build.Status.CreationTime; want zero, got %v", build.Status.CreationTime)
}
if !bs.CompletionTime.IsZero() {
t.Errorf("bs.CompletionTime; want zero, got %v", bs.CompletionTime)
if !build.Status.CompletionTime.IsZero() {
t.Errorf("build.Status.CompletionTime; want zero, got %v", build.Status.CompletionTime)
}
if !bs.StartTime.IsZero() {
t.Errorf("bs.StartTime; want non-zero, got %v", bs.StartTime)
if !build.Status.StartTime.IsZero() {
t.Errorf("build.Status.StartTime; want non-zero, got %v", build.Status.StartTime)
}
op, err = builder.OperationFromStatus(&bs)
op, err = builder.OperationFromStatus(&build.Status)
if err != nil {
t.Fatalf("Unexpected error executing OperationFromStatus: %v", err)
}
Expand Down Expand Up @@ -479,23 +487,25 @@ func TestStepFailureFlow(t *testing.T) {
t.Fatalf("Unexpected error executing builder.Build: %v", err)
}

var bs v1alpha1.BuildStatus
if err := op.Checkpoint(&bs); err != nil {
build := &v1alpha1.Build{
Status: v1alpha1.BuildStatus{},
}
if err := op.Checkpoint(build, &build.Status); err != nil {
t.Fatalf("Unexpected error executing op.Checkpoint: %v", err)
}
if buildercommon.IsDone(&bs) {
t.Errorf("IsDone(%v); wanted not done, got done.", bs)
if buildercommon.IsDone(&build.Status) {
t.Errorf("IsDone(%v); wanted not done, got done.", build.Status)
}
if bs.CreationTime.IsZero() {
t.Errorf("bs.CreationTime; want zero, got %v", bs.CreationTime)
if build.Status.CreationTime.IsZero() {
t.Errorf("build.Status.CreationTime; want zero, got %v", build.Status.CreationTime)
}
if !bs.StartTime.IsZero() {
t.Errorf("bs.StartTime; want non-zero, got %v", bs.StartTime)
if !build.Status.StartTime.IsZero() {
t.Errorf("build.Status.StartTime; want non-zero, got %v", build.Status.StartTime)
}
if !bs.CompletionTime.IsZero() {
t.Errorf("bs.CompletionTime; want zero, got %v", bs.CompletionTime)
if !build.Status.CompletionTime.IsZero() {
t.Errorf("build.Status.CompletionTime; want zero, got %v", build.Status.CompletionTime)
}
op, err = builder.OperationFromStatus(&bs)
op, err = builder.OperationFromStatus(&build.Status)
if err != nil {
t.Fatalf("Unexpected error executing OperationFromStatus: %v", err)
}
Expand Down Expand Up @@ -642,3 +652,77 @@ func statusMessage(status *v1alpha1.BuildStatus) string {
}
return ""
}

func TestStripStepStates(t *testing.T) {
for _, c := range []struct {
desc string
statuses []corev1.ContainerStatus
build *v1alpha1.Build
}{{
desc: "only creds-init",
statuses: []corev1.ContainerStatus{{
State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{Reason: "creds-init: should be stripped"}},
}, {
State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{Reason: "real step: should be retained"}},
}},
build: &v1alpha1.Build{
// No source.
Status: v1alpha1.BuildStatus{},
},
}, {
desc: "has source",
statuses: []corev1.ContainerStatus{{
Name: "creds-init",
State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{Reason: "creds-init: should be stripped"}},
}, {
Name: "git-init",
State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{Reason: "git-init: should be stripped"}},
}, {
State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{Reason: "real step: should be retained"}},
}},
build: &v1alpha1.Build{
// No source.
Spec: v1alpha1.BuildSpec{
Source: &v1alpha1.SourceSpec{
Git: &v1alpha1.GitSourceSpec{},
},
},
Status: v1alpha1.BuildStatus{},
},
}} {
t.Run(c.desc, func(t *testing.T) {
status := &v1alpha1.BuildStatus{}
op := &operation{
statuses: c.statuses,
builder: &builder{},
namespace: "namespace",
name: "podname",
}
if err := op.Checkpoint(c.build, status); err != nil {
t.Fatalf("Checkpoint: %v", err)
}

// In both cases, we want the same status, stripped of implicit
// steps' states.
wantStatus := &v1alpha1.BuildStatus{
Builder: v1alpha1.ClusterBuildProvider,
Cluster: &v1alpha1.ClusterSpec{
Namespace: "namespace",
PodName: "podname",
},
StepStates: []corev1.ContainerState{{
Terminated: &corev1.ContainerStateTerminated{Reason: "real step: should be retained"},
}},
StepsCompleted: []string{""},
Conditions: []v1alpha1.BuildCondition{{
Type: v1alpha1.BuildSucceeded,
Status: corev1.ConditionUnknown,
Reason: "Building",
}},
}
if d := buildtest.JSONDiff(wantStatus, status); d != "" {
t.Errorf("Diff:\n%s", d)
}
})
}
}
26 changes: 2 additions & 24 deletions pkg/builder/cluster/convert/convert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,8 @@ limitations under the License.
package convert

import (
"encoding/json"
"testing"

"github.com/sergi/go-diff/diffmatchpatch"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
fakek8s "k8s.io/client-go/kubernetes/fake"
Expand Down Expand Up @@ -123,7 +121,7 @@ func TestRoundtrip(t *testing.T) {
t.Fatalf("Unable to convert %q to CRD: %v", in, err)
}

if d := diff(og, b); d != "" {
if d := buildtest.JSONDiff(og, b); d != "" {
t.Errorf("Diff:\n%s", d)
}
})
Expand Down Expand Up @@ -348,29 +346,9 @@ func TestFromCRD(t *testing.T) {
t.Fatalf("FromCRD: %v", err)
}

if d := diff(got.Spec, c.want); d != "" {
if d := buildtest.JSONDiff(got.Spec, c.want); d != "" {
t.Errorf("Diff:\n%s", d)
}
})
}
}

func diff(l, r interface{}) string {
lb, err := json.MarshalIndent(l, "", " ")
if err != nil {
panic(err.Error())
}
rb, err := json.MarshalIndent(r, "", " ")
if err != nil {
panic(err.Error())
}

dmp := diffmatchpatch.New()
diffs := dmp.DiffMain(string(lb), string(rb), true)
for _, d := range diffs {
if d.Type != diffmatchpatch.DiffEqual {
return dmp.DiffPrettyText(diffs)
}
}
return ""
}
8 changes: 5 additions & 3 deletions pkg/builder/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,11 @@ type Operation interface {
// Name provides the unique name for this operation, see OperationFromStatus.
Name() string

// Checkpoint augments the provided BuildStatus with sufficient state to be restored
// by OperationFromStatus on an appropriate BuildProvider.
Checkpoint(*v1alpha1.BuildStatus) error
// Checkpoint augments the provided BuildStatus with sufficient state to be
// restored by OperationFromStatus on an appropriate BuildProvider.
//
// This takes into account necessary information about the provided Build.
Checkpoint(*v1alpha1.Build, *v1alpha1.BuildStatus) error

// Wait blocks until the Operation completes, returning either a status for the build or an error.
// TODO(mattmoor): This probably shouldn't be BuildStatus, but some sort of smaller-scope thing.
Expand Down
2 changes: 1 addition & 1 deletion pkg/builder/nop/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ type operation struct {

func (nb *operation) Name() string { return operationName }

func (nb *operation) Checkpoint(status *v1alpha1.BuildStatus) error {
func (nb *operation) Checkpoint(_ *v1alpha1.Build, status *v1alpha1.BuildStatus) error {
// Masquerade as the Google builder.
status.Builder = v1alpha1.GoogleBuildProvider
if status.Google == nil {
Expand Down
Loading