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 dictionary update in try_push_job_info() #1673

Merged
merged 1 commit into from
Sep 24, 2021

Conversation

kamoltat
Copy link
Member

@kamoltat kamoltat commented Sep 13, 2021

the function try_push_job_info() is not
updating job_info dictionary properly since
we want to update job_info with extra_info,
however, in lines 498 and 499 we are assigning
job_info to a copy of extra_info and updating
job_info with job_config which is incorrect.
Instead, we should assign job_info with
a copy of job_config and update job_info with
extra_info

@kamoltat
Copy link
Member Author

@zmc Just want to confirm with you that this makes sense since you initially wrote this part a long time ago and there weren't really any issues until I tested out my docker-compose script.

@zmc
Copy link
Member

zmc commented Sep 15, 2021

@kamoltat Conceptually you're definitely right here; extra_info should override job_config. There's only one place where this is used, however, and that's potentially updating the failure_reason in case of a job timeout. I'm honestly unsure which behavior we want there.

Out of curiosity, what problem did this cause with the compose deployment?

@kamoltat
Copy link
Member Author

kamoltat commented Sep 15, 2021

@kamoltat Conceptually you're definitely right here; extra_info should override job_config. There's only one place where this is used, however, and that's potentially updating the failure_reason in case of a job timeout. I'm honestly unsure which behavior we want there.

Out of curiosity, what problem did this cause with the compose deployment?

@zmc it is not used in the current master of technology but with #1650, it is used in https://github.com/ceph/teuthology/pull/1650/files#diff-518b3a856fe844adcc19187dade87bd07738e36d20a528eef4834be57b0ba4d0R116

and without the fix, the dispatcher in #1650 faced an issue with trying to create and deleting archive files.

@zmc
Copy link
Member

zmc commented Sep 16, 2021

@kamoltat that makes sense, thanks! Would you mind rebasing and repushing just to get the unit tests to run?

@zmc
Copy link
Member

zmc commented Sep 16, 2021

@kamoltat tests failed because of a dependency issue on fudge (which is old and bad) so I rewrote the tests that used it. Once #1677 is merged, a rebase should clear things up.

the function `try_push_job_info()` is not
updating `job_info` dictionary properly since
we want to update `job_info` with `extra_info`,
however, in lines 498 and 499 we are assigning
`job_info` to a copy of `extra_info` and updating
`job_info` with `job_config` which is incorrect.
Instead, we should assign `job_info` with
a copy of `job_config` and update `job_info` with
`extra_info`
@zmc zmc merged commit f08b273 into ceph:master Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants