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

coredump: support options in pack subcommand #1059

Merged
merged 1 commit into from
Dec 23, 2024
Merged

coredump: support options in pack subcommand #1059

merged 1 commit into from
Dec 23, 2024

Conversation

elhimov
Copy link
Contributor

@elhimov elhimov commented Dec 9, 2024 •
edited
Loading

-e,--executable path Tarantool executable path
-p,--pid pid PID of the dumped process
-t,--time time Time of dump (seconds since the Epoch)

Closes #763

@elhimov elhimov force-pushed the gh-763-coredump-pack-support-options branch 4 times, most recently from d7ea70a to f4a5c2f Compare December 11, 2024 09:55
@elhimov elhimov mentioned this pull request Dec 13, 2024
Merged
@elhimov elhimov force-pushed the gh-763-coredump-pack-support-options branch from f4a5c2f to 339242d Compare December 13, 2024 10:00
@elhimov elhimov marked this pull request as ready for review December 13, 2024 10:01
@elhimov elhimov force-pushed the gh-763-coredump-pack-support-options branch 3 times, most recently from 6c9745a to 0ade120 Compare December 13, 2024 13:49
@elhimov elhimov added the full-ci Enables full ci tests label Dec 13, 2024
@elhimov elhimov force-pushed the gh-763-coredump-pack-support-options branch from 0ade120 to 1b1918f Compare December 13, 2024 15:19
@elhimov elhimov added full-ci Enables full ci tests and removed full-ci Enables full ci tests labels Dec 13, 2024
@elhimov elhimov force-pushed the gh-763-coredump-pack-support-options branch from 1b1918f to 0f013c1 Compare December 13, 2024 16:03
@elhimov elhimov added full-ci Enables full ci tests and removed full-ci Enables full ci tests labels Dec 13, 2024
@elhimov elhimov force-pushed the gh-763-coredump-pack-support-options branch from 0f013c1 to c2be144 Compare December 13, 2024 17:37
@elhimov elhimov added the full-ci Enables full ci tests label Dec 13, 2024
@elhimov elhimov force-pushed the gh-763-coredump-pack-support-options branch 10 times, most recently from 59707a5 to 2a3c652 Compare December 16, 2024 19:47
oleg-jukovec
oleg-jukovec reviewed Dec 17, 2024
View reviewed changes
.github/workflows/full-ci.yml Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
cli/cmd/coredump.go Show resolved Hide resolved
patapenka-alexey
patapenka-alexey reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@patapenka-alexey patapenka-alexey left a comment

Choose a reason for hiding this comment

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

Thank you for the patch!

@elhimov elhimov force-pushed the gh-763-coredump-pack-support-options branch from 2a3c652 to 2228bd5 Compare December 17, 2024 08:59
@elhimov elhimov force-pushed the gh-763-coredump-pack-support-options branch 2 times, most recently from 80d06e6 to c56a250 Compare December 17, 2024 09:29
dmyger
dmyger reviewed Dec 17, 2024
View reviewed changes
test/utils.py Outdated
@@ -518,11 +518,12 @@ def is_valid_tarantool_installed(
return True


def get_tarantool_version():
def get_tarantool_version(env=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me suggest copying the logic from tt-ee to look up tarantool executable via the environment variable TARANTOOL_BIN.
This would simplify parameter passing, it would be enough to set the right variable, without additional manipulations with PATH and function arguments extension.
And, it will also bring the same logic to how the tarantool search works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted to use executable path instead of changing environment. tt-ee logic as-is is not suitable here.

test/integration/coredump/test_coredump.py Outdated
@@ -54,7 +65,7 @@ def apport_crash_to_coredump(crash):
resource.setrlimit(resource.RLIMIT_CORE, (resource.RLIM_INFINITY, rlim_core_hard))
# Crash tarantool.
cmd = ["tarantool", "-e", "require('ffi').cast('char *', 0)[0] = 42"]
rc, output = run_command_and_get_output(cmd, cwd=coredump_tmpdir)
rc, output = run_command_and_get_output(cmd, cwd=coredump_dir, env=env)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, you will lose other environment settings that may be needed in some situations.
The right way would be to make a copy of the current environment and add new settings to it.
But, I suggest you make a copy of tarantool search logic from the tt-ee project, then you don't need to make such changes here, and nor add a new fixture coredump_alt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted to use executable path instead of changing environment.
Considering that we need 2 different coredumps in the scope of single test session I see no way to avoid using another fixture.

test/integration/coredump/test_coredump.py Outdated
return apport_unpack_dir / 'CoreDump'
def coredump_systemd(core_source, outdir):
core = outdir / core_source.stem
rc, _ = run_command_and_get_output(['coredumpctl', 'dump', f'--output={core}'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you don't need output why you keep the same function here?

Suggested change
rc, _ = run_command_and_get_output(['coredumpctl', 'dump', f'--output={core}'])
rc = subprocess.run(['coredumpctl', 'dump', f'--output={core}']).returncode

This way the output of the command will not be captured and deleted, which will then appear in the log in case of problems with the test, and this in itself can help in finding problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reasonable. Fixed in both functions (the other function also doesn't use output).

@elhimov elhimov force-pushed the gh-763-coredump-pack-support-options branch from c56a250 to 514aacd Compare December 17, 2024 17:36
patapenka-alexey
patapenka-alexey approved these changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@patapenka-alexey patapenka-alexey left a comment

Choose a reason for hiding this comment

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

Thank you for the patch!

@oleg-jukovec
Copy link
Contributor

Please, add a TarantoolBot message into the commit.

@elhimov
coredump: support options in pack subcommand
98ab6a4 
-e,--executable path  Tarantool executable path
-p,--pid pid          PID of the dumped process
-t,--time time        Time of dump (seconds since the Epoch)

Closes #763

@TarantoolBot document
Title: support options in `tt coredump pack` subcommand
@elhimov elhimov force-pushed the gh-763-coredump-pack-support-options branch from 514aacd to 98ab6a4 Compare December 20, 2024 00:14
@elhimov elhimov requested a review from dmyger December 20, 2024 07:09
@elhimov
Copy link
Contributor Author

elhimov commented Dec 20, 2024

Please, add a TarantoolBot message into the commit.

Done

@elhimov elhimov requested a review from oleg-jukovec December 20, 2024 07:11
@oleg-jukovec oleg-jukovec removed their request for review December 20, 2024 10:05
@dmyger dmyger merged commit 337b24d into master Dec 23, 2024
24 checks passed
@dmyger dmyger deleted the gh-763-coredump-pack-support-options branch December 23, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patapenka-alexey patapenka-alexey patapenka-alexey approved these changes

@oleg-jukovec oleg-jukovec oleg-jukovec approved these changes

@dmyger dmyger dmyger approved these changes

Assignees
No one assigned
Labels
full-ci Enables full ci tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

coredump: allow to choose tarantool executable to pack
4 participants
@elhimov @oleg-jukovec @dmyger @patapenka-alexey