-
Notifications
You must be signed in to change notification settings - Fork 32
refactor: enhance restart command to utilize NodeProvider #516
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
refactor: enhance restart command to utilize NodeProvider #516
Conversation
cmd/beekeeper/cmd/restart.go
Outdated
|
|
||
| return nil | ||
| } | ||
| cluster, err := c.setupCluster(ctx, clusterName, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.setupCluster(ctx, clusterName, false) is already called in c.createNodeClient(ctx, true), no need to call it twice.
…d namespace checks
…tring and number values (temp)
* feat: fize-sizes param for smoke * ci: file-sizes in configs * ci: file-sizes in staging * refactor: comments * refactor: backward compatibility for ContentSize * fix: comments * fix(load): use content-size for load check (#522) --------- Co-authored-by: Ljubiša Gačević <35105035+gacevicljubisa@users.noreply.github.com>
#487) * chore: add new config file and split nodes setup for create cluster * chore: update config to use latest docker image
|
If image is not specified, pod should only be restarted. If specified, then update the Image of the statefull set and then pod should be deleted if strategy is |
cmd/beekeeper/cmd/restart.go
Outdated
| ); err != nil { | ||
| return fmt.Errorf("restarting cluster %s: %w", clusterName, err) | ||
| // TODO: Add cluster restart (should be handled by the node client) | ||
| if err := restartClient.RestartPods(ctx); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this method is enough to restart all bee nodes, we should rename it to Restart(), and remove RestartCluster()
pkg/node/client.go
Outdated
| // filterClientsByNodeGroups filters bee clients by node groups | ||
| // Node names in beekeeper deployments follow the pattern: {nodeGroup}-{index} | ||
| // This function extracts the node group from the node name and filters accordingly | ||
| func (sc *Client) filterClientsByNodeGroups(clients map[string]*bee.Client) map[string]*bee.Client { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do not need this logic. Much simplier and more precise solution would be to add NodeGroupName in bee.ClientOptions, and to expose new method NodeGroup() in bee package. Then you can compare on name. This should be done only in case of beekeeper deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is psudo code how this filtering could work:
filteredClients := make(map[string]*bee.Client)
if len(sc.nodeGroups) > 0 && sc.deploymentType == DeploymentTypeBeekeeper {
for _, ng := range sc.nodeGroups {
for name, client := range sc.beeClients {
if client.NodeGroup() == ng {
filteredClients[name] = client
}
}
}
} else {
filteredClients = sc.beeClients
}…e filtering logic for beekeeper deployments
|
#516 (comment) , Its already implemented here: beekeeper/pkg/restart/restart.go Line 69 in ae823b3
|
e6244a5
into
refactor/node-provider-interface
#512) * refactor: introduce NodeProvider interface for unified node management * fix: do not check for deployment type in node-funder and improve logs * refactor: enhance restart command to utilize NodeProvider (#516) * refactor: enhance restart command to utilize NodeProvider * refactor: add deployment-type flag to restart command * docs: update restart command docs in README * refactor: simplify restart command by removing unused cluster name and namespace checks * fix: adjust node name handling for Ingress and IngressRoute nodes * fix: change Service Port type to interface{} for better handling of string and number values (temp) * lint: remove unnecessary blank line * fix: update node discovery * feat: smoke file sizes (#505) * feat: fize-sizes param for smoke * ci: file-sizes in configs * ci: file-sizes in staging * refactor: comments * refactor: backward compatibility for ContentSize * fix: comments * fix(load): use content-size for load check (#522) --------- Co-authored-by: Ljubiša Gačević <35105035+gacevicljubisa@users.noreply.github.com> * refactor: add new config file and split nodes setup for create cluster (#487) * chore: add new config file and split nodes setup for create cluster * chore: update config to use latest docker image * fix: add image flag to restart cmd * refactor: add node groups support for filtering nodes * refactor: enhance Client structure to include NodeGroupName and update filtering logic for beekeeper deployments * refactor(node): improve filterClientsByNodeGroups method --------- Co-authored-by: Ljubisa Gacevic <ljubisa.rs@gmail.com> Co-authored-by: nugaon <toth.viktor.levente@gmail.com> Co-authored-by: Ljubiša Gačević <35105035+gacevicljubisa@users.noreply.github.com> * fix: minor changes * fix: use node-grups for filtering --------- Co-authored-by: Akrem Chabchoub <121046693+akrem-chabchoub@users.noreply.github.com> Co-authored-by: nugaon <toth.viktor.levente@gmail.com>
Refactor: Use NodeProvider interface for restart command
Problem
The restart command was tightly coupled to the orchestration package, making it difficult to reuse and maintain. It had separate methods for cluster and namespace-based restarts with duplicated logic.
Solution
Restart()methodChanges
--deployment-typeflag for beekeeper vs helm deploymentsUsage Examples
Closes #526