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.

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

Show correct message on NodeStatus failure #564

Merged
Merged
Show file tree
Hide file tree
Changes from 2 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
4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/parts/
/stage/
/prime/
.vscode/
Copy link
Contributor

Choose a reason for hiding this comment

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

don't folks normally put this in their global gitignore?

https://blog.martinhujer.cz/dont-put-idea-vscode-directories-to-projects-gitignore/

I don't really know how i land on this...

Copy link
Contributor Author

@HomayoonAlimohammadi HomayoonAlimohammadi Jul 25, 2024

Choose a reason for hiding this comment

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

TIL about global git ignore 😄

env/


**.snap
Expand All @@ -20,4 +22,4 @@ k8s_*.txt
/docs/tools/.sphinx/warnings.txt
/docs/tools/.sphinx/.wordlist.dic
/docs/tools/.sphinx/.doctrees/
/docs/tools/.sphinx/node_modules
/docs/tools/.sphinx/node_modules
6 changes: 1 addition & 5 deletions src/k8s/cmd/k8s/k8s_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,7 @@ func newKubeConfigCmd(env cmdutil.ExecutionEnvironment) *cobra.Command {
return
}

if _, err := client.NodeStatus(cmd.Context()); err != nil {
cmd.PrintErrln("Error: The node is not part of a Kubernetes cluster. You can bootstrap a new cluster with:\n\n sudo k8s bootstrap")
env.Exit(1)
return
}
_ = GetNodeStatus(client, cmd, env)
Copy link
Contributor

Choose a reason for hiding this comment

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

This implementation hides a lot of details and is not particular easy to read.
You would need to check the implementation of GetNodeStatus to understand what is going on and why we ignore everything here.

I would:
a) change the GetNodeStatus function to return the status and an error
b) create a new function e.g. isBootstrapped that wraps around the GetNodeStatus call, handles the error decoding logic and returns a bool
c) Then you can do something like this in the commands:

