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

fix(cluster/audit): compatible with old second based ts #882

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

9547
Copy link
Contributor

@9547 9547 commented Nov 3, 2020

What problem does this PR solve?

The old audit's filename was based on second, after #879 merged, the filename changed to nanosecond based. So this PR fixes the difference between those two situations.

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 3, 2020

Codecov Report

Merging #882 into master will increase coverage by 0.06%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #882      +/-   ##
==========================================
+ Coverage   53.26%   53.33%   +0.06%     
==========================================
  Files         263      263              
  Lines       19042    19047       +5     
==========================================
+ Hits        10143    10158      +15     
+ Misses       7321     7315       -6     
+ Partials     1578     1574       -4     
Flag Coverage Δ
cluster 45.28% <66.66%> (+0.08%) ⬆️
dm 25.32% <77.77%> (+0.07%) ⬆️
integrate 48.15% <77.77%> (+0.06%) ⬆️
playground 22.17% <ø> (ø)
tiup 10.76% <ø> (ø)
unittest 21.13% <77.77%> (+0.20%) ⬆️

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

Impacted Files Coverage Δ
pkg/cluster/audit/audit.go 63.46% <77.77%> (-0.37%) ⬇️
pkg/cluster/api/pdapi.go 60.06% <0.00%> (+1.23%) ⬆️
pkg/cluster/api/dmapi.go 60.00% <0.00%> (+1.73%) ⬆️
pkg/cluster/spec/pd.go 70.98% <0.00%> (+2.46%) ⬆️
pkg/cluster/template/scripts/pd.go 71.87% <0.00%> (+3.12%) ⬆️

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 871ae58...6139411. Read the comment docs.

@9547 9547 force-pushed the fix/audit-compatible-old-second branch from 9d40deb to b530f29 Compare November 3, 2020 16:31
@9547
Copy link
Contributor Author

9547 commented Nov 3, 2020

@lucklove It's curious why the cdc and dm-master failed to start, maybe due to Github CI's resource limit, the component was slow to startup, or other root cause.

Starting component dm-master
	Starting instance dm-master 172.19.0.101:8261
retry error: operation timed out after 1m0s
	dm-master 172.19.0.101:8261 failed to start: timed out waiting for port 8261 to be started after 1m0s, please check the log of the instance

Error: failed to start dm-master: 	dm-master 172.19.0.101:8261 failed to start: timed out waiting for port 8261 to be started after 1m0s, please check the log of the instance: timed out waiting for port 8261 to be started after 1m0s

Verbose debug logs has been written to /tiup-cluster/tests/tiup-dm/logs/tiup-cluster-debug-2020-11-03-16-56-18.log.

and

retry error: operation timed out after 2m0s
	cdc 172.19.0.104:8300 failed to start: timed out waiting for port 8300 to be started after 2m0s, please check the log of the instance

Error: failed to start cdc: 	cdc 172.19.0.104:8300 failed to start: timed out waiting for port 8300 to be started after 2m0s, please check the log of the instance: timed out waiting for port 8300 to be started after 2m0s

Verbose debug logs has been written to /tiup-cluster/tests/tiup-cluster/logs/tiup-cluster-debug-2020-11-03-16-56-17.log.

Seems the cluster's log is uploaded

- name: Upload component log
# if: steps.test.outputs.exit_code != 0
if: always()
uses: actions/upload-artifact@v1
with:
name: component_logs
path: ${{ env.working-directory }}/logs.tar.gz

But I don't where the log is stored, so could you please help me to dig into those logs(if you know it).

BTW, Seems there was no log inside tests/ dir, so those lines seems useless:

- name: Output cluster debug log
working-directory: ${{ env.working-directory }}
if: always()
run: |
pwd
docker ps
df -h
free -h
"cat ./tests/*.log" || true

IMO, we can docker exec into each tiup-cluster-n{1..5} node to print the logs inside /home/tidb/deploy/ to see the detail logs or upload those logs too, better to debug.

@lucklove
Copy link
Member

lucklove commented Nov 4, 2020

You are right, it's due to the limited resource, we can just re-run these failed jobs this time.

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 4, 2020
@lucklove lucklove merged commit 0c9ac6f into pingcap:master Nov 4, 2020
@9547 9547 deleted the fix/audit-compatible-old-second branch November 4, 2020 13:03
@lucklove lucklove added this to the v1.2.4 milestone Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants