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.

Sign up for GitHub

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

🌱 Improve output of exec.KubectlApply #9737

Merged
merged 1 commit into from
Nov 22, 2023
Merged

🌱 Improve output of exec.KubectlApply #9737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 20 additions & 18 deletions cmd/clusterctl/cmd/topology_plan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package cmd

import (
"context"
"errors"
"fmt"
"io"
"os"
Expand All @@ -28,7 +29,7 @@ import (
"strings"

"github.com/olekukonko/tablewriter"
"github.com/pkg/errors"
pkgerrors "github.com/pkg/errors"
"github.com/spf13/cobra"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/utils/exec"
Expand Down Expand Up @@ -121,11 +122,11 @@ func runTopologyPlan() error {
for _, f := range tp.files {
raw, err := os.ReadFile(f) //nolint:gosec
if err != nil {
return errors.Wrapf(err, "failed to read input file %q", f)
return pkgerrors.Wrapf(err, "failed to read input file %q", f)
Comment on lines -124 to +125
Copy link
Member

Choose a reason for hiding this comment

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

While we're here, should we move these to be fmt.Errorf instead?

Copy link
Member Author

@sbueringer sbueringer Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if I got your question correctly.

I think as of today github.com/pkg/errors is still the standard in CAPI that we use everywhere to create and wrap errors. Are you asking to use the Go SDK funcs instead? If yes, I would defer that to a separate issue / PR / discussion around if we want to migrate away from pkg/errors.

Related: Previously closed issue around migrating away from pkg/errors: #6688

Copy link
Member Author

@sbueringer sbueringer Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Merged this PR for now as we would like to use it to debug e2e tests. Happy to continue the conversation and follow-up accordingly

Copy link
Member

Choose a reason for hiding this comment

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

The packages are compatibile with each other, especially the wrap/unwrap; we could just use those going forward

Copy link
Member Author

@sbueringer sbueringer Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't there a difference in the amount of information they store in the errors? (pkg/errors stores the entire stacktrace which then gets logged if JSON logging is used)

(we should probably move this discussion to an issue, so others see it as well)

}
objects, err := utilyaml.ToUnstructured(raw)
if err != nil {
return errors.Wrapf(err, "failed to convert file %q to list of objects", f)
return pkgerrors.Wrapf(err, "failed to convert file %q to list of objects", f)
}
objs = append(objs, objects...)
}
Expand Down Expand Up @@ -154,7 +155,7 @@ func printTopologyPlanOutput(out *cluster.TopologyPlanOutput, outdir string) err
} else {
printChangeSummary(out)
if err := writeOutputFiles(out, outdir); err != nil {
return errors.Wrap(err, "failed to write output files of target cluster changes")
return pkgerrors.Wrap(err, "failed to write output files of target cluster changes")
}
}
fmt.Printf("\n")
Expand Down Expand Up @@ -233,17 +234,17 @@ func writeOutputFiles(out *cluster.TopologyPlanOutput, outDir string) error {
// Write created files
createdDir := path.Join(outDir, "created")
if err := os.MkdirAll(createdDir, 0750); err != nil {
return errors.Wrapf(err, "failed to create %q directory", createdDir)
return pkgerrors.Wrapf(err, "failed to create %q directory", createdDir)
}
for _, c := range out.Created {
yaml, err := utilyaml.FromUnstructured([]unstructured.Unstructured{*c})
if err != nil {
return errors.Wrap(err, "failed to convert object to yaml")
return pkgerrors.Wrap(err, "failed to convert object to yaml")
}
fileName := fmt.Sprintf("%s_%s_%s.yaml", c.GetKind(), c.GetNamespace(), c.GetName())
filePath := path.Join(createdDir, fileName)
if err := os.WriteFile(filePath, yaml, 0600); err != nil {
return errors.Wrapf(err, "failed to write yaml to file %q", filePath)
return pkgerrors.Wrapf(err, "failed to write yaml to file %q", filePath)
}
}
if len(out.Created) != 0 {
Expand All @@ -253,44 +254,44 @@ func writeOutputFiles(out *cluster.TopologyPlanOutput, outDir string) error {
// Write modified files
modifiedDir := path.Join(outDir, "modified")
if err := os.MkdirAll(modifiedDir, 0750); err != nil {
return errors.Wrapf(err, "failed to create %q directory", modifiedDir)
return pkgerrors.Wrapf(err, "failed to create %q directory", modifiedDir)
}
for _, m := range out.Modified {
// Write the modified object to file.
fileNameModified := fmt.Sprintf("%s_%s_%s.modified.yaml", m.After.GetKind(), m.After.GetNamespace(), m.After.GetName())
filePathModified := path.Join(modifiedDir, fileNameModified)
if err := writeObjectToFile(filePathModified, m.After); err != nil {
return errors.Wrap(err, "failed to write modified object to file")
return pkgerrors.Wrap(err, "failed to write modified object to file")
}

// Write the original object to file.
fileNameOriginal := fmt.Sprintf("%s_%s_%s.original.yaml", m.Before.GetKind(), m.Before.GetNamespace(), m.Before.GetName())
filePathOriginal := path.Join(modifiedDir, fileNameOriginal)
if err := writeObjectToFile(filePathOriginal, m.Before); err != nil {
return errors.Wrap(err, "failed to write original object to file")
return pkgerrors.Wrap(err, "failed to write original object to file")
}

// Calculate the jsonpatch and write to a file.
patch := crclient.MergeFrom(m.Before)
jsonPatch, err := patch.Data(m.After)
if err != nil {
return errors.Wrapf(err, "failed to calculate jsonpatch of modified object %s/%s", m.After.GetNamespace(), m.After.GetName())
return pkgerrors.Wrapf(err, "failed to calculate jsonpatch of modified object %s/%s", m.After.GetNamespace(), m.After.GetName())
}
patchFileName := fmt.Sprintf("%s_%s_%s.jsonpatch", m.After.GetKind(), m.After.GetNamespace(), m.After.GetName())
patchFilePath := path.Join(modifiedDir, patchFileName)
if err := os.WriteFile(patchFilePath, jsonPatch, 0600); err != nil {
return errors.Wrapf(err, "failed to write jsonpatch to file %q", patchFilePath)
return pkgerrors.Wrapf(err, "failed to write jsonpatch to file %q", patchFilePath)
}

// Calculate the diff and write to a file.
diffFileName := fmt.Sprintf("%s_%s_%s.diff", m.After.GetKind(), m.After.GetNamespace(), m.After.GetName())
diffFilePath := path.Join(modifiedDir, diffFileName)
diffFile, err := os.OpenFile(filepath.Clean(diffFilePath), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600)
if err != nil {
return errors.Wrapf(err, "unable to open file %q", diffFilePath)
return pkgerrors.Wrapf(err, "unable to open file %q", diffFilePath)
}
if err := writeDiffToFile(filePathOriginal, filePathModified, diffFile); err != nil {
return errors.Wrapf(err, "failed to write diff to file %q", diffFilePath)
return pkgerrors.Wrapf(err, "failed to write diff to file %q", diffFilePath)
}
}
if len(out.Modified) != 0 {
Expand All @@ -303,10 +304,10 @@ func writeOutputFiles(out *cluster.TopologyPlanOutput, outDir string) error {
func writeObjectToFile(filePath string, obj *unstructured.Unstructured) error {
yaml, err := utilyaml.FromUnstructured([]unstructured.Unstructured{*obj})
if err != nil {
return errors.Wrap(err, "failed to convert object to yaml")
return pkgerrors.Wrap(err, "failed to convert object to yaml")
}
if err := os.WriteFile(filePath, yaml, 0600); err != nil {
return errors.Wrapf(err, "failed to write yaml to file %q", filePath)
return pkgerrors.Wrapf(err, "failed to write yaml to file %q", filePath)
}
return nil
}
Expand Down Expand Up @@ -348,7 +349,7 @@ func writeDiffToFile(from, to string, out io.Writer) error {
cmd.SetStdout(out)

if err := cmd.Run(); err != nil && !isDiffError(err) {
return errors.Wrapf(err, "failed to run %q", diff)
return pkgerrors.Wrapf(err, "failed to run %q", diff)
}
return nil
}
Expand Down Expand Up @@ -382,7 +383,8 @@ func getDiffCommand(args ...string) (string, exec.Cmd) {
// This makes use of the exit code of diff programs which is 0 for no diff, 1 for
// modified and 2 for other errors.
func isDiffError(err error) bool {
if err, ok := err.(exec.ExitError); ok && err.ExitStatus() <= 1 {
var exitErr exec.ExitError
if errors.As(err, &exitErr) && exitErr.ExitStatus() <= 1 {
return true
}
return false
Expand Down
14 changes: 12 additions & 2 deletions test/framework/cluster_proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@ import (
"fmt"
"net/url"
"os"
"os/exec"
"path"
goruntime "runtime"
"sync"
"time"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
pkgerrors "github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/wait"
Expand All @@ -42,7 +44,7 @@ import (

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
"sigs.k8s.io/cluster-api/test/framework/exec"
testexec "sigs.k8s.io/cluster-api/test/framework/exec"
"sigs.k8s.io/cluster-api/test/framework/internal/log"
"sigs.k8s.io/cluster-api/test/infrastructure/container"
)
Expand Down Expand Up @@ -250,7 +252,15 @@ func (p *clusterProxy) Apply(ctx context.Context, resources []byte, args ...stri
Expect(ctx).NotTo(BeNil(), "ctx is required for Apply")
Expect(resources).NotTo(BeNil(), "resources is required for Apply")

return exec.KubectlApply(ctx, p.kubeconfigPath, resources, args...)
if err := testexec.KubectlApply(ctx, p.kubeconfigPath, resources, args...); err != nil {
var exitErr *exec.ExitError
if errors.As(err, &exitErr) {
return pkgerrors.New(fmt.Sprintf("%s: stderr: %s", err.Error(), exitErr.Stderr))
}
return err
}

return nil
}

func (p *clusterProxy) GetRESTConfig() *rest.Config {
Expand Down
7 changes: 5 additions & 2 deletions test/framework/clusterctl/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package clusterctl

import (
"context"
"errors"
"fmt"
"os"
"os/exec"
Expand Down Expand Up @@ -99,7 +100,8 @@ func InitWithBinary(_ context.Context, binary string, input InitInput) {
_ = os.WriteFile(filepath.Join(input.LogFolder, "clusterctl-init.log"), out, 0644) //nolint:gosec // this is a log file to be shared via prow artifacts
var stdErr string
if err != nil {
if exitErr, ok := err.(*exec.ExitError); ok {
var exitErr *exec.ExitError
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
if errors.As(err, &exitErr) {
stdErr = string(exitErr.Stderr)
}
}
Expand Down Expand Up @@ -333,7 +335,8 @@ func ConfigClusterWithBinary(_ context.Context, clusterctlBinaryPath string, inp
_ = os.WriteFile(filepath.Join(input.LogFolder, fmt.Sprintf("%s-cluster-template.yaml", input.ClusterName)), out, 0644) //nolint:gosec // this is a log file to be shared via prow artifacts
var stdErr string
if err != nil {
if exitErr, ok := err.(*exec.ExitError); ok {
var exitErr *exec.ExitError
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
if errors.As(err, &exitErr) {
stdErr = string(exitErr.Stderr)
}
}
Expand Down
12 changes: 6 additions & 6 deletions test/framework/exec/kubectl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"context"
"fmt"
"os"
"strings"
)

// KubectlApply shells out to kubectl apply.
Expand All @@ -34,13 +35,12 @@ func KubectlApply(ctx context.Context, kubeconfigPath string, resources []byte,
WithArgs(aargs...),
WithStdin(rbytes),
)

fmt.Printf("Running kubectl %s\n", strings.Join(aargs, " "))
stdout, stderr, err := applyCmd.Run(ctx)
if err != nil {
fmt.Println(string(stderr))
return err
}
fmt.Println(string(stdout))
return nil
fmt.Printf("stderr:\n%s\n", string(stderr))
fmt.Printf("stdout:\n%s\n", string(stdout))
return err
}

// KubectlWait shells out to kubectl wait.
Expand Down
Loading