if isBootstrapped(...) {
   cmd.Print("Node already bootstrapped/part of a cluster etc")
   cmd.Exit(1)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the amazing comment @bschimke95 . I tried to address your concern. it didn't turn out to be exactly like you suggested but I guess it's close. returning 3 things does bother me a bit but I feel it's not really that bad. it reduces duplication, it's easily testable and more readable than the previous version. the name isBootstrapped does not seem to work here since we need to return the status and an error as well.


ctx, cancel := context.WithTimeout(cmd.Context(), opts.timeout)
cobra.OnFinalize(cancel)
Expand Down
6 changes: 1 addition & 5 deletions src/k8s/cmd/k8s/k8s_helm.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,7 @@ func newHelmCmd(env cmdutil.ExecutionEnvironment) *cobra.Command {
return
}

if status, err := client.NodeStatus(cmd.Context()); err != nil {
cmd.PrintErrln("Error: The node is not part of a Kubernetes cluster. You can bootstrap a new cluster with:\n\n sudo k8s bootstrap")
env.Exit(1)
return
} else if status.ClusterRole == apiv1.ClusterRoleWorker {
if status := GetNodeStatus(client, cmd, env); status.ClusterRole == apiv1.ClusterRoleWorker {
cmd.PrintErrln("Error: k8s helm commands are not allowed on worker nodes.")
env.Exit(1)
return
Expand Down
6 changes: 1 addition & 5 deletions src/k8s/cmd/k8s/k8s_kubectl.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,7 @@ func newKubectlCmd(env cmdutil.ExecutionEnvironment) *cobra.Command {
return
}

if status, err := client.NodeStatus(cmd.Context()); err != nil {
cmd.PrintErrln("Error: The node is not part of a Kubernetes cluster. You can bootstrap a new cluster with:\n\n sudo k8s bootstrap")
env.Exit(1)
return
} else if status.ClusterRole == apiv1.ClusterRoleWorker {
if status := GetNodeStatus(client, cmd, env); status.ClusterRole == apiv1.ClusterRoleWorker {
cmd.PrintErrln("Error: k8s kubectl commands are not allowed on worker nodes.")
env.Exit(1)
return
Expand Down
9 changes: 1 addition & 8 deletions src/k8s/cmd/k8s/k8s_local_node_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,7 @@ func newLocalNodeStatusCommand(env cmdutil.ExecutionEnvironment) *cobra.Command
return
}

status, err := client.NodeStatus(cmd.Context())
if err != nil {
cmd.PrintErrf("Error: Failed to get the status of the local node.\n\nThe error was: %v\n", err)
env.Exit(1)
return
}

outputFormatter.Print(status)
outputFormatter.Print(GetNodeStatus(client, cmd, env))
},
}
cmd.Flags().StringVar(&opts.outputFormat, "output-format", "plain", "set the output format to one of plain, json or yaml")
Expand Down
6 changes: 1 addition & 5 deletions src/k8s/cmd/k8s/k8s_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,7 @@ func newStatusCmd(env cmdutil.ExecutionEnvironment) *cobra.Command {
ctx, cancel := context.WithTimeout(cmd.Context(), opts.timeout)
cobra.OnFinalize(cancel)

if _, err := client.NodeStatus(cmd.Context()); err != nil {
cmd.PrintErrln("Error: The node is not part of a Kubernetes cluster. You can bootstrap a new cluster with:\n\n sudo k8s bootstrap")
env.Exit(1)
return
}
_ = GetNodeStatus(client, cmd, env)

status, err := client.ClusterStatus(ctx, opts.waitReady)
if err != nil {
Expand Down
40 changes: 40 additions & 0 deletions src/k8s/cmd/k8s/node_status.go
Copy link
Contributor

Choose a reason for hiding this comment

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

the files in src/k8s/cmd/k8s follow the format k8s_<command>_<subcommand>.go
That way a developer can easily see what commands the CLI supports.

Therefore, please move utils like that to src/k8s/cmd/utils.

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package k8s

import (
"errors"
"net/http"

"github.com/spf13/cobra"

apiv1 "github.com/canonical/k8s/api/v1"
cmdutil "github.com/canonical/k8s/cmd/util"
"github.com/canonical/k8s/pkg/client/k8sd"

"github.com/canonical/lxd/shared/api"
)

// GetNodeStatus retrieves the NodeStatus from k8sd client.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it retrieves it from the server - the client just does the underlying http request.

Also, let's not overdo it with the line breaks. The comments can be at least the same length as the code.

// If the daemon is not initialized, it exits with an error
// describing that the cluster should be bootstrapped.
// In case of any other errors it exits and shows the error.
func GetNodeStatus(client k8sd.Client, cmd *cobra.Command, env cmdutil.ExecutionEnvironment) apiv1.NodeStatus {
status, err := client.NodeStatus(cmd.Context())
if err == nil {
return status
}

if errors.As(err, &api.StatusError{}) {
// the returned `ok` can be ignored since we're using errors.As()
// on the same type immediately before it
statusErr, _ := err.(api.StatusError)
if statusErr.Status() == http.StatusServiceUnavailable {
cmd.PrintErrln("Error: The node is not part of a Kubernetes cluster. You can bootstrap a new cluster with:\n\n sudo k8s bootstrap")
env.Exit(1)
return status
}
}

cmd.PrintErrf("Error: Failed to retrieve the node status.\n\nThe error was: %v\n", err)
env.Exit(1)
return status
}
101 changes: 101 additions & 0 deletions src/k8s/cmd/k8s/node_status_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
package k8s_test

import (
"bytes"
"context"
"errors"
"net/http"
"testing"

apiv1 "github.com/canonical/k8s/api/v1"
"github.com/canonical/k8s/cmd/k8s"
cmdutil "github.com/canonical/k8s/cmd/util"
k8sdmock "github.com/canonical/k8s/pkg/client/k8sd/mock"
snapmock "github.com/canonical/k8s/pkg/snap/mock"
"github.com/canonical/lxd/shared/api"

. "github.com/onsi/gomega"
)

func TestGetNodeStatusUtil(t *testing.T) {
type testcase struct {
expectedCode int
name string
nodeStatusErr error
expectedInStdErr []string
nodeStatusResult apiv1.NodeStatus
}

tests := []testcase{
{
name: "NoError",
nodeStatusResult: apiv1.NodeStatus{
Name: "name", Address: "addr",
ClusterRole: apiv1.ClusterRoleControlPlane, DatastoreRole: apiv1.DatastoreRoleVoter,
},
},
{
name: "DaemonNotInitialized",
nodeStatusErr: api.StatusErrorf(http.StatusServiceUnavailable, "Daemon not yet initialized"),
expectedCode: 1,
expectedInStdErr: []string{"The node is not part of a Kubernetes cluster. You can bootstrap a new cluster"},
},
{
name: "RandomError",
nodeStatusErr: errors.New("something went bad"),
expectedCode: 1,
expectedInStdErr: []string{"something went bad", "Failed to retrieve the node status"},
},
{
name: "ContextCanceled",
nodeStatusErr: context.Canceled,
expectedCode: 1,
expectedInStdErr: []string{context.Canceled.Error(), "Failed to retrieve the node status"},
},
{
name: "ContextDeadlineExceeded",
nodeStatusErr: context.DeadlineExceeded,
expectedCode: 1,
expectedInStdErr: []string{context.DeadlineExceeded.Error(), "Failed to retrieve the node status"},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

var (
returnCode int
stdout = &bytes.Buffer{}
stderr = &bytes.Buffer{}
mockClient = &k8sdmock.Mock{
NodeStatusErr: tt.nodeStatusErr,
NodeStatusResult: tt.nodeStatusResult,
}
env = cmdutil.ExecutionEnvironment{
Stdout: stdout,
Stderr: stderr,
Getuid: func() int { return 0 },
Snap: &snapmock.Snap{
Mock: snapmock.Mock{
K8sdClient: mockClient,
},
},
Exit: func(rc int) { returnCode = rc },
}
cmd = k8s.NewRootCmd(env)
)

status := k8s.GetNodeStatus(mockClient, cmd, env)

g.Expect(returnCode).To(Equal(tt.expectedCode))
for _, exp := range tt.expectedInStdErr {
g.Expect(stderr.String()).To(ContainSubstring(exp))
}

if tt.expectedCode == 0 {
g.Expect(status).To(Equal(tt.nodeStatusResult))
}
})
}
}
2 changes: 1 addition & 1 deletion src/k8s/pkg/k8sd/api/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ func (e *Endpoints) getNodeStatus(s *state.State, r *http.Request) response.Resp

status, err := impl.GetLocalNodeStatus(r.Context(), s, snap)
if err != nil {
response.InternalError(err)
return response.InternalError(err)
bschimke95 marked this conversation as resolved.
Show resolved Hide resolved
}

result := apiv1.GetNodeStatusResponse{
Expand Down
Loading