-
Notifications
You must be signed in to change notification settings - Fork 31
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
Fixes #33176 - Improve tracer upload #119
Conversation
Can you share your thoughts about this PR @jturel ? Another question: when is the tracer state resetted? Like, tracer upload happened because system need to be rebooted. You do a reboot. After the host is back, the state "tracer state" on katello need to be set again. When and where is this done? |
Absolutely! Can you summarize that nature of the improvement so that I can review it with your goals in mind?
https://github.com/Katello/katello/blob/master/app/views/foreman/job_templates/restart_services.erb When the services are restarted via REX, katello-tracer-upload is called to let the server know |
... and finally there is a cron job on EL which calls katello-tracer-upload after reboot. https://github.com/Katello/katello-host-tools/blob/master/extra/katello-agent-send.cron I renamed the cron job. We should have this cron job for sles + debian/ubuntu. |
After this PR is done, we should create a new client release and make sure, that the build instructions for debian/ubuntu works and contains the cron job to do katello-tracer-upload. see theforeman/foreman-packaging#6958 |
@sbernhard will you create a redmine issue for this? That will allow me to track it with my team so someone can pick it up for review and test |
Done: 33176 |
Ping :-) |
It's on our sprint board for review! Likely will be reviewed later this week, but we do have a lot going on right now. We'll make sure this gets in to the upcoming Katello 4.2 release. This PR may be reviewed by team mates of mine who wish to learn about tracer (ie their first exposure to it) so any details you can provide to guide their review ('Here's what changed & why') would be very helpful as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a look at the code and the PR description so I see the intent clearly - thank you! Overall I like the direction. Left a few requests / ideas
This change highlights the need to get the required suse/zypper/apt/etc support into https://github.com/FrostyX/tracer itself so we can have a more consistent behavior across platforms and much less 'if this then that' checking |
Hey @sbernhard how is it going with the test cases? |
b17a97c
to
c614e24
Compare
@@ -31,6 +31,7 @@ | |||
modules.append('test_katello.test_plugin') | |||
modules.append('test_katello.test_agent.test_pulp.test_handler') | |||
modules.append('test_katello.test_agent.test_pulp.test_libyum') | |||
modules.append('test_katello.test_tracer') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should also be added under the zypper_enabled
block a few lines up. Other than that LGTM!
@sbernhard now the test failures seem related. please have a look remember that you can run the tests locally to save time: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. Thanks @sbernhard! Do you plan on updating packaging for some of the changes you made such as the cron job rename?
Creating a release + update foreman-packaging would be the next logical step @jturel |
3.5.7 release created - https://github.com/Katello/katello-host-tools/releases/tag/3.5.7 Do you mind taking a look @ the packaging @sbernhard ? |
Goal: