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

check if missing hosts during the deployment #618

Closed
wants to merge 3 commits into from

Conversation

SteBaum
Copy link
Contributor

@SteBaum SteBaum commented Aug 1, 2024

Which issue(s) this PR fixes

Fixes # None

Additional comments

When a deployment is launched check if hosts have been removed from the inventory.ini. The logger displays a warning if a host has been removed.

There are three commits:

  • Added get_collections_hosts method for the Collections class.
  • Added check_collections_hosts method for the Collection class. It compares hosts from the collections with the hosts in the inventory.ini file and logger sends a warning if a host is missing in the inventory.ini file.
  • Integrated check_collections_hosts in the deployment iterator.

Agreements

@SteBaum SteBaum self-assigned this Aug 1, 2024
@SteBaum SteBaum force-pushed the stephan/fix/collections branch from 2675b23 to 8f07d2d Compare August 1, 2024 11:59
@SteBaum SteBaum marked this pull request as draft August 1, 2024 12:35
@SteBaum SteBaum force-pushed the stephan/fix/collections branch from 8f07d2d to 4461a2a Compare August 2, 2024 08:22
@SteBaum SteBaum force-pushed the stephan/fix/collections branch from 4461a2a to 4083892 Compare August 2, 2024 08:38
@SteBaum SteBaum requested a review from rpignolet August 2, 2024 08:55
@SteBaum SteBaum marked this pull request as ready for review August 2, 2024 08:55
@SteBaum
Copy link
Contributor Author

SteBaum commented Aug 2, 2024

The host checking procedure slows down the deployment. It can specifically be seen when a tdp deploy --dry or tdp -deploy --mock-deploy is performed.

Copy link
Contributor

@PaulFarault PaulFarault left a comment

Choose a reason for hiding this comment

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

I see several issues here.

  1. get_collections_hosts should be a property initialized directly within the class, rather than a getter that computes it each time it's called. Additionally, its implementation can be made much more efficient by creating a set directly, avoiding the need for three loops.
  2. check_collections_hosts is currently returning the first host removed from the collections compared to the inventory:
  • This behavior is unrelated to the operations running in the current deployment.
  • Since it always returns the same value, there’s no need to invoke it for every operation.

It could be useful to both store the hosts used in the collections and verify that the hosts for the current operation haven't been removed from inventory.ini. However, the current approach doesn't seem like the right solution for this.

@SteBaum SteBaum removed the request for review from rpignolet October 21, 2024 12:42
@SteBaum
Copy link
Contributor Author

SteBaum commented Jan 28, 2025

No more up to date.

@SteBaum SteBaum closed this Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants