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

Updating Kubernetes check to handle multiple namespaces fix #2838 #3028

Merged
merged 1 commit into from
Nov 21, 2016

Conversation

hush-hush
Copy link
Member

What does this PR do?

This PR allows the agent to collect events from multiple Kubernetes namespaces by using a list of names and/or a regexp.

Two new configuration keys are added:
'namespaces': a list of names
'namespace_name_regexp': a regexp to match any namespace

Additional Notes

The old configuration value 'namespace' is still handled by the agent: if present it is added to the 'namespaces' list.

@hkaj hkaj added this to the 5.11.0 milestone Nov 17, 2016
@hkaj hkaj self-assigned this Nov 18, 2016
Copy link
Member

@hkaj hkaj left a comment

Choose a reason for hiding this comment

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

left a few comments, looks good overall :)

@@ -423,16 +423,43 @@ def _process_events(self, instance, pods_list):
node_ip, node_name = self.kubeutil.get_node_info()
self.log.debug('Processing events on {} [{}]'.format(node_name, node_ip))

k8s_namespace = instance.get('namespace', 'default')
events_endpoint = '{}/namespaces/{}/events'.format(self.kubeutil.kubernetes_api_url, k8s_namespace)
default_namespaces = ['default']
Copy link
Member

Choose a reason for hiding this comment

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

This should be a module constant

k8s_namespaces = default_namespaces

# handle old config value
if 'namespace' in instance and instance.get('namespace') not in (None, 'default'):
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add a log.warning with a deprecation notice here and get rid of it in a couple versions. This is a quick & easy change for the customer and shouldn't be too damaging if they forget.
This way we'll be able to get rid of the special case later.
Eg:

if 'namespace' in instance and instance.get('namespace') not in (None, 'default'):
    self.log.warning('''The namespace parameter is deprecated and will stop being supported starting '''
                     '''from 5.12. Please use namespaces and/or namespace_name_regexp instead.''')

k8s_namespace_regexp = instance.get('namespace_name_regexp', None)
if k8s_namespace_regexp:
try:
regexp = re.compile(k8s_namespace_regexp)
Copy link
Member

Choose a reason for hiding this comment

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

if we compile it, let's do it in __init__ to avoid redoing it at each check run.


# The regexp used to select namespaces for which events should be collected.
# The matched namespaces will be added to the "namespaces" list.
# If empty, regexp selection will be ignore.
Copy link
Member

Choose a reason for hiding this comment

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

s/ignore/ignored

The configuration key 'namespace' is renamed 'namespaces' so multiple
names can be specified (if present the old key will be added to the
'namespaces' list).

A new configuration key 'namespace_name_regexp' is added to allow
namespaces selection using regexp.

We now pull every event from Kubernetes and filter them the
namespace list.
Copy link
Member

@hkaj hkaj left a comment

Choose a reason for hiding this comment

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

LGTM

@hush-hush hush-hush merged commit 2ae4b17 into master Nov 21, 2016
@masci masci modified the milestones: 5.11.0, 5.12.0 Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants