Keep looking for master process #617

spencerugbo · 2024-03-25T16:19:30Z

Proposed changes

Continuously searches for master process

Checklist

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

I have read the CONTRIBUTING document
I have run make install-tools and have attached any dependency changes to this pull request
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
If applicable, I have updated any relevant documentation (README.md)
If applicable, I have tested my cross-platform changes on Ubuntu 22, Redhat 8, SUSE 15 and FreeBSD 13

netlify · 2024-03-25T16:19:47Z

✅ Deploy Preview for agent-public-docs canceled.

Name	Link
🔨 Latest commit	`f72e5ca`
🔍 Latest deploy log	https://app.netlify.com/sites/agent-public-docs/deploys/6603fb9a69bc7a0008d1c4f1

codecov-commenter · 2024-03-26T09:19:37Z

Codecov Report

Attention: Patch coverage is 65.71429% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 66.02%. Comparing base (71ae09c) to head (f72e5ca).
Report is 32 commits behind head on main.

Files	Patch %	Lines
...hub.com/nginx/agent/v2/src/plugins/registration.go	65.71%	10 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #617      +/-   ##
==========================================
- Coverage   66.13%   66.02%   -0.12%     
==========================================
  Files         118      118              
  Lines       13461    13504      +43     
==========================================
+ Hits         8903     8916      +13     
- Misses       3958     3987      +29     
- Partials      600      601       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

olli-holmala · 2024-03-26T09:52:50Z

src/plugins/registration.go

@@ -29,6 +29,7 @@ const (
 	dataplaneSoftwareDetailsMaxWaitTime = time.Duration(5 * time.Second)
 	// Time between attempts to gather DataplaneSoftwareDetails
 	softwareDetailsOperationInterval = time.Duration(1 * time.Second)
+	nginxStartMaxWaitTime            = 0


Would this not essentially turn off backoff altogether, if the MaxElapsedTime is zero? If that is intentional, then I would maybe adding a comment explaining why it is turned off.

When the max wait time is set to 0 backoff will continue waiting indefinitely until the master process is found. I'll add in a comment now

olli-holmala · 2024-03-26T10:07:35Z

src/plugins/registration.go

@@ -93,13 +94,16 @@ func (r *OneTimeRegistration) Process(msg *core.Message) {
 			defer r.dataplaneSoftwareDetailsMutex.Unlock()
 			r.dataplaneSoftwareDetails[data.GetPluginName()] = data.GetDataplaneSoftwareDetails()
 		}
+	case msg.Exact(core.NginxDetailProcUpdate):
+		r.processes = msg.Data().([]*core.Process)


I would add a type check here, to ensure that the payload of the message is of the correct type (see the previous case).

If the payload were to contain something other than []*core.Process, then the code would panic, which is not great. Even though it should never have anything else than a []*core.Process based on what the code looks like today, you never know how the code base will change in the future. As a rule of thumb, it's always good practice to perform type checking to ensure safe code.

olli-holmala · 2024-03-26T10:22:07Z

src/plugins/registration.go

+				}
+			} else {
+				log.Tracef("NGINX non-master process: %d", proc.Pid)
+				return nil


This return causes the whole function to exit, meaning that the function will only ever find the master process if it is the first process in r.processes. I would remove this return.

olli-holmala · 2024-03-26T11:20:17Z

src/plugins/registration.go

+					// Reading nginx config during registration to populate nginx fields like access/error logs, etc.
+					_, err := r.binary.ReadConfig(nginxDetails.GetConfPath(), nginxDetails.NginxId, r.env.GetSystemUUID())
+					if err != nil {
+						log.Warnf("Unable to read config for NGINX instance %s, %v", nginxDetails.NginxId, err)


I know that this line isn't originally your code, but I'm wondering if we should actually be returning the error here instead of merely logging it 🤔

@oliveromahony, @dhurley, @Dean-Coakley, @aphralG thoughts?

I believe the reason we dont return an error here is because we dont want to stop registration just because we cant read the config. If the config is invalid it should not stop the agent from performing other tasks like reporting the status of NGINX is that makes sense.

olli-holmala · 2024-03-26T11:30:42Z

src/plugins/registration.go

+		context.Background(), backoffSetting, findMaster,
+	)
+	if err != nil {
+		log.Warn(err.Error())


This was previously:

log.Info("No master process found")

But my instinct is that this should be an error log, as I'd think we can't complete registration if we haven't found a master process.

Others could also weigh in on this, as I wouldn't consider myself an expert on registration.

Yes I believe previously we would continue with registration even if we didn't find master processes but now that we are waiting until we find one I think it should be an error log message.

log.Errorf("Unable to find NGINX master processes, %v", err)

dhurley · 2024-03-27T09:43:24Z

src/plugins/registration.go

+		Multiplier:      backoff.BACKOFF_MULTIPLIER,
+	}
+
+	findMaster := func() error {


Suggested change

findMaster := func() error {

findNginxMasterProcesses := func() error {

spencerugbo added 9 commits February 20, 2024 09:18

Added supervisor.d

7156fcd

Installed supervisor into the Docker Image

317da66

changes to registration.go to make it continuously search for NGINX

fe59ae9

changes to registration.go to make it continuously search for NGINX

1b8b468

Used backoff instead of sleep

21819a3

Used backoff instead of sleep

1936f20

Added unit test for registerAgent()

7f97955

Prep for PR

9f37147

Prep for PR

b89cf0e

spencerugbo requested review from oliveromahony, dhurley, Dean-Coakley, craigell, olli-holmala and aphralG March 25, 2024 16:19

spencerugbo self-assigned this Mar 25, 2024

github-actions bot added chore Pull requests for routine tasks dependencies labels Mar 25, 2024

spencerugbo added 2 commits March 25, 2024 17:11

Updated subscription test

794ff64

Updated subscription test

ac725e2

olli-holmala reviewed Mar 26, 2024

View reviewed changes

spencerugbo added 2 commits March 26, 2024 13:31

type check for core.NginxDetailProcUpdate

cff923c

type check for core.NginxDetailProcUpdate

a3506c5

dhurley reviewed Mar 27, 2024

View reviewed changes

spencerugbo added 2 commits March 27, 2024 10:55

Logging an error when master nginx process is not found

799bfd7

Logging an error when master nginx process is not found

f72e5ca

dhurley approved these changes Mar 27, 2024

View reviewed changes

olli-holmala approved these changes Mar 27, 2024

View reviewed changes

spencerugbo merged commit 43a80a0 into main Mar 27, 2024
29 checks passed

spencerugbo deleted the Fix-Agent-Launch-Systemd branch March 27, 2024 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep looking for master process #617

Keep looking for master process #617

spencerugbo commented Mar 25, 2024 •

edited

Loading

netlify bot commented Mar 25, 2024 •

edited

Loading

codecov-commenter commented Mar 26, 2024 •

edited

Loading

olli-holmala Mar 26, 2024

spencerugbo Mar 26, 2024

olli-holmala Mar 26, 2024 •

edited

Loading

olli-holmala Mar 26, 2024 •

edited

Loading

olli-holmala Mar 26, 2024

dhurley Mar 27, 2024

olli-holmala Mar 26, 2024 •

edited

Loading

dhurley Mar 27, 2024

dhurley Mar 27, 2024

	findMaster := func() error {
	findNginxMasterProcesses := func() error {

Keep looking for master process #617

Keep looking for master process #617

Conversation

spencerugbo commented Mar 25, 2024 • edited Loading

Proposed changes

Checklist

netlify bot commented Mar 25, 2024 • edited Loading

✅ Deploy Preview for agent-public-docs canceled.

codecov-commenter commented Mar 26, 2024 • edited Loading

Codecov Report

olli-holmala Mar 26, 2024

Choose a reason for hiding this comment

spencerugbo Mar 26, 2024

Choose a reason for hiding this comment

olli-holmala Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

olli-holmala Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

olli-holmala Mar 26, 2024

Choose a reason for hiding this comment

dhurley Mar 27, 2024

Choose a reason for hiding this comment

olli-holmala Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

dhurley Mar 27, 2024

Choose a reason for hiding this comment

dhurley Mar 27, 2024

Choose a reason for hiding this comment

spencerugbo commented Mar 25, 2024 •

edited

Loading

netlify bot commented Mar 25, 2024 •

edited

Loading

codecov-commenter commented Mar 26, 2024 •

edited

Loading

olli-holmala Mar 26, 2024 •

edited

Loading

olli-holmala Mar 26, 2024 •

edited

Loading

olli-holmala Mar 26, 2024 •

edited

Loading