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
[WIP][FEATURE] AutoRange parsing & format #1664
base: master
Are you sure you want to change the base?
Conversation
Please sign your commits following these rules: $ git clone -b "master" git@github.com:vdaviot/cli.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354128976
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f Amending updates the existing PR. You DO NOT need to open a new one. |
a0445ee
to
0035956
Compare
cli/command/bundlefile/bundlefile.go
Outdated
WorkingDir *string `json:",omitempty"` | ||
User *string `json:",omitempty"` | ||
Networks []string `json:",omitempty"` | ||
AutoRange map[string]map[string]string `json:",omitempty"` |
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.
Haven't looked at the feature as a whole, but we might want to skip updating the bundle file changes, as it never left the experimental stage, and is mainly obsoleted by the addition of stack deploy through compose-files
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.
Hi @thaJeztah,
In doubt i added it in the patch, didn't knew it was obsolete.
Will be removed in the next commit.
Hello @vdaviot, thank you for opening this PR.
|
@@ -13,11 +13,13 @@ const ( | |||
winOSType = "windows" | |||
defaultStatsTableFormat = "table {{.ID}}\t{{.Name}}\t{{.CPUPerc}}\t{{.MemUsage}}\t{{.MemPerc}}\t{{.NetIO}}\t{{.BlockIO}}\t{{.PIDs}}" | |||
winDefaultStatsTableFormat = "table {{.ID}}\t{{.Name}}\t{{.CPUPerc}}\t{{.MemUsage}}\t{{.NetIO}}\t{{.BlockIO}}" | |||
autoRangeStatsTableFormat = "table {{.ID}} {{.MemUsage}} {{.AutoRange}}" |
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.
Shouldn't you use \t
between columns?
table {{.ID}}\t{{.MemUsage}}\t{{.AutoRange}}
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.
Hi @silvin-lubecki
Concerning the lint / test / validate steps, i'm correcting those right now, should be commited today.
For the Format, i had some trouble with values not being placed at the right spot, that's why i did it like that, but i can totally change it with \t.
@@ -29,6 +31,7 @@ type StatsEntry struct { | |||
Container string | |||
Name string | |||
ID string | |||
AutoRange 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.
nit: I think you should configure your text editor/IDE to run a go fmt
each time you save a go file.
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.
Will do it ASAP
|
||
containerHeader = "CONTAINER" | ||
cpuPercHeader = "CPU %" | ||
netIOHeader = "NET I/O" | ||
blockIOHeader = "BLOCK I/O" | ||
autoRangeHeader = " Cur MIN/MAX Opti SOFT/HARD | CPUS / % / Time" |
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'm not familiar with the container stats formatter, but it feels weird having these spaces at the beginning and all these lower cases letters (all other headers are fully upper case).
It also feels like it is 3 different columns( Cur MIN/MAX
, Opti SOFT/HARD
and CPU / % / Time
)? Shouldn't they be 3 different headers? And what is this |
for before CPUS? (maybe I'm missing something 😅 )
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 are right, should have wrote everything in caps and yes they should, it totally make sense.
The logic behind this was:
I didn't wanted to modify the formatter too heavily implementing new vars that weren't useful for every users, so i did it kinda custom as you see. But the spacing is kinda bad so i should maybe do it differently. Implementing a var for each Headers is a good start, as it should help keep the spacing right
mem, memLimit float64 | ||
blkRead, blkWrite uint64 // Only used on Linux | ||
pidsStatsCurrent uint64 | ||
memoryAutoRange, cpuAutoRange 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.
nit: This file is missing a go fmt
@@ -164,6 +169,65 @@ func collect(ctx context.Context, s *Stats, cli client.APIClient, streamStats bo | |||
} | |||
} | |||
|
|||
func formatCPUAutoRange(ar map[string]string) string { | |||
numCPU, exist := ar["numCPU"] | |||
if !exist { |
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 guess this pattern can be extracted and factorized 😄
numCPU := getAutoRangeValue(ar, "numCPU")
percentOpti := getAutoRangeValue(ar, "percentOpti")
...
func addUnit(value string) string { | ||
lval := len(value) | ||
|
||
if lval > 9 { |
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 think there's already a vendored library for that. Can you check github.com/go-units/size.go
? 🤔
vendor.conf
Outdated
@@ -14,7 +14,7 @@ github.com/cpuguy83/go-md2man v1.0.8 | |||
github.com/davecgh/go-spew 346938d642f2ec3594ed81d874461961cd0faa76 # v1.1.0 | |||
github.com/dgrijalva/jwt-go a2c85815a77d0f951e33ba4db5ae93629a1530af | |||
github.com/docker/distribution 83389a148052d74ac602f5f1d62f86ff2f3c4aa5 | |||
github.com/docker/docker 50e63adf30d33fc1547527a4097c796cbe4b770f | |||
github.com/docker/docker 9e351d037bfb61a94de4241ced02cfbb7db8e3b3 https://github.com/vdaviot/engine.git |
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 will add a [WIP]
to this PR title as long as the PR is not merged on moby/moby. I think it's not desirable to merge it as is, depending on a personal fork of moby/moby. Same for swarmkit
.
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.
@silvin-lubecki yes, of course, thanks
cli/compose/convert/service.go
Outdated
case "threshold": | ||
threshold, _ = strconv.Atoi(v) | ||
if threshold <= 0 || threshold > 100 { | ||
return fmt.Errorf("Wrong vlaue for threshold") |
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.
nit: typo
Maybe fmt.Errorf("invalid threshold %q", v)
?
cli/compose/convert/service.go
Outdated
|
||
// Do checks on values | ||
if min > max && max != -1 || min < -1 || max < -1 { | ||
return fmt.Errorf("Empty/Wrong values for min or max value") |
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.
nit: fmt.Errorf("invalid min %q or max %q values", min, max)
@@ -161,6 +191,50 @@ func (c *statsContext) MarshalJSON() ([]byte, error) { | |||
return formatter.MarshalJSON(c) | |||
} | |||
|
|||
func (c *statsContext) CurrentMemoryMin() string { | |||
if c.s.CurrentMemoryMin == "--" { |
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 think there is still room for some factorization here 😺
Thanks @vdaviot for the modifications, that's better 👍 |
Codecov Report
@@ Coverage Diff @@
## master #1664 +/- ##
==========================================
- Coverage 56.26% 56.14% -0.13%
==========================================
Files 308 306 -2
Lines 21293 21052 -241
==========================================
- Hits 11980 11819 -161
+ Misses 8439 8383 -56
+ Partials 874 850 -24 |
@vdaviot your last commit |
And can you squash your commits? 👍 |
@silvin-lubecki After looking again, yes it seems weird, but it was generated by Is this the right way to generate a proper bindata.go ? Anyway i will revert it. |
There is a target in the But for your case I think you don't use the same |
And yes you're right about the squash, take it as a reminder then 😄 |
Signed-off-by: Philipp Schmied <pschmied@schutzwerk.com>
Signed-off-by: Valentin Daviot <valentin.daviot@alterway.fr> Signed-off-by: Valentin Daviot <valentin.daviot@alterway.fr>
This pull-request is related to:
This pull-request contains the necessary changes to parse the
autorange
key from.yml
and.dab
files (Contains new v3.8 json schema). It also contains a new format to visualize the predicted values:$ docker container stats --format autorange
I'm open to any suggestion regarding the implementation.
Cute animal