Async nginx config reload #70

dhurley · 2022-10-11T16:21:16Z

Proposed changes

Updated apply config logic to validate the nginx config in a separate go routine and return the result of the config reply in the Dataplane status.
If a nginx config apply takes a long time, the Dataplane status will indicate that the config apply is pending. Once the nginx config apply completes, the Dataplane status will indicate whether the config apply was either successful or failed.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING document
If applicable, I have added tests that prove my fix is effective or that my feature works
If applicable, I have checked that any relevant tests pass after adding my changes
I have updated any relevant documentation (README.md)

… go routine and return the result of the config reply in the Dataplane status.

oliveromahony · 2022-10-11T16:23:28Z

Maybe rename the PR to async reload

lcrilly · 2022-10-11T16:31:45Z

Updated apply config logic

This makes it sound like the existing synchronous-with-timeout logic is replaced with an async operation. But I've also heard that this is not a breaking change.

Please clarify if the previous behaviour is preserved?

oliveromahony · 2022-10-12T11:04:40Z

Updated apply config logic

This makes it sound like the existing synchronous-with-timeout logic is replaced with an async operation. But I've also heard that this is not a breaking change.

Please clarify if the previous behaviour is preserved?

yeah the old behaviour/workflow is upheld, it is only the failure scenarios are more "comprehensive" beyond the timeout period of the DPM on NMS

…alidation takes less than 15 seconds.

nickchen

didn't look at the test. Can you ensure each config apply action only has one response.

nickchen · 2022-10-18T14:41:14Z

src/plugins/nginx.go

-				succeeded:     succeeded,
-				correlationId: cmd.Meta.MessageId,
-				timestamp:     types.TimestampNow(),
+	go func() {


nit, use a named function with passed in params for maintainability.

nickchen · 2022-10-18T14:48:53Z

src/plugins/nginx.go

+	}
+}
+
+func (n *Nginx) completeConfigApply(response *NginxConfigValidationResponse) *proto.AgentActivityStatus {


kind of hard to follow because of async, but i believe we could end up with two responses due to the race between: 1) the non-async/inline response 2) the message handling.

So testing this build, i'm seeing failed to create response message: unknown action: UNKNOWN

ah yeah I can see where the completeConfigApply function could be called twice alright. I'll put a fix in for that, but you still should just get back one NginxConfigResponse. I'll double check that there in the morning and get back to you on it

nickchen · 2022-10-18T14:49:35Z

src/plugins/nginx.go

+	case core.NginxConfigValidationFailed:
+		switch response := message.Data().(type) {
+		case *NginxConfigValidationResponse:
+			n.rollbackConfigApply(response)


same potential race between rollback message handling, and inline.

…ng executed twice

src/plugins/nginx.go

nickchen · 2022-10-25T22:56:46Z

sdk/agent/config/config_helpers.go

+	FeatureActivityEvents   = FeaturesKey + KeyDelimiter + "activity-events"
+)
+
+func GetDefaultFeatures() []string {


the server needs to know the configured set of features that the agent supports. I think we have the following options:

agent sends to NMS in AgentMeta

agent sends to NMS in separate AgentConfigRequest

oliveromahony · 2022-10-26T13:28:12Z

src/plugins/registration.go

@@ -131,6 +131,12 @@ func (r *OneTimeRegistration) registerAgent() {
 					InstanceGroup: r.config.InstanceGroup,
 					Updated:       updated,
 					SystemUid:     r.env.GetSystemUUID(),
+					AgentDetails: &proto.AgentDetails{


@nickchen added the AgentDetails here. For backwards compatibility, if the AgentDetails is missing, assume default config minus the nginx-conifig-async? (basically what you have right now)

nickchen · 2022-10-27T18:29:20Z

src/plugins/nginx.go

+	select {
+	case result := <-n.configApplyStatusChannel:
+		return result
+	case <-time.After(validationTimeout):


Here are comments that describe the intended actions for the config apply and subsequent config response:
• As part of the AgentConnectRequest include the feature flag in the AgentMeta message as part of the AgentConnectRequest message.
• Always issue NginxConfigResponse without waiting for a timeout (eliminate the timeout here) following the config apply.
• The periodic DataplaneStatus should include the AgentActivityStatus message with the last known deployment status.
• Send an on demand DataplaneStatus when terminal states are achieved like success or fail. This should be sent once every 60 seconds by default.

so this timeout would be un-necessary. Adding note here for reference.

Hi Nick, so the reason for the timeout is that we wanted to preserve backgrounds compatibly as best we could. We added the timeout so that old versions of the controller continued to work with the newer version of the agent. We do this by waiting to see if the config apply can be done in a reasonable time and if it can return the response as before. I have added a task for a future release to remove this logic and go fully async (i.e. remove the timeout).

…leted

o.omahony and others added 6 commits September 9, 2022 09:17

wip

7f3136e

Merge branch 'main' into timed_reload

aef8cad

added timeout and channels

cd8c4fc

Updated apply config logic to validate the nginx config in a separate…

64dd34e

… go routine and return the result of the config reply in the Dataplane status.

Merge branch 'main' into timed_reload

e5ce0ee

Clean up

25b0345

dhurley requested review from oliveromahony, Dean-Coakley, craigell and aphralG October 11, 2022 16:21

dhurley self-assigned this Oct 11, 2022

github-actions bot added the dependencies label Oct 11, 2022

dhurley added enhancement New feature or request and removed dependencies labels Oct 11, 2022

dhurley changed the title ~~Timed reload~~ Async nginx config reload Oct 12, 2022

Fix nginx config apply unit tests

cce6a4a

github-actions bot added the dependencies label Oct 12, 2022

dhurley added 6 commits October 12, 2022 17:01

Updated config apply to return a response straight away if a config v…

40e1377

…alidation takes less than 15 seconds.

Update unit tests

0dded62

Update unit tests

23d74c6

Update unit tests

8677bab

Merge branch 'main' into timed_reload

e55fb93

Merge branch 'main' into timed_reload

97b7db4

dhurley requested a review from nickchen October 17, 2022 15:04

craigell approved these changes Oct 18, 2022

View reviewed changes

nickchen reviewed Oct 18, 2022

View reviewed changes

Updated nginx config validation logic to stop nginx reload from getti…

545bcc3

…ng executed twice

Dean-Coakley approved these changes Oct 20, 2022

View reviewed changes

src/plugins/nginx.go Outdated Show resolved Hide resolved

oliveromahony approved these changes Oct 20, 2022

View reviewed changes

clean up logging & tests

fa3b264

aphralG approved these changes Oct 20, 2022

View reviewed changes

added the default list of features to SDK

ecc5be2

nickchen reviewed Oct 25, 2022

View reviewed changes

added details to connect request

d66c78b

oliveromahony reviewed Oct 26, 2022

View reviewed changes

nickchen reviewed Oct 27, 2022

View reviewed changes

dhurley added 3 commits November 2, 2022 10:46

Merge branch 'main' into timed_reload

f2fd77c

Updated DataplaneStatus to remove agent activities when they are comp…

6c458db

…leted

Add comments to nginx plugin

584e597

dhurley merged commit 5261845 into main Nov 3, 2022

dhurley deleted the timed_reload branch November 3, 2022 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async nginx config reload #70

Async nginx config reload #70

dhurley commented Oct 11, 2022 •

edited

Loading

oliveromahony commented Oct 11, 2022

lcrilly commented Oct 11, 2022 •

edited

Loading

oliveromahony commented Oct 12, 2022

nickchen left a comment

nickchen Oct 18, 2022

nickchen Oct 18, 2022

dhurley Oct 18, 2022

nickchen Oct 18, 2022

nickchen Oct 25, 2022

oliveromahony Oct 26, 2022 •

edited

Loading

nickchen Oct 26, 2022

nickchen Oct 27, 2022 •

edited

Loading

dhurley Nov 2, 2022

Async nginx config reload #70

Async nginx config reload #70

Conversation

dhurley commented Oct 11, 2022 • edited Loading

Proposed changes

Checklist

oliveromahony commented Oct 11, 2022

lcrilly commented Oct 11, 2022 • edited Loading

oliveromahony commented Oct 12, 2022

nickchen left a comment

Choose a reason for hiding this comment

nickchen Oct 18, 2022

Choose a reason for hiding this comment

nickchen Oct 18, 2022

Choose a reason for hiding this comment

dhurley Oct 18, 2022

Choose a reason for hiding this comment

nickchen Oct 18, 2022

Choose a reason for hiding this comment

nickchen Oct 25, 2022

Choose a reason for hiding this comment

oliveromahony Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

nickchen Oct 26, 2022

Choose a reason for hiding this comment

nickchen Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

dhurley Nov 2, 2022

Choose a reason for hiding this comment

dhurley commented Oct 11, 2022 •

edited

Loading

lcrilly commented Oct 11, 2022 •

edited

Loading

oliveromahony Oct 26, 2022 •

edited

Loading

nickchen Oct 27, 2022 •

edited

Loading