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.

Sign up for GitHub

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

Jump to bottom

Ensure k8s pod names/labels are RFC 1123 compliant #3639

Merged
merged 4 commits into from
Oct 21, 2024
Merged

Ensure k8s pod names/labels are RFC 1123 compliant #3639

merged 4 commits into from
Oct 21, 2024

Conversation

rjmello
Copy link
Member

@rjmello rjmello commented Oct 17, 2024 •
edited
Loading

Description

  • Modified Kubernetes pod names and labels to conform to RFC 1123 for DNS subdomain names and labels, ensuring compliance with Kubernetes naming conventions.
  • Modified KubernetesProvider.submit() to return an eight-character hex value as the job ID instead of the pod name.
  • Replaced the trailing timestamp in the pod name with the job ID to improve collision avoidance.
  • Replaced app pod label with parsl-job-id.
  • Updated container name to use job ID.

Type of change

  • Bug fix
  • New feature

@rjmello rjmello force-pushed the rjmello-kube-pod-names branch 4 times, most recently from 2fb44a7 to 1987f00 Compare October 17, 2024 22:28
@benclifford
Copy link
Collaborator

This has failed CI in the drain test a few times on Python 3.12, in what should be something unrelated. I'll dig into that a bit more and try to understand if that's a drain bug that is just started to show by unrelated chance, or if there's something going on with this PR (I suspect not).

@benclifford
Copy link
Collaborator

This has failed CI in the drain test a few times on Python 3.12, in what should be something unrelated. I'll dig into that a bit more and try to understand if that's a drain bug that is just started to show by unrelated chance, or if there's something going on with this PR (I suspect not).

I have recreated this a few times in PR #3640 independent of this PR #3639, so that test failure should not count against this PR.

