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

Switch the keadm ctl client to client go and add the restful interface for restart pod. #5572

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Switch the keadm ctl client to client go and add the restful interface for restart pod. #5572

wants to merge 1 commit into from

Conversation

luomengY
Copy link
Member

@luomengY luomengY commented Apr 28, 2024 •
edited

…e for restart pod.

What type of PR is this?
/kind feature

What this PR does / why we need it:
1.switch the keadm ctl client to client go.
2.add the restful interface for restart pod.
POST:/api/v1/namespaces/{namespace}/pods/restart
body: ["pod-1","pod-2","pod-3"]
response: {"errMessages":[],"containerIds":[]}
image

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

@kubeedge-bot kubeedge-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 28, 2024
@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign skdwriting after the PR has been reviewed.
You can assign the PR to them by writing /assign @skdwriting in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubeedge-bot kubeedge-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 28, 2024
@luomengY luomengY force-pushed the kead_ctl_client-go branch 4 times, most recently from 0d18440 to c9c9019 Compare April 28, 2024 05:35
WillardHu
WillardHu reviewed Apr 29, 2024
View reviewed changes
keadm/cmd/keadm/app/cmd/ctl/restart/pod.go Outdated
Comment on lines 81 to 88
for _, errMsg := range restartResponse.ErrMessages {
fmt.Println(errMsg)
}

for _, containerID := range restartResponse.ContainerIds {
fmt.Println(containerID)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's not a friendly way to output messages, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not a friendly way to output messages, right?

What is the appropriate way to print the stopped container ID and the error message returned by the request here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Follow your logic, friendly information sample:

case 1. The pod 'aaa' restart failure, reason: <error-message>.
case 2. There are some containers of Pod 'bbb' that failed to restart, reason:  <error-message>,  containers container-1, container-2 have been restarted.
case 3. The pod 'ccc' restart successful.

You can handle these response error messages in package storage.

if err != nil {
    message := fmt.Sprintf(xxxxxxxxx)
    klog.Warning("[metaserver/reststorage] ", message)
    restartResponse.ErrMessages = append(restartResponse.ErrMessages, message)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Follow your logic, friendly information sample:

case 1. The pod 'aaa' restart failure, reason: <error-message>.
case 2. There are some containers of Pod 'bbb' that failed to restart, reason:  <error-message>,  containers container-1, container-2 have been restarted.
case 3. The pod 'ccc' restart successful.

You can handle these response error messages in package storage.

if err != nil {
    message := fmt.Sprintf(xxxxxxxxx)
    klog.Warning("[metaserver/reststorage] ", message)
    restartResponse.ErrMessages = append(restartResponse.ErrMessages, message)
}

Done

keadm/cmd/keadm/app/cmd/ctl/restart/pod.go Outdated
if err != nil {
return nil, err
}
ctx := context.TODO()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it better if the variable context is inputed by the caller?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

keadm/cmd/keadm/app/cmd/ctl/client/pod.go Outdated
Method: "GET",
Path: "/" + CoreAPIPrefix + "/" + CoreAPIGroupVersion.Version +
"/namespaces/" + podRequest.Namespace + "/pods",
podList, err := kubeClient.CoreV1().Pods(metaV1.NamespaceAll).List(context.TODO(), metaV1.ListOptions{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it better if the variable context is inputed by the caller?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@luomengY luomengY force-pushed the kead_ctl_client-go branch 4 times, most recently from 47dc48f to e096c5c Compare April 30, 2024 09:37
WillardHu
WillardHu reviewed Apr 30, 2024
View reviewed changes
keadm/cmd/keadm/app/cmd/ctl/client/pod.go Outdated
}

func (podRequest *PodRequest) GetPod(ctx context.Context) (*corev1.Pod, error) {
select {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use select here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why use select here?

Modified

keadm/cmd/keadm/app/cmd/ctl/client/pod.go Outdated
}

func (podRequest *PodRequest) GetPods(ctx context.Context) (*corev1.PodList, error) {
select {
Copy link
Collaborator

Choose a reason for hiding this comment

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

select?

Copy link
Member Author

Choose a reason for hiding this comment

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

select?

Modified

@luomengY luomengY force-pushed the kead_ctl_client-go branch 4 times, most recently from 7bf1a80 to 040f800 Compare May 1, 2024 05:50
WillardHu
WillardHu reviewed May 29, 2024
View reviewed changes
edge/pkg/metamanager/metaserver/handlerfactory/handler.go Outdated
@@ -200,6 +201,46 @@ func (f *Factory) Patch(reqInfo *request.RequestInfo) http.Handler {
return http.HandlerFunc(h)
}

func (f *Factory) Restart(namespace string) http.Handler {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can consider moving these special functions to a specific file for management, like: ext_pods_handler.go

Copy link
Member Author

Choose a reason for hiding this comment

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

ext_pods_handler.go

ok

edge/pkg/metamanager/metaserver/server.go
@@ -183,7 +183,11 @@ func (ls *MetaServer) BuildBasicHandler() http.Handler {
case reqInfo.Verb == "list", reqInfo.Verb == "watch":
ls.Factory.List().ServeHTTP(w, req)
case reqInfo.Verb == "create":
ls.Factory.Create(reqInfo).ServeHTTP(w, req)
if reqInfo.Name == "restart" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if reqInfo.SubResource == "restart"?

Copy link
Member Author

Choose a reason for hiding this comment

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

if reqInfo.SubResource == "restart"?

The Subresource here is nil, Name is "restart".

@luomengY luomengY force-pushed the kead_ctl_client-go branch 3 times, most recently from 5930cfb to 56962c7 Compare May 29, 2024 11:59
WillardHu
WillardHu reviewed May 30, 2024
View reviewed changes
pkg/metaserver/constants.go Outdated
@@ -59,6 +59,11 @@ type PatchInfo struct {
Subresources []string
}

type RestartInfo struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Defining structures in the constants.go is not good, you can add a file types.go to maintain them, the struct RestartResponse can also be moved over to this file, it is not good that the keadm module depend on the metaserver module package.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@luomengY
Switch the keadm ctl client to client go and add the restful interfac…
ee4de68 
…e for restart pod.

Signed-off-by: luomengY <2938893385@qq.com>
@WillardHu
Copy link
Collaborator

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillardHu WillardHu WillardHu left review comments

@FengyunPan2 FengyunPan2 Awaiting requested review from FengyunPan2

@fisherxu fisherxu Awaiting requested review from fisherxu

@subpathdev subpathdev Awaiting requested review from subpathdev

Assignees
No one assigned
Labels
kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@luomengY @kubeedge-bot @WillardHu