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 #16542 - More thorough pulp services check #6314

Merged
merged 1 commit into from Sep 14, 2016

Conversation

Projects
None yet
5 participants
@johnpmitsch
Copy link
Member

commented Sep 13, 2016

pulp should be checked to be fully functional by checking

verify messaging_connection is True
verify database_connection is True
verify at least one scheduler@ entry is present
verify at least one resource_manager@ entry is present
verify at least one reserved_resource_worker-*@ is present
@daviddavis

View changes

app/models/katello/ping.rb Outdated

def all_pulp_workers_present?(json)
worker_ids = json["known_workers"].collect { |worker| worker["_id"] }
if worker_ids

This comment has been minimized.

Copy link
@daviddavis

daviddavis Sep 13, 2016

Member

I would probably use a guard clause here. I think it'll simplify the code.

This comment has been minimized.

Copy link
@daviddavis

daviddavis Sep 13, 2016

Member

Actually, collect always returns a non-nil value. So this check will always evaluate to true.

@daviddavis

This comment has been minimized.

Copy link
Member

commented Sep 13, 2016

Any way to add a test for this change?

@ehelms

This comment has been minimized.

Copy link
Member

commented Sep 13, 2016

Has a Pulp dev looked at this? I seem to recall a conversation once where someone indicated we werent checking or interpreting their status properly.

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:ping_pulp_16542 branch Sep 13, 2016

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2016

@daviddavis good call, we should be testing this
@ehelms I talked with @bmbouter about it on #pulp, he may want to take a look at this to ensure I am understanding him correctly


def all_pulp_workers_present?(json)
worker_ids = json["known_workers"].collect { |worker| worker["_id"] }
return false unless worker_ids.any?

This comment has been minimized.

Copy link
@johnpmitsch

johnpmitsch Sep 13, 2016

Author Member

@daviddavis updated to check if anything is present in the array returned

Fixes #16542 - More thorough pulp services check
pulp should be checked to be fully functional by checking

```
verify messaging_connection is True
verify database_connection is True
verify at least one scheduler@ entry is present
verify at least one resource_manager@ entry is present
verify at least one reserved_resource_worker-*@ is present
```

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:ping_pulp_16542 branch to f5d64ed Sep 13, 2016

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2016

@daviddavis added tests 💯

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2016

To test:

check each pulp service being shut off individually causes failing ping
SERVICES = [pulp_resource_manager.service, pulp_workers.service, pulp_celerybeat.service]
For service in SERVICES:

  1. systemctl stop service
  2. check Katello::Ping.ping fails from the rails console
  3. systemctl start service

With all services turned on, check Katello::Ping.ping works from the console
With all services turned off, check Katello::Ping.ping fails from the console

Then its worth testing basic pulp functionality like repo syncing and CV publishes

@daviddavis
Copy link
Member

left a comment

This worked for me. Code looks good. ACK.

@daviddavis

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

Do we want to have @bmbouter sign off on this before we merge?

@bmbouter

This comment has been minimized.

Copy link

commented Sep 14, 2016

I can't speak to if the ruby is correct or not, but I can see each required check has a line which reads like it is doing the right thing. This looks good to merge IMO.

Thank you all so much for doing this!

@johnpmitsch johnpmitsch merged commit 6aca541 into Katello:master Sep 14, 2016

1 check passed

default Job result: SUCCESS
Details

@johnpmitsch johnpmitsch deleted the johnpmitsch:ping_pulp_16542 branch Sep 14, 2016

johnpmitsch added a commit to johnpmitsch/katello that referenced this pull request Oct 4, 2016

Fixes #16542 - More thorough pulp services check (Katello#6314)
pulp should be checked to be fully functional by checking

```
verify messaging_connection is True
verify database_connection is True
verify at least one scheduler@ entry is present
verify at least one resource_manager@ entry is present
verify at least one reserved_resource_worker-*@ is present
```
(cherry picked from commit 6aca541)

jlsherrill added a commit that referenced this pull request Oct 10, 2016

Fixes #16542 - More thorough pulp services check (#6314)
pulp should be checked to be fully functional by checking

```
verify messaging_connection is True
verify database_connection is True
verify at least one scheduler@ entry is present
verify at least one resource_manager@ entry is present
verify at least one reserved_resource_worker-*@ is present
```
(cherry picked from commit 6aca541)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.