Skip to content

support configurable stream-port to virtlet server instead of default 10010 #734

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
Merged
merged 1 commit into from
Aug 24, 2018
Merged

support configurable stream-port to virtlet server instead of default 10010 #734

merged 1 commit into from
Aug 24, 2018

Conversation

volume-ji
Copy link
Contributor

@volume-ji volume-ji commented Aug 9, 2018 •
edited by pigmej
Loading

What this PR does / why we need it:

  1. virtlet server use default port 10010 conflicting with containerd server port 10010, so I strongly suggest that It shoud be a configurable flag, for example: "--stream-port"
  2. I add argument “$*” in script start.sh to accept virtlet-ds.yaml parameter, which means we can use yaml filed like
    "image: mirantis/virtlet
    command:
  • /start.sh
  • --stream-port=xxx"
    to set ourself streamPort.

Which issue(s) this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #NONE

Special notes for your reviewer:
NONE
Release note:
NONE

This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Aug 9, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

jellonek
jellonek suggested changes Aug 9, 2018
View reviewed changes
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.

Small nits and rest LGTM

Reviewed 3 of 5 files at r1, 2 of 2 files at r2.
Reviewable status: 0 of 2 LGTMs obtained

images/image_skel/start.sh, line 24 at r2 (raw file):

    verbose="--v ${VIRTLET_LOGLEVEL}"
fi
/usr/local/bin/virtlet ${verbose} $*

Missing EOL

pkg/api/virtlet.k8s/v1/virtletconfig.go, line 67 at r2 (raw file):

	// LogLevel specifies the log level to use
	LogLevel *int `json:"logLevel,omitempty"`
	// Configrable stream port in command line

Docstring should start with name of the field.

pkg/config/config.go, line 104 at r2 (raw file):

	// by "+" here (it's only for doc)
	fs.addIntField("logLevel", "+v", "", "Log level to use", logLevelEnv, 1, 0, math.MaxInt32, &c.LogLevel)
	//add configrable stream port flag

Please add space after // as in rest comments.

@jellonek
Copy link
Contributor

jellonek commented Aug 9, 2018

When you are changing anything in virtletconfig.go you have to call later ./build/cmd.sh update-generated.

@volume-ji
Copy link
Contributor Author

OK, I get it

xiangpengzhao
xiangpengzhao suggested changes Aug 10, 2018
View reviewed changes
Copy link

@xiangpengzhao xiangpengzhao left a comment

Choose a reason for hiding this comment

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

You also need to fix the tests which are influenced :)

pkg/stream/server.go Outdated
@@ -27,6 +27,7 @@ import (
"github.com/golang/glog"
knet "k8s.io/apimachinery/pkg/util/net"
"k8s.io/kubernetes/pkg/kubelet/server/streaming"
"strconv"
Copy link

Choose a reason for hiding this comment

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

nit: move "strconv" above "syscall"

@volume-ji
Copy link
Contributor Author

@jellonek ，what happened for my commiting, why did it have failures about deploying kubernetes-dashboard. can you give me a hand? very thanks.

jellonek
jellonek reviewed Aug 10, 2018
View reviewed changes
glide.lock Outdated
@@ -103,7 +103,7 @@ imports:
- protoc-gen-gogo/descriptor
- sortkeys
- name: github.com/golang/glog
version: 23def4e6c14b4da8ac2ed8007337bc5eb5007998
version: 44145f04b68cf362d9c4df2182967c2275eaefed
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this change?

glide.lock Outdated
@@ -208,7 +208,7 @@ imports:
- name: github.com/vishvananda/netns
version: 8ba1072b58e0c2a240eb5f6120165c7776c3e7b8
- name: go.universe.tf/netboot
version: cc33920b4f3296801a64d731d269978116f40d92
version: 870d490bda51cd67f4d53593ce4219d47db00a0d
Copy link
Contributor

Choose a reason for hiding this comment

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

As above.

@jellonek
Copy link
Contributor

Issue with dashboard looks like something connected with port forwarding. Will try to dig it out later today.

@pigmej
Copy link
Contributor

pigmej commented Aug 20, 2018

@jellonek I restarted jobs.

@jellonek
Copy link
Contributor

@volume-ji you need to rebase this PR on top of master, we have already fixed this issue with not accessible port.

@volume-ji
Copy link
Contributor Author

OK, I get it

jellonek
jellonek approved these changes Aug 22, 2018
View reviewed changes
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.

Looks ok, but we are still waiting for rebase (please ignore failure of tests depending on ceph/demo - it will be fixed later).

<type>adapt: support configurable stream-port to virtlet server inste…
5d93aa1 
…ad of default 10010

<type>view: modify corresponding doc file.

<type>UTest: modify UTest output file.

<type>view: modify the location of pakcage, system package should be in front of other packages.

<type>view: modify location of package that "st" shoud be above "sy"

<type>view: rollback glide.lock file which glide tool lead to.
@volume-ji
Copy link
Contributor Author

I have already featched and rebased code.

jellonek
jellonek reviewed Aug 23, 2018
View reviewed changes
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.

:lgtm:

Reviewed 4 of 5 files at r3, 27 of 27 files at r4.
Reviewable status: 1 of 2 approvals obtained (waiting on @volume-ji and @xiangpengzhao)

ivan4th
ivan4th approved these changes Aug 24, 2018
View reviewed changes
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.

Reviewed 4 of 5 files at r3, 27 of 27 files at r4.
Reviewable status: 1 of 2 approvals obtained (waiting on @volume-ji and @xiangpengzhao)

ivan4th
ivan4th approved these changes Aug 24, 2018
View reviewed changes
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:

Reviewable status: :shipit: complete! 2 of 2 approvals obtained (waiting on @volume-ji and @xiangpengzhao)

@ivan4th ivan4th merged commit 6ac1a65 into Mirantis:master Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
3 more reviewers

@xiangpengzhao xiangpengzhao xiangpengzhao requested changes

@ivan4th ivan4th ivan4th approved these changes

@jellonek jellonek jellonek approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@volume-ji @CLAassistant @jellonek @pigmej @ivan4th @xiangpengzhao