@rjmello rjmello force-pushed the rjmello-kube-pod-names branch 3 times, most recently from 5f0cb77 to 4d099d6 Compare October 18, 2024 13:34
@benclifford benclifford mentioned this pull request Oct 19, 2024
Merged
benclifford added a commit that referenced this pull request Oct 19, 2024
@benclifford
Reduce load on drain integration test to reduce race-condition failur…
973ee48 
…es (#3640)

This PR reduces the load places on the interchange and on the whole test environment caused by repeatedly querying the interchange for connected managers. It does this by increasing the period between such requests, from the default, every 20ms, to every 100ms.

In the last few days, test_drain.py began failing often. I have seen it occasionally fail before. This was initially a problem in PR #3639 which is unrelated, but I recreated the problem in CI against master as of #3627.

I investigated and found this behaviour causing the failure:

* test_drain configures the drain period to be 1 second
* startup of a worker pool was taking more than 1 second
* the worker pool enters drain state, drains and terminates immediately on being fully started up.
* test_py fails its assumption that there is a worker pool to inspect after waiting for there to be worker pool to inspect. This is the race condition: the assertion at line 57 is true but line 59 returns an empty managers list.

```
57    try_assert(lambda: len(htex.connected_managers()) == 1, check_period_ms=CONNECTED_MANAGERS_POLL_MS)
58
59    managers = htex.connected_managers()
60    assert managers[0]['active'], "The manager should be active"
```

Looking at the CI logs for a failing case, I saw direct evidence that the worker pool takes more than 1 second to start up in `manager.log`:

```
2024-10-18 10:31:16.007 parsl:914 29414 MainThread [INFO]  Python version: 3.12.7 (main, Oct  1 2024, 15:17:50) [GCC 9.4.0]
[...]
2024-10-18 10:31:16.008 parsl:151 29414 MainThread [INFO]  Manager initializing
[this is where the worker start time for drain purposes is measured]
[...]
2024-10-18 10:31:16.011 parsl:183 29414 MainThread [INFO]  Manager connected to interchange
2024-10-18 10:31:17.058 parsl:243 29414 MainThread [INFO]  Will request drain at 1729247477.0087547
[...]
2024-10-18 10:31:17.073 parsl:336 29414 Task-Puller [DEBUG]  ready workers: 0, pending tasks: 0
```

There's more than a second delay between "... connected to interchange" and the subsequent message "Will request drain". There's not a huge amount of stuff happening between these lines but there are things like multiprocessing initialization which starts a new process.

It looks like this bit of code is slow even in the successful case: rerunning until success, I see this timing in CI:

```
2024-10-18 12:11:17.475 parsl:183 23062 MainThread [INFO]  Manager connected to interchange
2024-10-18 12:11:18.181 parsl:243 23062 MainThread [INFO]  Will request drain at 1729253478.4731379
```

which is still a large fraction of a second (but sufficiently less than a second for the test to pass).

I haven't investigated what is taking that time. I haven't investigated if I also see that on my laptop.

I hypothesised that a lot of these test failures come from the test environment being quite loaded. I'm especially suspicious of using `try_assert` with its default timings which are very tight (20ms) - the connected managers RPC here would be expected to run much less frequently, more like every 5 seconds in regular Parsl use.

So I lengthed the period of the try_asserts in this test, to try to reduce load caused there.

That makes the test pass repeatedly again.

Things not investigated/debugged:

* why this is taking >0.5 second even in the successful case - it's possible that this is a reasonable startup time and so the test might be lengthened by a few seconds
* how to do a test without being timing reliant - draining is, by its very nature, reliant on the passage of "real time". For example, you might mock (at the libc level if not at the Python level) system time.
* what other loads are present on the system - one of the points of slowly-ongoing PR #3397 shutdown tidyup is to reduce thread load left behind by earlier tests
benclifford
benclifford reviewed Oct 20, 2024
View reviewed changes
parsl/utils.py Outdated Show resolved Hide resolved
benclifford
benclifford reviewed Oct 20, 2024
View reviewed changes
parsl/utils.py Outdated Show resolved Hide resolved
@rjmello rjmello force-pushed the rjmello-kube-pod-names branch from 8500faa to 53cc37d Compare October 20, 2024 16:27
@rjmello rjmello requested a review from benclifford October 20, 2024 16:30
@rjmello rjmello force-pushed the rjmello-kube-pod-names branch 3 times, most recently from 741d3f6 to 7752073 Compare October 21, 2024 03:39
@rjmello
Add utils to sanitize strings for DNS compliance
1f09e5c 
These convert any string to a valid RFC 1123 DNS subdomain or label.
@rjmello
Ensure k8s pod names/labels are RFC 1123 compliant
83278c2 
- Modified Kubernetes pod names and labels to conform to RFC 1123 for
DNS subdomain names and labels, ensuring compliance with Kubernetes
naming conventions.

- Replaced the trailing timestamp in the job name with an eight-character
hex string (job ID) to improve collision avoidance.

- Replaced `app` pod label with `parsl-job-id`.

- Updated container name to use job ID.
@rjmello
Use hex value for k8s job ID instead of pod name
86ade32
@rjmello
Add tests for KubernetesProvider submit
0c4d541
@rjmello rjmello force-pushed the rjmello-kube-pod-names branch from 7752073 to 0c4d541 Compare October 21, 2024 04:38
@benclifford
Copy link
Collaborator

I merged this into #3482 (some of which was the base of code for this #3639 PR) so that #3482 has identical kube.py to this PR. The kubernetes test in #3482 passes, with pod names like: parsl.taskvineexecutor.block-0.8af7938f

@benclifford benclifford merged commit 410d2cb into master Oct 21, 2024
7 checks passed
@benclifford benclifford deleted the rjmello-kube-pod-names branch October 21, 2024 12:57
This was referenced Oct 21, 2024
Open
Closed
@rjmello rjmello mentioned this pull request Oct 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benclifford benclifford benclifford approved these changes

@yadudoc yadudoc Awaiting requested review from yadudoc

@khk-globus khk-globus Awaiting requested review from khk-globus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rjmello @benclifford