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

1. Introducing ability to reload microgateway for new config without … #42

Closed
wants to merge 1 commit into from

Conversation

vk4u
Copy link
Collaborator

@vk4u vk4u commented Sep 12, 2016

  1. Introducing ability to reload microgateway for new config without dropping messages.
  2. Used a node module called naught to achieve this.
  3. After this change, microgateway will always be starting in cluster mode and as a daemon.
  4. Owing to this there are 3 new commands added. Status, Reload, Stop.

…dropping messages.

2. Used a node module called naught to achieve this.
3. After this change, microgateway will always be starting in cluster mode  and as a daemon.
4. Owing to this there are 3 new commands added. Status, Reload, Stop.
@vk4u
Copy link
Collaborator Author

vk4u commented Sep 12, 2016

Kevin,
Im preparing the design document for this. Meanwhile please go through the changes and let me know if you need more info. npm test was failing already. So it continues to be the same.

.option('-e, --env <env>', 'the environment')
.option('-k, --key <key>', 'key for authenticating with Edge')
.option('-s, --secret <secret>', 'secret for authenticating with Edge')
.option('-p, --processes <processes>', 'number of processes to start, defaults to # of cores')
Copy link
Contributor

Choose a reason for hiding this comment

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

@vk4u Why do we need to know the number of processes on reload? I would think we want to reload config on all processes. What happens if one were to pass in a smaller number than what was used on start?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kevinswiber Its an optional attribute. If one needs to bring up a different number of workers (may be more or less than the original number of workers) for any reasons, they can use this option. If left alone, original number of workers will be brought up.

@kevinswiber
Copy link
Contributor

@vk4u These changes will probably get rolled into a 2.1.0 release along with the changes coming for apigee/microgateway-core#9. It would be spectacular to get tests working before the minor release, but we can address that post-merge.

@kevinswiber
Copy link
Contributor

@vk4u From the naught2 README:

Supports POSIX operating systems (does not support Windows)

This is problematic. Looking at the documentation, it appears that edgemicro currently supports Windows. Breaking Windows compatibility would be a pretty big bummer. Is there a way to offer this in a way that's cross-platform?

@vk4u
Copy link
Collaborator Author

vk4u commented Sep 13, 2016

@kevinswiber I did have a look at that. Naught was only possible to use from command line. I forked that and added the capability to call it from node. I think the restriction was given by the author to act on signals like SIGHUP and SIGTERM for non-daemon based bootup for server. For us we are always starting the process as a daemon and have dedicated scripts for reload and stop. Ideally, We should not have any problems. However, i have not tested on Windows, as i dont have one ;) Let me test it on windows and confirm this. Having said that im not sure if the current version of Microgateway works in Windows. If you can confirm that is works, ill test it for Windows and make necessary changes if anything is needed.

}
}
});
run.start(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the callback removed here?

'remove-old-ipc': 'true',
'node-args': ''
};
naught.start(naught_options, 'start.js', JSON.stringify(args));
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about the review open / review close stuff. Clearly I don't know what I'm doing with the new feature.

Error: Cannot find module '/Users/kevin/start.js'
    at Function.Module._resolveFilename (module.js:440:15)
    at Function.Module._load (module.js:388:25)
    at Module.runMain (module.js:575:10)
    at run (bootstrap_node.js:352:7)
    at startup (bootstrap_node.js:144:9)
    at bootstrap_node.js:467:3
module.js:442
    throw err;

@kevinswiber
Copy link
Contributor

@vk4u Should this be reloading on HUP signals sent to the master process? I can't seem to get that to work.

agentConfig(argsJson, function (e) {
if (e) {
console.error('edge micro failed to start', e);
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

@vk4u Can you add a process.exit(1) here?

@asnowfix
Copy link

BTW, isn't that PR re-adding another kind of edgemicro-agent... that was removed from 1.x to 2.x to enable single process to fit into a Docker container?

@kevinswiber
Copy link
Contributor

BTW, isn't that PR re-adding another kind of edgemicro-agent... that was removed from 1.x to 2.x to enable single process to fit into a Docker container?

Hey, @asnowfix. I wasn't involved with microgateway during the 1.x phase, but I believe this is different. This proposed change would allow the microgateway process to spawn multiple worker processes for handling incoming requests. A reload command should trigger the master process to start cycling workers, pending disconnect of existing traffic handling operations. This is how NGINX works today, IIRC. The number of worker processes is configurable.

This should run just fine in Docker and should be deployable in a way that the master process is PID 1 in the container. Ideally, we'd be able to run docker kill -s HUP <container-id> to trigger a reload from the Docker host. Again, this is exactly what NGINX does.

@vk4u can likely do a better job answering these questions, as he's the one responsible for these changes. :)

@vk4u vk4u closed this Oct 5, 2016
@mdobson mdobson deleted the reload_config branch February 17, 2017 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants