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

Jump to bottom

x-snap-config: Do not check if node is part of the cluster #688

Merged
merged 1 commit into from
Sep 23, 2024
Merged

x-snap-config: Do not check if node is part of the cluster #688

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
6 changes: 1 addition & 5 deletions src/k8s/cmd/k8s/k8s_x_snapd_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,7 @@ func newXSnapdConfigCmd(env cmdutil.ExecutionEnvironment) *cobra.Command {
ctx, cancel := context.WithTimeout(cmd.Context(), opts.timeout)
defer cancel()
if err := control.WaitUntilReady(ctx, func() (bool, error) {
_, partOfCluster, err := client.NodeStatus(cmd.Context())
if !partOfCluster {
cmd.PrintErrf("Node is not part of a cluster: %v\n", err)
env.Exit(1)
}
_, _, err := client.NodeStatus(cmd.Context())
Copy link
Contributor

Choose a reason for hiding this comment

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

So, if NodeStatus() always returns partofCluster=false for any error why do we no longer need this exit early?

i'm not sure i have the context from #633 to properly review this. Maybe @mateoflorido

Copy link
Contributor

Choose a reason for hiding this comment

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

basically you're saying any error from NodeStatus at all exits early-- and that's wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, WaitUntilReady will retry if the bool return value is false. If something in the error return value is returned, then this exits immediately.

NodeStatus second return value indicates if the node is part of a cluster or not. However, it will also return false for this argument if an error is returned (third return value). The current implementation would always exit with Node is notpart of cluster even if NodeStatus returned and error (which means the "is part of cluster" return value is not valid).

Copy link
Contributor

Choose a reason for hiding this comment

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

AHH! i didn't look down at lines 70-71. Thank you

return err == nil, nil
}); err != nil {
cmd.PrintErrf("Error: k8sd did not come up in time: %v\n", err)
Expand Down
Loading