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

fix(sync): log a warning for empty pods #9599

Merged
merged 3 commits into from
Dec 16, 2024
Merged

fix(sync): log a warning for empty pods #9599

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
25a48dc
fix(sync): add validation for namespaces and handle empty pods
idsulik Dec 6, 2024
ff1fd91
fixes
idsulik Dec 7, 2024
c848fb1
fixes
idsulik Dec 11, 2024
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
10 changes: 10 additions & 0 deletions pkg/skaffold/sync/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,10 @@ func Perform(ctx context.Context, image string, files syncMap, cmdFn func(contex
return nil
}

if len(namespaces) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any way to add a test for this? not sure if that's possible with the current unit test setup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

were you able to reproduce the issue and confirm this fixes it? In the bug they filed, I didn't see anything in the logs to indicate what the issue was

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's just an assumption, because according to the log if something went wrong, it returns error, but it doesn't return any specific error if there is no namespaces OR nor running pod was found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these logs should be merged regardless of the issue, because in these cases(empty namespaces or no running pods) there is no way to know what went wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

So I would delete this line as it'll never be hit. Specifically here: https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/deploy/util/namespaces.go#L35C4-L35C26 if the namespace is empty, it gets the kube context's namespace. If nothing was set, the default namespace is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return fmt.Errorf("no namespaces provided for syncing")
}

errs, ctx := errgroup.WithContext(ctx)

client, err := kubernetesclient.Client(kubeContext)
Expand All @@ -340,10 +344,16 @@ func Perform(ctx context.Context, image string, files syncMap, cmdFn func(contex
pods, err := client.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{
FieldSelector: fmt.Sprintf("status.phase=%s", v1.PodRunning),
})

if err != nil {
return fmt.Errorf("getting pods for namespace %q: %w", ns, err)
}

if len(pods.Items) == 0 {
log.Entry(ctx).Warnf("no running pods found in namespace %q", ns)
continue
}

for _, p := range pods.Items {
for _, c := range p.Spec.Containers {
if c.Image != image {
Expand Down
92 changes: 49 additions & 43 deletions pkg/skaffold/sync/sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -903,7 +903,7 @@ func fakeCmd(ctx context.Context, _ v1.Pod, _ v1.Container, files syncMap) *exec
return exec.CommandContext(ctx, "copy", args...)
}

var pod = &v1.Pod{
var runningPod = &v1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "podname",
},
Expand All @@ -921,58 +921,64 @@ var pod = &v1.Pod{
}

func TestPerform(t *testing.T) {
tests := []struct {
description string
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would revert and keep description as a field instead of making this a map. It is easier to read the tests that way and is the general best practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

image string
files syncMap
pod *v1.Pod
cmdFn func(context.Context, v1.Pod, v1.Container, syncMap) *exec.Cmd
cmdErr error
clientErr error
expected []string
shouldErr bool
tests := map[string]struct {
image string
files syncMap
pod *v1.Pod
cmdFn func(context.Context, v1.Pod, v1.Container, syncMap) *exec.Cmd
cmdErr error
clientErr error
expected []string
shouldErr bool
}{
{
description: "no error",
image: "gcr.io/k8s-skaffold:123",
files: syncMap{"test.go": {"/test.go"}},
pod: pod,
cmdFn: fakeCmd,
expected: []string{"copy test.go /test.go"},
"no error": {
image: "gcr.io/k8s-skaffold:123",
files: syncMap{"test.go": {"/test.go"}},
pod: runningPod,
cmdFn: fakeCmd,
expected: []string{"copy test.go /test.go"},
},
{
description: "cmd error",
image: "gcr.io/k8s-skaffold:123",
files: syncMap{"test.go": {"/test.go"}},
pod: pod,
cmdFn: fakeCmd,
cmdErr: fmt.Errorf(""),
shouldErr: true,
"cmd error": {
image: "gcr.io/k8s-skaffold:123",
files: syncMap{"test.go": {"/test.go"}},
pod: runningPod,
cmdFn: fakeCmd,
cmdErr: fmt.Errorf(""),
shouldErr: true,
},
{
description: "client error",
image: "gcr.io/k8s-skaffold:123",
files: syncMap{"test.go": {"/test.go"}},
pod: pod,
cmdFn: fakeCmd,
clientErr: fmt.Errorf(""),
shouldErr: true,
"client error": {
image: "gcr.io/k8s-skaffold:123",
files: syncMap{"test.go": {"/test.go"}},
pod: runningPod,
cmdFn: fakeCmd,
clientErr: fmt.Errorf(""),
shouldErr: true,
},
{
description: "no copy",
image: "gcr.io/different-pod:123",
files: syncMap{"test.go": {"/test.go"}},
pod: pod,
cmdFn: fakeCmd,
shouldErr: true,
"pod not running": {
image: "gcr.io/k8s-skaffold:123",
files: syncMap{"test.go": {"/test.go"}},
pod: nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

in the code you're only logging a warning, but here the test should fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it fails because no files were synced

cmdFn: fakeCmd,
shouldErr: true,
},
"no copy": {
image: "gcr.io/different-pod:123",
files: syncMap{"test.go": {"/test.go"}},
pod: runningPod,
cmdFn: fakeCmd,
shouldErr: true,
},
}
for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
for name, test := range tests {
testutil.Run(t, name, func(t *testutil.T) {
cmdRecord := &TestCmdRecorder{err: test.cmdErr}

t.Override(&util.DefaultExecCommand, cmdRecord)
t.Override(&client.Client, func(string) (kubernetes.Interface, error) {
if test.pod == nil {
return fake.NewSimpleClientset(), nil
}

return fake.NewSimpleClientset(test.pod), test.clientErr
})

Copy link
Contributor

Choose a reason for hiding this comment

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

there are no namespaces being given below in Perform the arg is `[]string{""} - why does the first test case still work "no error"? Is there a default namespace being used?

instead of adding a new test (i.e. TestPerform_WithoutNamespaces) could you just add another test case here? First add a new value to the test struct "namespaces", add to existing tests and then pass that along when calling Perform. And in that test case you'd have an empty namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it's []string{""} slice of 1 item and the item is an empty string, this is why the len returns a non-zero value and it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about adding one more test case to the old test, but it'll complicate the test, so decided to create a separate test

Copy link
Contributor

Choose a reason for hiding this comment

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

See above comment it turns out we don't need to check for empty namespaces and can remove the separate test. Appreciate the explanation though.

Expand Down