-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
support kubectl attach in KubeEdge #4734
Conversation
Signed-off-by: Shelley-BaoYue <baoyue2@huawei.com> (cherry picked from commit 7cee5b0) Signed-off-by: dehaocheng <dehaocheng@kubesphere.io>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
@Shelley-BaoYue Can you give us some advice? |
// first send connect message | ||
connector, err := ah.SendConnection() | ||
if err != nil { | ||
klog.Errorf("%s send %s info error %v", ah.String(), stream.MessageTypeExecConnect, err) |
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.
Try changing it to MessageTypeAttachConnect
.
I will debug it in my local environment, it may take some time to give you feedback. |
a69e642
to
1f30e7c
Compare
edge/pkg/edgestream/tunnel.go
Outdated
@@ -104,6 +119,10 @@ func (s *TunnelSession) ServeConnection(m *stream.Message) { | |||
if err := s.serveMetricsConnection(m); err != nil { | |||
klog.Errorf("Serve Metrics connection error %s", m.String()) | |||
} | |||
case stream.MessageTypeAttachConnect: | |||
if err := s.serveContainerAttachConnection(m); err != nil { | |||
klog.Errorf("Serve Metrics connection error %s", m.String()) |
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.
log info error, need to be Serve Attach
pkg/stream/edgedattachconnection.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
return NewMessage(ah.MessID, MessageTypeExecConnect, data), 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.
MessageTypeExecConnect
--> MessageTypeAttachConnect
"context" | ||
"errors" | ||
"fmt" | ||
"github.com/kubeedge/kubeedge/common/constants" |
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.
The import sequence needs to be adjusted. Generally, the import sequence is the standard lib, third-party lib, and local
lib.
pkg/stream/edgedattachconnection.go
Outdated
"errors" | ||
"fmt" | ||
"io" | ||
"k8s.io/apimachinery/pkg/util/httpstream/spdy" |
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.
Adjusting the import sequence
pkg/stream/edgedattachconnection.go
Outdated
} | ||
|
||
func (ah *EdgedAttachConnection) receiveFromCloudStream(con net.Conn, stop chan struct{}) { | ||
// todo: read msg from cloudstream |
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.
todo
flag can be removed now
pkg/stream/edgedattachconnection.go
Outdated
} | ||
|
||
func (ah *EdgedAttachConnection) write2CloudStream(tunnel SafeWriteTunneler, con net.Conn, stop chan struct{}) { | ||
// todo: write response to tunnel |
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.
ditto
pkg/stream/edgedattachconnection.go
Outdated
} | ||
|
||
func (ah *EdgedAttachConnection) Serve(tunnel SafeWriteTunneler) error { | ||
// todo: serve msg from clousstream, send req to edged and write response to tunnel |
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.
ditto
@wenchajun I have successfully debugged locally. You can adjust it according to my modification suggestions and try again. |
1f30e7c
to
9771ed7
Compare
Thanks for your reply, I have fixed the problems. |
9771ed7
to
e338a94
Compare
In the CI just now, I see that the error is reported as an extra blank line added. I modified it, can you help me rerun this CI? @Shelley-BaoYue |
OK. And the description of this pr could be added, including associated issues #4424 , proposal: #4690 . |
pkg/stream/constants.go
Outdated
@@ -8,6 +8,7 @@ const ( | |||
const ( | |||
MessageTypeLogsConnect MessageType = iota | |||
MessageTypeExecConnect | |||
MessageTypeAttachConnect |
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.
Here is a compatibility problem. We hope CloudCore can be compatible with EdgeCore of earlier versions generally. If MessageTypeAttachConnect
is inserted here, when CloudCore send MessageTypeData
,EdgeCore will identify it as MessageTypeRemoveConnect
. I suggest adding MessageTypeAttachConnect
at the end, and the judgment at line 195 of edge/pkg/edgestream/tunnel
needs to be changed accordingly.
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.
fixed
e338a94
to
32dce2e
Compare
edge/pkg/edgestream/tunnel.go
Outdated
@@ -172,7 +191,7 @@ func (s *TunnelSession) Serve() error { | |||
return fmt.Errorf("close tunnel stream connection, error:%s", string(mess.Data)) | |||
} | |||
|
|||
if mess.MessageType < stream.MessageTypeData { | |||
if (mess.MessageType < stream.MessageTypeData) || (mess.MessageType == stream.MessageTypeAttachConnect) { |
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.
mess.MessageType >= stream.MessageTypeAttachConnect
maybe better for expansion
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.
const (
MessageTypeLogsConnect MessageType = iota
MessageTypeExecConnect
MessageTypeMetricConnect
MessageTypeData
MessageTypeRemoveConnect
MessageTypeCloseConnect
MessageTypeAttachConnect
What should this field be adjusted to be greater than or equal to if it is increased by 1 each time here?
(mess.MessageType < stream.MessageTypeData) || (mess.MessageType >= stream.MessageTypeAttachConnect)
?
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.
const ( MessageTypeLogsConnect MessageType = iota MessageTypeExecConnect MessageTypeMetricConnect MessageTypeData MessageTypeRemoveConnect MessageTypeCloseConnect MessageTypeAttachConnect
What should this field be adjusted to be greater than or equal to if it is increased by 1 each time here?
I guess that in order to ensure compatibility, new message type will be added at the end. If we add more capabilities of kubectl in the future, we don't need to change that judgment, just using mess.MessageType >= stream.MessageTypeAttachConnect
is OK. It's just my suggestion.
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.
I get it .thanks
Signed-off-by: dehaocheng <dehaocheng@kubesphere.io>
32dce2e
to
8bc26fc
Compare
All changes have been made, are there any questions so far? @Shelley-BaoYue |
It looks like there is a slight error in action, please check codecov/codecov-action#598 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Currently, in a native kubernetes cluster,
kubectl exec
command can execute a command in a container.kubectl logs
command can print the logs for a container in a pod.kubectl attach
command can attach to a running container.Users can execute these commands in the cloud and do not need to operate on the nodes.
we plan to implement
kubectl exec
,kubectl logs
andkubectl attach
feature based on edge computing scenarios.These two commands(
kubectl exec
,kubectl logs
) are currently integrated, and this PR is to integrate thekubectl attach
command.Which issue(s) this PR fixes:
Fixes #4424
Special notes for your reviewer:
Does this PR introduce a user-facing change?: