fix: reject reconnecting agents with different resource pool configuration #9815

ShreyaLnuHpe · 2024-08-13T16:38:07Z

Ticket

Description

Resource pool change does not apply if agent restarts too quickly. Resource Pool change on reconnect, discard the corresponding agent state and tell the agent to shutdown (and restart) as a way to not overly complicate the restore process and keep sanity.

Error thrown by master:

[ERRO] change in agent resource pool was detected    component=agent error="resource pool has changed: pool1 -> pool2" id=agent1 resource-pool=pool1
[ERRO] agent crashed                                 address=127.0.0.1 component=agent error="resource pool has changed: pool1 -> pool2" id=agent1 resource-pool=pool1 started=true

Error thrown by agent before it dies:
[FATA] agent is past reconnect period, it must restart

Previously, the steps were as follows:

The master starts and creates resource pools.
The agent starts and connects to the master.
The master assigns the agent to the resource pool specified in the configuration.
The configuration is updated.
The agent is stopped, and during this time, the master attempts to reconnect/restore the agent for a few seconds.
If the agent restarts while the master is still trying to reconnect, the master does not reload the configuration and continues using the old resource pool.
If the agent does not restart within those few seconds, the master drains/removes the agent. If the agent starts afterward, the master reads the updated configuration and attempts to connect the agent to the new resource pool.

After my code changes, the steps are now:

The master starts and creates resource pools.
The agent starts and connects to the master.
The master assigns the agent to the resource pool specified in the configuration.
The configuration is updated.
The agent is stopped, and during this time, the master attempts to reconnect/restore the agent for a few seconds.
If the agent tries to restart while the master is still attempting to reconnect, the master will detect the change in the resource pool, throw an error, and remove the agent, prompting a restart of the agent.
If the agent does not restart within those few seconds, the master drains/removes the agent. If the agent starts afterward, the master reads the updated configuration and attempts to connect the agent to the new resource pool.

Test Plan

Unit test added
Automated E2E test

To test manually:

Update resource pool config and have agent reconnect back to master
Agent state will be wiped and it will be told it needs to restart
After another reconnection agent and master should work ok

Commands to make/run/test manually:
Build only master and agent to get their binaries:
make -C master build
make -C agent build 
Run the docker to create a db: <you can get this command by ‘Copy the docker run’ from already running docker container> 
% docker run --hostname=d07083bced2f --mac-address=02:42:ac:11:00:02 --env=POSTGRES_DB=determined --env=POSTGRES_PASSWORD=postgres --env=PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/lib/postgresql/10/bin --env=GOSU_VERSION=1.12 --env=LANG=en_US.utf8 --env=PG_MAJOR=10 --env=PG_VERSION=10.14-1.pgdg90+1 --env=PGDATA=/var/lib/postgresql/data --volume=/Users/shreya/.postgres:/var/lib/postgresql/data --volume=/var/lib/postgresql/data -p 5432:5432 --restart=no --runtime=runc -d postgres:10.14 
Run the master.yaml: 
% master/build/determined-master --config-file determined/.circleci/devcluster/master.yaml
Run the agent.yaml:
% agent/build/determined-agent --config-file determined/.circleci/devcluster/agent.yaml
Run E2E test:
% pytest e2e_tests/tests/cluster/test_master_restart.py -k "test_agent_resource_pool_change" --log-cli-level=info --user-password=<INITIAL_USER_PASSWORD>

Checklist

Changes have been manually QA'd
New features have been approved by the corresponding PM
User-facing API changes have the "User-facing API Change" label
Release notes have been added as a separate file under docs/release-notes/
See Release Note for details.
Licenses have been included for new code which was copied and/or modified from any external code

…ation

netlify · 2024-08-13T16:38:24Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`2cbf48f`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66cf8429294b5e00081ab3a3

codecov · 2024-08-13T16:38:29Z

Codecov Report

Attention: Patch coverage is 33.33333% with 10 lines in your changes missing coverage. Please review.

Project coverage is 54.75%. Comparing base (ee269c8) to head (2cbf48f).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
master/internal/rm/agentrm/agent.go	0.00%	10 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9815   +/-   ##
=======================================
  Coverage   54.75%   54.75%           
=======================================
  Files        1261     1261           
  Lines      156333   156348   +15     
  Branches     3600     3598    -2     
=======================================
+ Hits        85604    85615   +11     
- Misses      70598    70602    +4     
  Partials      131      131

Flag	Coverage Δ
backend	`45.22% <33.33%> (+<0.01%)`	⬆️
harness	`72.62% <ø> (ø)`
web	`54.53% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
agent/internal/agent.go	`0.00% <ø> (ø)`
agent/internal/options/options.go	`30.35% <100.00%> (+1.26%)`	⬆️
master/internal/rm/agentrm/agent_state.go	`84.02% <100.00%> (+0.17%)`	⬆️
master/pkg/aproto/master_message.go	`0.00% <ø> (ø)`
master/internal/rm/agentrm/agent.go	`25.72% <0.00%> (+0.29%)`	⬆️

... and 3 files with indirect coverage changes

ioga · 2024-08-14T22:18:13Z

master/internal/rm/agentrm/agent_state.go

+	// If the agent's resource pool is empty in the configuration and the master has it set to default,
+	// the agent should not be restarted. However, if the agent's resource pool differs from the master's record,
+	// the agent should be restarted.
+	if !(agentStarted.ResourcePoolName == "" && a.resourcePoolName == defaultResourcePoolName) &&


maybe it makes sense to replace/rewrite "" -> defaultResourcePoolName earlier when agent tries to connect? there may be a lot of places which look at the ResourcePoolName field, having to do the same check in all of them may be redundant.

Updated it in DefaultOptions to eliminate the need for checking it wherever an Agent is created. Let me know if you have any suggestions for improvement.

dannysauer · 2024-08-23T17:03:12Z

.circleci/devcluster/double-reattach.devcluster.yaml

@@ -92,4 +92,4 @@ stages:
        agent_reconnect_attempts: 24
        agent_reconnect_backoff: 5
        container_auto_remove_disabled: true
-        artificial_slots: 4
+        artificial_slots: 4


It'd be good to configure your editor so it does not remove newlines at the end of files. We can afford the extra byte of data. :)

Noted! I will configure my editor better. But surprisingly this .yaml file didn't have a trailing new line in the end of it, nor did I add/remove any. Will see what is wrong and correct it. Thanks!

If you use VS Code, there's this setting you can toggle:

For more context, this is a POSIX standard thing: all lines should be terminated with a newline. Generally, this means that files' last line should have a \n at the end as well. Git and Github show us when the file does not comply with the standard.

Awesome, thanks!

dannysauer

I'm good with the parts infra owns, though "see note re: trailing newlines in files" ;)

ShreyaLnuHpe · 2024-08-25T04:25:40Z

.circleci/real_config.yml

@@ -4512,6 +4512,20 @@ workflows:
          extra-pytest-flags: "--no-compare-stats"
          collect-det-job-logs: false

+      - test-e2e:
+          name: test-e2e-managed-devcluster-resource-pools


Option 1 (DONE): Create a separate E2E test for the .circleci/devcluster/multi-resource-pools.devcluster.yaml config file to allow for broader test coverage in the future.
Option 2: Add a parallel run within test-e2e-managed-devcluster for the new config file.
https://circleci.com/docs/parallelism-faster-jobs/

In both options, the overall execution time of the CircleCI test-e2e runs remains unaffected since they run in parallel with other E2E tests.

I think option 1 is good for now, when we need broader coverage we can decide how to split things up!

salonig23 · 2024-08-26T20:18:00Z

.circleci/real_config.yml

@@ -4512,6 +4512,20 @@ workflows:
          extra-pytest-flags: "--no-compare-stats"
          collect-det-job-logs: false

+      - test-e2e:
+          name: test-e2e-managed-devcluster-resource-pools


I think option 1 is good for now, when we need broader coverage we can decide how to split things up!

master/internal/rm/agentrm/agent_state_test.go

salonig23

LGTM, great work!

maxrussell

LGTM! Very minor suggestion about the logged message that's totally optional.

master/internal/rm/agentrm/agent.go

master/internal/rm/agentrm/agent_state_test.go

tara-hpe · 2024-08-28T20:02:50Z

docs/get-started/architecture/introduction.rst

@@ -391,6 +391,11 @@ If you are using static resource pools and launching agents by hand, you will ne
 :ref:`agent configuration <agent-config-reference>` to specify which resource pool the agent should
 join.

+Note that to change an agent's assigned resource_pool after it has already joined one, you need to


Suggested change

Note that to change an agent's assigned resource_pool after it has already joined one, you need to

To change the resource pool an agent is assigned to after it has already joined one, you need to

tara-hpe · 2024-08-28T20:04:54Z

docs/get-started/architecture/introduction.rst

@@ -391,6 +391,11 @@ If you are using static resource pools and launching agents by hand, you will ne
 :ref:`agent configuration <agent-config-reference>` to specify which resource pool the agent should
 join.

+Note that to change an agent's assigned resource_pool after it has already joined one, you need to
+update the :ref:`agent configuration <agent-config-reference>`. Make sure to drain the agents


Suggested change

update the :ref:`agent configuration <agent-config-reference>`. Make sure to drain the agents

update the :ref:`agent's configuration <agent-config-reference>`. Before making this change, ensure the agents are properly drained. Once the configuration is updated, restart the agent to connect it to the new resource pool.

tara-hpe · 2024-08-28T20:05:15Z

docs/get-started/architecture/introduction.rst

@@ -391,6 +391,11 @@ If you are using static resource pools and launching agents by hand, you will ne
 :ref:`agent configuration <agent-config-reference>` to specify which resource pool the agent should
 join.

+Note that to change an agent's assigned resource_pool after it has already joined one, you need to
+update the :ref:`agent configuration <agent-config-reference>`. Make sure to drain the agents
+properly before modifying the resource_pool. After making the changes, restart the agent to connect


Suggested change

properly before modifying the resource_pool. After making the changes, restart the agent to connect

tara-hpe · 2024-08-28T20:05:26Z

docs/get-started/architecture/introduction.rst

+Note that to change an agent's assigned resource_pool after it has already joined one, you need to
+update the :ref:`agent configuration <agent-config-reference>`. Make sure to drain the agents
+properly before modifying the resource_pool. After making the changes, restart the agent to connect
+it to the new resource pool.


Suggested change

it to the new resource pool.

tara-hpe

suggested edits

tara-hpe

LGTM

fix: reject reconnecting agents with different resource pool configur…

d3caacd

…ation

cla-bot bot added the cla-signed label Aug 13, 2024

ShreyaLnuHpe added 3 commits August 13, 2024 22:47

add e2e test

b4a24f7

Merge branch 'main' into shreya/agentRestart

61384cf

add extra test cases

2a4d4f2

ShreyaLnuHpe marked this pull request as ready for review August 14, 2024 22:02

ShreyaLnuHpe requested review from a team as code owners August 14, 2024 22:02

ShreyaLnuHpe requested review from hamidzr and maxrussell August 14, 2024 22:02

ioga reviewed Aug 14, 2024

View reviewed changes

defaulted resource pool in options

8076a1b

ShreyaLnuHpe requested review from ioga and salonig23 August 14, 2024 23:54

ShreyaLnuHpe added 5 commits August 18, 2024 22:33

correct e2e test with new fixture

cbf6884

correct e2e test with new fixture

47bea5b

Sort in conftest.py

d93bb75

create restartable managed cluster

556220b

change port for connection

b3eb2d0

ShreyaLnuHpe marked this pull request as draft August 20, 2024 18:05

ShreyaLnuHpe added 3 commits August 21, 2024 21:32

Test new mark

69920f5

remove comments/unused lines

27825d2

minor correction

178a096

ShreyaLnuHpe marked this pull request as ready for review August 22, 2024 05:38

ShreyaLnuHpe requested review from a team as code owners August 22, 2024 05:38

ShreyaLnuHpe requested a review from dannysauer August 22, 2024 05:38

dannysauer reviewed Aug 23, 2024

View reviewed changes

dannysauer approved these changes Aug 23, 2024

View reviewed changes

ShreyaLnuHpe commented Aug 25, 2024

View reviewed changes

salonig23 reviewed Aug 26, 2024

View reviewed changes

salonig23 approved these changes Aug 27, 2024

View reviewed changes

maxrussell approved these changes Aug 28, 2024

View reviewed changes

master/internal/rm/agentrm/agent.go Outdated Show resolved Hide resolved

master/internal/rm/agentrm/agent_state_test.go Outdated Show resolved Hide resolved

master/internal/rm/agentrm/agent_state_test.go Show resolved Hide resolved

ShreyaLnuHpe added 4 commits August 27, 2024 18:31

changes based on comments

4877be6

add newline

5342281

Merge branch 'main' into shreya/agentRestart

6af1bd5

add note

f329db4

determined-ci added the documentation Improvements or additions to documentation label Aug 28, 2024

determined-ci requested a review from a team August 28, 2024 19:38

ShreyaLnuHpe requested a review from tara-hpe August 28, 2024 19:40

tara-hpe reviewed Aug 28, 2024

View reviewed changes

correct the note

2cbf48f

determined-ci requested a review from a team August 28, 2024 20:10

ShreyaLnuHpe requested a review from tara-hpe August 28, 2024 20:10

tara-hpe approved these changes Aug 28, 2024

View reviewed changes

ShreyaLnuHpe merged commit a605f00 into main Aug 29, 2024
84 of 98 checks passed

ShreyaLnuHpe deleted the shreya/agentRestart branch August 29, 2024 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: reject reconnecting agents with different resource pool configuration #9815

fix: reject reconnecting agents with different resource pool configuration #9815

ShreyaLnuHpe commented Aug 13, 2024 •

edited

Loading

netlify bot commented Aug 13, 2024 •

edited

Loading

codecov bot commented Aug 13, 2024 •

edited

Loading

ioga Aug 14, 2024

ShreyaLnuHpe Aug 14, 2024

dannysauer Aug 23, 2024

ShreyaLnuHpe Aug 23, 2024

maxrussell Aug 27, 2024 •

edited

Loading

ShreyaLnuHpe Aug 28, 2024

dannysauer left a comment

ShreyaLnuHpe Aug 25, 2024 •

edited

Loading

salonig23 Aug 26, 2024

salonig23 Aug 26, 2024

salonig23 left a comment

maxrussell left a comment

tara-hpe Aug 28, 2024

tara-hpe Aug 28, 2024

tara-hpe Aug 28, 2024

tara-hpe Aug 28, 2024

tara-hpe left a comment

tara-hpe left a comment

	Note that to change an agent's assigned resource_pool after it has already joined one, you need to
	To change the resource pool an agent is assigned to after it has already joined one, you need to

	update the :ref:`agent configuration <agent-config-reference>`. Make sure to drain the agents
	update the :ref:`agent's configuration <agent-config-reference>`. Before making this change, ensure the agents are properly drained. Once the configuration is updated, restart the agent to connect it to the new resource pool.

fix: reject reconnecting agents with different resource pool configuration #9815

fix: reject reconnecting agents with different resource pool configuration #9815

Conversation

ShreyaLnuHpe commented Aug 13, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Aug 13, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

codecov bot commented Aug 13, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxrussell Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannysauer left a comment

Choose a reason for hiding this comment

ShreyaLnuHpe Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonig23 left a comment

Choose a reason for hiding this comment

maxrussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tara-hpe left a comment

Choose a reason for hiding this comment

tara-hpe left a comment

Choose a reason for hiding this comment

ShreyaLnuHpe commented Aug 13, 2024 •

edited

Loading

netlify bot commented Aug 13, 2024 •

edited

Loading

codecov bot commented Aug 13, 2024 •

edited

Loading

maxrussell Aug 27, 2024 •

edited

Loading

ShreyaLnuHpe Aug 25, 2024 •

edited

Loading