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.

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

Fix: github workflow upload logs dir error #917

Merged
merged 8 commits into from
Nov 23, 2020
Merged

Fix: github workflow upload logs dir error #917

merged 8 commits into from
Nov 23, 2020

Conversation

9547
Copy link
Contributor

@9547 9547 commented Nov 17, 2020

What problem does this PR solve?

As #908 set the default log dir to $TIUP_HOME/logs, we need to change the control's log path. And one more thing, the SSHD process was not activated in host tiup-cluster-control, and the scp always failed, such as:

Run docker exec tiup-cluster-control bash /tiup-cluster/tests/tiup-cluster/script/pull_log.sh /tiup-cluster/logs
172.19.0.100
ssh: connect to host 172.19.0.100 port 22: Connection refused
172.19.0.101
Warning: Permanently added '172.19.0.101' (ECDSA) to the list of known hosts.
172.19.0.102
Warning: Permanently added '172.19.0.102' (ECDSA) to the list of known hosts.
172.19.0.103
Warning: Permanently added '172.19.0.103' (ECDSA) to the list of known hosts.
172.19.0.104
Warning: Permanently added '172.19.0.104' (ECDSA) to the list of known hosts.
172.19.0.105
Warning: Permanently added '172.19.0.105' (ECDSA) to the list of known hosts.

Replace the scp with local cp for control's node.

What is changed and how it works?

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
Copy link

codecov-io commented Nov 17, 2020 •
edited
Loading

Codecov Report

Merging #917 (28a9fcd) into master (abfd109) will decrease coverage by 3.78%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #917      +/-   ##
==========================================
- Coverage   55.22%   51.44%   -3.79%     
==========================================
  Files         261      261              
  Lines       19253    19253              
==========================================
- Hits        10632     9904     -728     
- Misses       6917     7738     +821     
+ Partials     1704     1611      -93
Flag Coverage Δ
cluster 38.28% <ø> (-5.22%) ⬇️
dm 24.18% <ø> (+0.06%) ⬆️
integrate 46.26% <ø> (-3.80%) ⬇️
playground 20.23% <ø> (ø)
tiup 17.17% <ø> (ø)
unittest 21.64% <ø> (ø)

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

Impacted Files Coverage Δ
components/cluster/command/check.go 6.37% <0.00%> (-72.82%) ⬇️
pkg/cluster/task/limits.go 0.00% <0.00%> (-68.75%) ⬇️
pkg/cluster/task/sysctl.go 0.00% <0.00%> (-66.67%) ⬇️
components/cluster/command/audit.go 27.27% <0.00%> (-54.55%) ⬇️
pkg/cluster/operation/check.go 0.00% <0.00%> (-51.95%) ⬇️
pkg/cluster/task/rmdir.go 0.00% <0.00%> (-50.00%) ⬇️
pkg/cluster/operation/operation.go 34.78% <0.00%> (-43.48%) ⬇️
components/cluster/command/rename.go 25.00% <0.00%> (-41.67%) ⬇️
components/cluster/command/stop.go 38.46% <0.00%> (-30.77%) ⬇️
components/cluster/command/enable.go 38.46% <0.00%> (-30.77%) ⬇️
... and 25 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 abfd109...28a9fcd. Read the comment docs.

@9547 9547 force-pushed the fix/github-workflow-upload-logs-dir-error branch from 6cd5a66 to bc012cb Compare November 18, 2020 14:35
lucklove
lucklove reviewed Nov 19, 2020
View reviewed changes
.github/workflows/integrate-dm.yaml
@@ -66,6 +66,23 @@ jobs:
# ref: https://stackoverflow.com/questions/43099116/error-the-input-device-is-not-a-tty
docker exec tiup-cluster-control bash /tiup-cluster/tests/tiup-dm/run.sh ${{matrix.cases}}

- name: Collect component log
Copy link
Member

Choose a reason for hiding this comment

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

Why upload log? I think just replacing cat ./tests/*.log with a correct log path will be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As cluster

tiup/.github/workflows/integrate-cluster-cmd.yaml

Line 70 in cc41ad5

docker exec tiup-cluster-control bash /tiup-cluster/tests/tiup-cluster/script/pull_log.sh /tiup-cluster/logs
uploads it, so I think dm should upload it too.

Copy link
Member

Choose a reason for hiding this comment

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

OK, How about add cat /path/to/*.log too? By this way we can check what happened without downloading the logs when some error occurs in test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's a good idea, as the debug file may be very large and may not be human-readable on the terminal, BTW, I'll give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to adjust my strategy to look like this:if the test passed, no log were uploaded and print out; else upload and print it.

Copy link
Member

Choose a reason for hiding this comment

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

That's cool

@9547 9547 force-pushed the fix/github-workflow-upload-logs-dir-error branch from 5b48c1b to 5c09a7c Compare November 19, 2020 14:41
lucklove
lucklove reviewed Nov 20, 2020
View reviewed changes
.github/workflows/integrate-cluster-cmd.yaml Outdated
run: |
docker exec tiup-cluster-control bash /tiup-cluster/tests/tiup-cluster/script/pull_log.sh /tiup-cluster/logs
ls ${{ env.working-directory }}
cat /tiup-cluster/logs/*.log || true
Copy link
Member

Choose a reason for hiding this comment

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

Prefer to move this line to the Output cluster debug log step because its' name is Output cluster debug log...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your advice

@9547 9547 force-pushed the fix/github-workflow-upload-logs-dir-error branch from 5c09a7c to f4e1a4f Compare November 22, 2020 12:44
lucklove
lucklove approved these changes Nov 23, 2020
View reviewed changes
Copy link
Member

@lucklove lucklove left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 23, 2020
@lucklove
Copy link
Member

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 23, 2020
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 922
  • 926

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit baace08 into pingcap:master Nov 23, 2020
@9547 9547 deleted the fix/github-workflow-upload-logs-dir-error branch November 23, 2020 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucklove lucklove lucklove approved these changes

Assignees
No one assigned
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@9547 @codecov-io @lucklove @ti-srebot @jsvisa