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

Add ability to update worker name #7746

Closed
wants to merge 2 commits into from
Closed

Conversation

wouterdb
Copy link
Contributor

Description

I added the ability to update the name of the process, to make debugging easier

  1. It now identifies the agent, not the code version or anything
  2. it shows the status (connecting/connected/disconnected)
  3. I could take it further and show every action (deploying resource x_y_z)
  4. this requires an additional dependency, which I don't like, but I see no alternative

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

  • Attached issue to pull request
  • Changelog entry
  • Type annotations are present
  • Code is clear and sufficiently documented
  • No (preventable) type errors (check using make mypy or make mypy-diff)
  • Sufficient test cases (reproduces the bug/tests the requested feature)
  • Correct, in line with design
  • End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
  • If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see test-fixes for more info)

@wouterdb wouterdb requested review from bartv, arnaudsjs and sanderr June 19, 2024 12:30
@wouterdb wouterdb self-assigned this Jun 19, 2024
@@ -165,6 +166,9 @@ async def test_executor_server_dirty_shutdown(mpmanager: MPManager, caplog):
assert ["aaaa"] == result
print("Child there")

process_name = psutil.Process(pid=child1.process.pid).name()
assert process_name == "inmanta: executor test - connected"
Copy link
Contributor

Choose a reason for hiding this comment

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

I wondering whether we should include the full identity of an executor here, because multiple agents with the same name can exist at the same time. For example when a dry-run is requested for an unreleased version. With this implementation it will be hard to keep them apart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree (see description), I just don't see how to make this identity meaningful...

Copy link
Contributor

Choose a reason for hiding this comment

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

We could make it the concatenation of all known model versions (with a limit), collapsed into ranges? e.g. v1-10,12-20.

Not pretty, but the best I can think of atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is volatile and not part of the current interfaces, but I kind of like it.

The ranges may compact quite badly, because not every version may reach the executor so it will not be aware of everything.

I don't think I'm going to add it now, but it is something to keep in mind

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved remark to #7692

Copy link
Contributor

@sanderr sanderr left a comment

Choose a reason for hiding this comment

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

Nice!

@wouterdb wouterdb removed the request for review from bartv June 19, 2024 13:31
@wouterdb wouterdb added the merge-tool-ready This ticket is ready to be merged in label Jun 19, 2024
@inmantaci
Copy link
Contributor

Processing this pull request

@inmantaci
Copy link
Contributor

Merged into branches master in d51ede4

inmantaci pushed a commit that referenced this pull request Jun 19, 2024
# Description

I added the ability to update the name of the process, to make debugging easier

1. It now identifies the agent, not the code version or anything
2. it shows the status (connecting/connected/disconnected)
2. I could take it further and show every action (deploying resource x_y_z)
3. this requires an additional dependency, which I don't like, but I see no alternative

# Self Check:

Strike through any lines that are not applicable (`~~line~~`) then check the box

- [ ] Attached issue to pull request
- [x] Changelog entry
- [x] Type annotations are present
- [x] Code is clear and sufficiently documented
- [x] No (preventable) type errors (check using make mypy or make mypy-diff)
- [x] Sufficient test cases (reproduces the bug/tests the requested feature)
- [x] Correct, in line with design
- [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
- [ ] If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)
@inmantaci inmantaci closed this Jun 19, 2024
@inmantaci inmantaci deleted the update_process_name branch June 19, 2024 13:32
@wouterdb wouterdb mentioned this pull request Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-tool-ready This ticket is ready to be merged in
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants