dm,cluster: display with uptime returned #1231

9547 · 2021-03-21T14:33:56Z

What problem does this PR solve?

implement #1223

What is changed and how it works?

display the uptime of the instance,

if this instance is up, then uptime means the alive time, else is the inactive time.

see #1223 for more detail

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Has exported function/method change
Has exported variable/fields change
Has interface methods change
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Release notes:

NONE

codecov-io · 2021-03-21T15:12:52Z

Codecov Report

Merging #1231 (2248366) into master (37487dc) will increase coverage by 0.43%.
The diff coverage is 82.06%.

❗ Current head 2248366 differs from pull request most recent head 80ee2e0. Consider uploading reports for the commit 80ee2e0 to get more accurate results

@@            Coverage Diff             @@
##           master    #1231      +/-   ##
==========================================
+ Coverage   53.42%   53.85%   +0.43%     
==========================================
  Files         289      277      -12     
  Lines       20687    20355     -332     
==========================================
- Hits        11051    10963      -88     
+ Misses       7903     7668     -235     
+ Partials     1733     1724       -9

Flag	Coverage Δ
cluster	`45.11% <81.15%> (+0.16%)`	⬆️
dm	`26.61% <47.22%> (+0.12%)`	⬆️
integrate	`50.85% <82.06%> (+2.96%)`	⬆️
playground	`?`
tiup	`16.34% <0.00%> (-0.06%)`	⬇️
unittest	`22.68% <1.37%> (-0.16%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/cluster/operation/operation.go	`78.26% <ø> (ø)`
pkg/cluster/spec/spec.go	`88.92% <ø> (ø)`
pkg/cluster/manager/display.go	`79.74% <70.00%> (-2.78%)`	⬇️
pkg/cluster/spec/util.go	`77.77% <70.37%> (-4.45%)`	⬇️
components/cluster/command/display.go	`29.41% <100.00%> (+1.05%)`	⬆️
components/dm/command/display.go	`63.63% <100.00%> (+1.73%)`	⬆️
components/dm/spec/logic.go	`78.65% <100.00%> (ø)`
pkg/cluster/spec/alertmanager.go	`66.12% <100.00%> (+1.72%)`	⬆️
pkg/cluster/spec/cdc.go	`82.75% <100.00%> (ø)`
pkg/cluster/spec/drainer.go	`58.76% <100.00%> (ø)`
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37487dc...80ee2e0. Read the comment docs.

components/dm/spec/logic.go

pkg/cluster/manager/display.go

pkg/cluster/spec/instance.go

components/cluster/command/display.go

components/dm/command/display.go

9547 · 2021-03-24T16:07:27Z

@AstroProfundis @lucklove PTAL

lucklove · 2021-03-25T06:53:53Z

pkg/cluster/manager/display.go

-		if status == "-" {
+		since := "-"
+		if !opt.ShowUptime {
+			since = formatInstanceSince(ins.Uptime(tlsCfg))


I'm a little confused, why call ins.Uptime even if the user didn't specify --show-uptime?

Looks like the opposite logical ...

lucklove · 2021-03-25T06:57:32Z

pkg/cluster/manager/display.go

+		}
+
+		// Query the service status and uptime
+		if status == "-" || (opt.ShowUptime && since == "-") {


It seems this check is not necessary since we checked if status == "-" and if opt.ShowUptime && since == "-" latter.

Sorry, the previous check is reversed, so the check here is necessary, if user specifies opt.ShowUptime but since is empty(not implemented or server was down), we try to fetch it via ssh-systemctl-status)

I means that you have checked this in latter codes:

https://github.com/pingcap/tiup/pull/1231/files#diff-89b17f92d341c20020ab4246afa2eed55ab6aae2ce14541cfd40168e39b0c014R253
https://github.com/pingcap/tiup/pull/1231/files#diff-89b17f92d341c20020ab4246afa2eed55ab6aae2ce14541cfd40168e39b0c014R262

No, the check is necessary.

The previous check is A or B,
because they all need to invoke operator.GetServiceStatus,
and later for condition A I'm trying to get the status(if not this condition, don't need to rewrite status), and for B to get the since.

lucklove · 2021-03-26T08:07:20Z

It's better to hide Since column when the user doesn't specify --uptime

lucklove · 2021-03-26T08:08:52Z

It seems the format is not consistent when the data source different from metric and systemd.

It's better to make it consistent:

Maybe we can parse the string returned by systemd to int and format it again
Or we can use the same format with systemd

@9547 @AndreMouche What do you think about it?

9547 · 2021-03-27T03:17:01Z

It seems the format is not consistent when the data source different from metric and systemd.

It's better to make it consistent:

Maybe we can parse the string returned by systemd to int and format it again

Or we can use the same format with systemd

@9547 @AndreMouche What do you think about it?

Agree with it, keep the format consistent, I'll try to parse the output of systemd's output.

+1 to hide Since column if --uptime not specified.

lucklove · 2021-03-29T04:52:46Z

/lgtm

ti-chi-bot · 2021-03-29T04:52:48Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lucklove

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

lucklove · 2021-03-29T04:52:58Z

/merge

ti-chi-bot · 2021-03-29T04:53:00Z

This pull request has been accepted and is ready to merge.

Commit hash: 055bb80

ti-chi-bot · 2021-03-29T04:53:07Z

@9547: Your PR was out of date, I have automatically updated it for you.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot requested review from AstroProfundis and lonng March 21, 2021 14:34

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 21, 2021

AstroProfundis reviewed Mar 22, 2021

View reviewed changes

components/dm/spec/logic.go Show resolved Hide resolved

pkg/cluster/manager/display.go Outdated Show resolved Hide resolved

pkg/cluster/spec/instance.go Show resolved Hide resolved

AstroProfundis added category/usability Categorizes issue or PR as a usability enhancement. type/new-feature Categorizes pr as related to a new feature. labels Mar 22, 2021

AstroProfundis added this to the v1.5.0 milestone Mar 22, 2021

AstroProfundis reviewed Mar 23, 2021

View reviewed changes

components/cluster/command/display.go Show resolved Hide resolved

components/dm/command/display.go Show resolved Hide resolved

9547 requested a review from AstroProfundis March 24, 2021 16:06

lucklove reviewed Mar 25, 2021

View reviewed changes

9547 force-pushed the feature/display-uptime branch 2 times, most recently from 1d9b083 to 9257678 Compare March 25, 2021 15:53

jsvisa added 13 commits March 27, 2021 11:21

*: add github.com/prometheus/common

a604517

cluster: query uptime by metric api

0229105

cluster/spec: add UptimeFn

2aff8af

cluster/spec: implement Uptime interface

69c505e

cluster/manager: print Uptime

7968b9f

cluster/manager: display uptime from systemctl

1b39b5b

cluster: query alertmanager,grafana,prometheus status from http api

c9680d2

cluster/manager: inactive service should highlighted

2ff2565

cluster/spec: uptimeByHost -> UptimeByHost

38ac927

dm: implement UptimeFn

856185f

cluster: fix the typo

79acf90

cluster/spec: dry

0d48111

cluster,dm: add --uptime for display

56519fb

jsvisa added 8 commits March 27, 2021 11:21

cluster/spec: uptime -> since

e3fca81

cluster: opt.ShowUptime is opposite

de7df9c

tests/cluster: add --uptime test case

4220365

tests/dm: add display --uptime testcase

3b06849

tests/cluster: sleep 10 to ensure --uptime success

63680cb

tests/cluster: the cluster maybe not up, don't check process time

7155c0d

cluster/display: hide Since if not --uptime

a093c6b

cluster/display: parse systemctl since str

055bb80

9547 force-pushed the feature/display-uptime branch from 6cde2c0 to 055bb80 Compare March 27, 2021 04:15

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 27, 2021

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 29, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 29, 2021

Merge branch 'master' into feature/display-uptime

80ee2e0

ti-chi-bot merged commit da012e4 into pingcap:master Mar 29, 2021

9547 deleted the feature/display-uptime branch April 5, 2021 22:41

9547 mentioned this pull request Apr 8, 2021

display component uptime #1223

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dm,cluster: display with uptime returned #1231

dm,cluster: display with uptime returned #1231

9547 commented Mar 21, 2021

codecov-io commented Mar 21, 2021 •

edited

Loading

9547 commented Mar 24, 2021

lucklove Mar 25, 2021

9547 Mar 25, 2021 •

edited

Loading

lucklove Mar 25, 2021

9547 Mar 25, 2021

lucklove Mar 25, 2021

9547 Mar 25, 2021

lucklove commented Mar 26, 2021

lucklove commented Mar 26, 2021 •

edited

Loading

9547 commented Mar 27, 2021

lucklove commented Mar 29, 2021

ti-chi-bot commented Mar 29, 2021

lucklove commented Mar 29, 2021

ti-chi-bot commented Mar 29, 2021

ti-chi-bot commented Mar 29, 2021

dm,cluster: display with uptime returned #1231

dm,cluster: display with uptime returned #1231

Conversation

9547 commented Mar 21, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

codecov-io commented Mar 21, 2021 • edited Loading

Codecov Report

9547 commented Mar 24, 2021

lucklove Mar 25, 2021

Choose a reason for hiding this comment

9547 Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

lucklove Mar 25, 2021

Choose a reason for hiding this comment

9547 Mar 25, 2021

Choose a reason for hiding this comment

lucklove Mar 25, 2021

Choose a reason for hiding this comment

9547 Mar 25, 2021

Choose a reason for hiding this comment

lucklove commented Mar 26, 2021

lucklove commented Mar 26, 2021 • edited Loading

9547 commented Mar 27, 2021

lucklove commented Mar 29, 2021

ti-chi-bot commented Mar 29, 2021

lucklove commented Mar 29, 2021

ti-chi-bot commented Mar 29, 2021

ti-chi-bot commented Mar 29, 2021

codecov-io commented Mar 21, 2021 •

edited

Loading

9547 Mar 25, 2021 •

edited

Loading

lucklove commented Mar 26, 2021 •

edited

Loading