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

support add fd parallelly #719

Merged
merged 1 commit into from
Sep 6, 2018
Merged

Conversation

yanxuean
Copy link
Contributor

@yanxuean yanxuean commented Jul 16, 2018

create new connect for every fd request

Signed-off-by: yanxuean yan.xuean@zte.com.cn

virtletv1.0
hit a problem.
The request and response of AddFd will be mismatch when we create plenty of pod(e.g. 10).

{"log":"E0508 07:24:05.205665   10024 manager.go:220] Error when adding pod yy-8 (d6f96ee4-5290-11e8-8d7b-fa163e15c857) to CNI network: fd key mismatch in the server response\n","stream":"stderr","time":"2018-05-08T07:24:05.205934204Z"}
{"log":"nsfix reexec: pid 28438: entering the namespaces of target pid 1\n","stream":"stderr","time":"2018-05-08T07:24:05.244085618Z"}

{"log":"I0508 07:24:05.869500   10033 nettools.go:716] Opening tap interface \"tap0\" for link \"virtlet-eth0\"\n","stream":"stderr","time":"2018-05-08T07:24:05.87005565Z"}
{"log":"I0508 07:24:05.869571   10033 nettools.go:721] Adding interface \"virtlet-eth0\" as \"tap0\"\n","stream":"stderr","time":"2018-05-08T07:24:05.870131321Z"}
{"log":"2018/05/08 07:24:06 transport: http2Server.HandleStreams failed to receive the preface from client: EOF\n","stream":"stderr","time":"2018-05-08T07:24:06.092375098Z"}
{"log":"E0508 07:24:06.371938   10024 manager.go:220] Error when adding pod yy-7 (d601d1c1-5290-11e8-8d7b-fa163e15c857) to CNI network: fd key mismatch in the server response\n","stream":"stderr","time":"2018-05-08T07:24:06.372188913Z"}
{"log":"nsfix reexec: pid 28549: entering the namespaces of target pid 1\n","stream":"stderr","time":"2018-05-08T07:24:06.415327404Z"}


This change is Reviewable

@jellonek
Copy link
Contributor

There is no more a need to do that for AddFd over a unix socket connection (finally tapmanager is now internal to virtlet process goroutine) and @ivan4th will provide shortly a PR in which this part of code will be changed.

Still unix domain sockets will be used for tapmanger<->vmwrapper communication so this fix can be relevant.

Copy link
Contributor

@jellonek jellonek left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: 0 of 2 LGTMs obtained


pkg/tapmanager/fdserver.go, line 429 at r1 (raw file):

}

func (c *FDClient) connect() (*net.UnixConn, error) {

There is no need to return there conn as it can be set in c.conn already, so in Connect there will be also no need to assign conn to c.conn and do the error checking, as whole 6 lines added to Connect can then be replaced by single return c.connect().


pkg/tapmanager/fdserver.go, line 443 at r1 (raw file):

// Close closes the connection to FDServer
func (c *FDClient) close(conn *net.UnixConn) error {

Above suggested change would mean that this one is unnecessary...


pkg/tapmanager/fdserver.go, line 453 at r1 (raw file):

func (c *FDClient) request(hdr *fdHeader, data []byte) (*fdHeader, []byte, []byte, error) {
	conn, err := c.connect()
	if err != nil {

... also combining these 2 lines...


pkg/tapmanager/fdserver.go, line 456 at r1 (raw file):

		return nil, nil, nil, errors.New("not connected")
	}
	defer c.close(conn)

... with there call to defer c.conn.Close()

so rest of changes in this func from c.conn to conn will also be unnecessary.

@yanxuean
Copy link
Contributor Author

Maybe we should wait for @ivan4th 's new PR at first:)

@yanxuean
Copy link
Contributor Author

/HOLD

@yanxuean yanxuean changed the title support add fd parallelly [WIP]support add fd parallelly Jul 18, 2018
@yanxuean
Copy link
Contributor Author

There is no need to export Connect() function to user, I remove it.
We hope FDClient.request is atomic, so we can start a connect in FDClient.request by self.

create new connect for every fd request

Signed-off-by: yanxuean <yan.xuean@zte.com.cn>
@yanxuean
Copy link
Contributor Author

@jellonek PTAL

@yanxuean yanxuean changed the title [WIP]support add fd parallelly support add fd parallelly Aug 28, 2018
@pigmej pigmej requested a review from ivan4th September 4, 2018 19:26
Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Reviewable status: 1 change requests, 0 of 2 approvals obtained (waiting on @yanxuean and @ivan4th)

Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r2.
Reviewable status: 1 change requests, 0 of 2 approvals obtained (waiting on @yanxuean and @ivan4th)

@yanxuean
Copy link
Contributor Author

yanxuean commented Sep 6, 2018

@jellonek PTAL

Copy link
Contributor

@lukaszo lukaszo left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 change requests, 1 of 2 approvals obtained (waiting on @yanxuean)

@lukaszo lukaszo merged commit 7264829 into Mirantis:master Sep 6, 2018
@yanxuean yanxuean deleted the parallel-add-fd branch September 6, 2018 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants