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

Fixes #33176 - Improve tracer upload #119

Merged
merged 2 commits into from Oct 12, 2021
Merged

Conversation

sbernhard
Copy link
Member

@sbernhard sbernhard commented Jul 28, 2021

Goal:

  • katello-tracer-upload script and the package manager plugin / hook works
  • share the code for both cases

@sbernhard sbernhard changed the title Improve tracer upload WIP: Improve tracer upload Jul 28, 2021
@sbernhard
Copy link
Member Author

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?

@jturel
Copy link
Member

jturel commented Jul 28, 2021

Can you share your thoughts about this PR @jturel ?

Absolutely! Can you summarize that nature of the improvement so that I can review it with your goals in mind?

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?

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

@sbernhard
Copy link
Member Author

sbernhard commented Jul 28, 2021

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.

@sbernhard
Copy link
Member Author

sbernhard commented Jul 29, 2021

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 sbernhard changed the title WIP: Improve tracer upload Improve tracer upload Jul 29, 2021
@jturel
Copy link
Member

jturel commented Jul 29, 2021

@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

@sbernhard sbernhard changed the title Improve tracer upload Fixes #33176 - Improve tracer upload Jul 29, 2021
@sbernhard
Copy link
Member Author

Done: 33176

@sbernhard
Copy link
Member Author

Ping :-)

@jturel
Copy link
Member

jturel commented Aug 3, 2021

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.

Copy link
Member

@jturel jturel left a 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

src/katello/tracer.py Show resolved Hide resolved
src/katello/tracer.py Show resolved Hide resolved
test/test_katello/test_tracer.py Outdated Show resolved Hide resolved
src/katello/tracer.py Show resolved Hide resolved
@jturel
Copy link
Member

jturel commented Aug 31, 2021

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

@jturel
Copy link
Member

jturel commented Sep 21, 2021

Hey @sbernhard how is it going with the test cases?

@sbernhard sbernhard force-pushed the improve branch 2 times, most recently from b17a97c to c614e24 Compare October 11, 2021 14:55
@@ -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')
Copy link
Member

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!

@jturel
Copy link
Member

jturel commented Oct 11, 2021

@sbernhard now the test failures seem related. please have a look

remember that you can run the tests locally to save time: DOCKERFILE=images/Dockerfile.suseLeap42 make docker-test

Copy link
Member

@jturel jturel left a 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?

@jturel jturel merged commit 2d7d6f3 into Katello:master Oct 12, 2021
@sbernhard
Copy link
Member Author

Creating a release + update foreman-packaging would be the next logical step @jturel

@jturel
Copy link
Member

jturel commented Oct 14, 2021

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants