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

Creating a new client through the API disables `keepalives`. #1203

Closed
nhooey opened this issue Apr 1, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@nhooey
Copy link

commented Apr 1, 2016

When I create a new client that doesn"t already exist in the API:
POST /clients:

{
    "name": "<name>",
    "address": "<ip_address>"
}

I get keepalives disabled in the client:
GET /clients/<name>:

{
    "address": "<ip_address>",
    "keepalives": false,
    "name": "<name>",
    "subscriptions": [],
    "timestamp": 1459539874,
    "version": "0.22.1"
}

Why are keepalives set to false by default?

Even when I enabled keepalives, specify the default handler, and specify thresholds for keepalive:
POST /clients:

{
    "name": "<name>",
    "address": "<ip_address>",
    "keepalives": true,
    "keepalive": {
        "handlers": ["default"],
        "thresholds": {
            "critical": 180,
            "warning": 60
        }
    }
}

I still get a client with keepalives disabled:
GET /clients/<name>:

{
    "address": "<ip_address>",
    "keepalive": {
        "handlers": ["default"],
        "thresholds": {
            "critical": 180,
            "warning": 60
        }
    },
    "keepalives": false,
    "name": "<name>",
    "subscriptions": [],
    "timestamp": 1459540464,
    "version": "0.22.1"
}
@nhooey

This comment has been minimized.

Copy link
Author

commented Apr 1, 2016

Thanks for zbintliff from IRC for pointing out that keepalives is always disabled on client creation.

In the case where you want to create a Sensu client for a node that is expected to check-in later after it is successfully provisioned, this is definitely a bug that prevents you from doing that.

@portertech

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

@nhooey this is by design, since whatever the future Sensu client sends in its first keepalive will replace anything you have added to the registry via the API.

@portertech

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

POST /clients is only intended for JIT client management.

@nhooey

This comment has been minimized.

Copy link
Author

commented Apr 6, 2016

@portertech Okay but let's assume the Sensu client never comes up, how am I going to know that it didn't come up within a reasonable amount of time?

My idea to solve this problem was to create a client through the API, set the keepalive timeout to 15 minutes, then provision the node with Ansible during which the Sensu client should be installed and check in and overwrite the keepalive timeout back to the default.

How else am I supposed to do this?

@portertech

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

We could just add a fetch() or ||= at https://github.com/sensu/sensu/blob/master/lib/sensu/api/process.rb#L359 to support "keepalives": true.

@portertech portertech self-assigned this Apr 6, 2016

@portertech portertech added the Small label Apr 6, 2016

@calebhailey

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

@nhooey this is a pretty clever idea. As previously mentioned, the current behavior was by design because Sensu is designed to support automatic/dynamic registration, so the idea of telling Sensu that a client is supposed to exist is an anti-pattern. Having said that, there's no harm in making this possible, so we're going to shoot for adding this in 0.24!

Thanks for your submission!

#monitoringlove

@calebhailey calebhailey added this to the 0.24 milestone Apr 6, 2016

@nhooey

This comment has been minimized.

Copy link
Author

commented Apr 6, 2016

That's great, @calebhailey. Thanks for adding this to the milestone.

@portertech

This comment has been minimized.

Copy link
Member

commented May 9, 2016

Closing this, as #1250 has been merged into the 0.24 release branch, scheduled to be release soon 👍

@portertech portertech closed this May 9, 2016

@portertech portertech removed the in progress label May 9, 2016

@calebhailey

This comment has been minimized.

Copy link
Member

commented May 9, 2016

I'm super interested to see how people use this. As previously mentioned, this is a bit of an anti-pattern in Sensu, but it does make alternative workflows possible without breaking anything, so it's a net positive. I just want to state for the record that I won't be surprised if this confuses new users and we spend a lot of time explaining why they shouldn't do this. 😄

@nhooey

This comment has been minimized.

Copy link
Author

commented May 9, 2016

@calebhailey How would you usually keep track of a server that's still booting up?

I don't think there's any reason to think that this is an anti-pattern.

@calebhailey

This comment has been minimized.

Copy link
Member

commented May 9, 2016

@nhooey see: #1203 (comment)

Ensuring that a provisioning event is successful is a provisioning problem (and/or something that should be monitored via monitoring the provisioning system itself). Telling Sensu a system is going to be provisioned is an anti-pattern because if the provisioning system is sufficiently dynamic, a provisioning event could fail for correct reasons, which could leave you with a client registration in Sensu for a client that will never exist (i.e. because the client name(s) or instance id(s) for the subsequent provisioning event(s) should be different), which you would then need to clean up manually. This is just one of many examples I could provide.

I'm probably just being pessimistic because open source software is hard. It's impossible to please everyone. We're obviously not opposed to opening the door for this type of workflow since we have implemented this new feature. I even think it will let you do some interesting things, as I have previously commented. I just wanted to leave a artifact of my own personal concerns in case this causes negative side effects within the community, down the road.

#monitoringlove

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.