API: Display a correct status when removing a downtime #8104

yhabteab · 2020-07-09T13:40:02Z

resolves #7408

lib/icinga/apiactions.cpp

Al2Klimov

git reset --soft HEAD~$(git rev-list --count $(git merge-base HEAD master)..HEAD^)
git commit --amend --no-edit
git push -f origin bugfix/remove-downtime-returns-wrong-status-7408

Al2Klimov

Write a before/after test protocol for both cases you handle.

yhabteab · 2020-07-09T14:39:16Z

Testprotocol:
The problem was that Icinga would output incorrect API response even though the downtime could not be removed. So, although a log message is displayed saying that deleting downtime is not possible, you will get an api response in the console Successfully removed downtime ........

First create scheduledDowntime object like this..

object ScheduledDowntime "some-downtime" {
  host_name = "example"
  service_name = "ping4"

  author = "root"
  comment = "Some comment Here"

  fixed = false
  duration = 30m

  ranges = {
    "thursday" = "14:29-15:00"
  }
}

Now restart Icinga2 daemon and try to remove downtime: curl -k -s -u root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/actions/remove-downtime' -d '{ "downtime": "example!ping4!94c0de19-1dcd-429c-9613-9a9ecd537d3e", "pretty": true }'

Log message:

[2020-07-09 16:31:59 +0200] information/ApiListener: New client connection from [127.0.0.1]:38918 (no client certificate)
[2020-07-09 16:31:59 +0200] information/HttpServerConnection: Request: POST /v1/actions/remove-downtime (from [127.0.0.1]:38918), user: root, agent: curl/7.58.0).
[2020-07-09 16:31:59 +0200] warning/Downtime: Cannot remove downtime 'example!ping4!94c0de19-1dcd-429c-9613-9a9ecd537d3e'. It is owned by scheduled downtime object 'example!ping4!some-downtime'
[2020-07-09 16:31:59 +0200] information/HttpServerConnection: HTTP client disconnected (from [127.0.0.1]:38918)

Api response:

"results": [
        {
            "code": 200.0,
            "status": "Successfully removed downtime 'example!ping4!94c0de19-1dcd-429c-9613-9a9ecd537d3e'."
        }
    ]

After:
Log message:

[2020-07-09 16:27:37 +0200] information/HttpServerConnection: Request: POST /v1/actions/remove-downtime (from [127.0.0.1]:38908), user: root, agent: curl/7.58.0).
[2020-07-09 16:27:37 +0200] warning/Downtime: Cannot remove downtime 'example!ping4!94c0de19-1dcd-429c-9613-9a9ecd537d3e'. It is owned by scheduled downtime object 'example!ping4!some-downtime'
[2020-07-09 16:27:37 +0200] information/HttpServerConnection: HTTP client disconnected (from [127.0.0.1]:38908)

Api :

"results": [
        {
            "code": 400.0,
            "status": "Cannot remove downtime 'example!ping4!94c0de19-1dcd-429c-9613-9a9ecd537d3e'. It is owned by scheduled downtime object 'example!ping4!some-downtime'"
        }
    ]

Or if you want to delete host and services that belong to the scheduledDowntime:

Before:

[2020-07-09 16:33:12 +0200] information/ApiListener: New client connection from [127.0.0.1]:38922 (no client certificate)
[2020-07-09 16:33:12 +0200] information/HttpServerConnection: Request: POST /v1/actions/remove-downtime (from [127.0.0.1]:38922), user: root, agent: curl/7.58.0).
[2020-07-09 16:33:12 +0200] warning/Downtime: Cannot remove downtime 'example!ping4!94c0de19-1dcd-429c-9613-9a9ecd537d3e'. It is owned by scheduled downtime object 'example!ping4!some-downtime'
[2020-07-09 16:33:12 +0200] information/HttpServerConnection: HTTP client disconnected (from [127.0.0.1]:38922)

"results": [
        {
            "code": 200.0,
            "status": "Successfully removed all downtimes for object 'example!ping4'."
        }
    ]

After:

[2020-07-09 16:38:17 +0200] information/ApiListener: New client connection from [127.0.0.1]:38946 (no client certificate)
[2020-07-09 16:38:17 +0200] information/HttpServerConnection: Request: POST /v1/actions/remove-downtime (from [127.0.0.1]:38946), user: root, agent: curl/7.58.0).
[2020-07-09 16:38:17 +0200] warning/Downtime: Cannot remove downtime 'example!ping4!94c0de19-1dcd-429c-9613-9a9ecd537d3e'. It is owned by scheduled downtime object 'example!ping4!some-downtime'
[2020-07-09 16:38:17 +0200] information/HttpServerConnection: HTTP client disconnected (from [127.0.0.1]:38946)

"results": [
        {
            "code": 400.0,
            "status": "Cannot remove downtime 'example!ping4!94c0de19-1dcd-429c-9613-9a9ecd537d3e'. It is owned by scheduled downtime object 'example!ping4!some-downtime'"
        }
    ]

julianbrost

Have you considered passing the error from Downtime::RemoveDowntime(...) instead of duplicating the check, for example by raising an exception? That function is used in a few places though and probably all of them would have to be updated though. But this could also show different places where the caller should know about this failure.

I think it could also be possible that downtime->IsExpired() changes between the check here and in Downtime::RemoveDowntime(...), so in theory you could get different results in the log and the API response, but nobody would really care I guess (log says it can't be removed right before it expires, API response says it was removed as it just expired).

julianbrost

There are also two more uses of Downtime::RemoveDowntime in the code base, have you checked these?

Checkable::RemoveAllDowntimes

icinga2/lib/icinga/checkable-downtime.cpp

Lines 12 to 17 in 4167624

    
           void Checkable::RemoveAllDowntimes() 
        
           { 
        
           	for (const Downtime::Ptr& downtime : GetDowntimes()) { 
        
           		Downtime::RemoveDowntime(downtime->GetName(), true, true); 
        
           	} 
        
           }

Downtime::DowntimesExpireTimerHandler

icinga2/lib/icinga/downtime.cpp

Lines 441 to 445 in 4167624

    
           for (const Downtime::Ptr& downtime : downtimes) { 
        
           	/* Only remove downtimes which are activated after daemon start. */ 
        
           	if (downtime->IsActive() && (downtime->IsExpired() || !downtime->HasValidConfigOwner())) 
        
           		RemoveDowntime(downtime->GetName(), false, true); 
        
           }

lib/icinga/externalcommandprocessor.cpp

lib/icinga/apiactions.cpp

yhabteab · 2020-11-25T12:48:58Z

There are also two more uses of Downtime::RemoveDowntime in the code base, have you checked these?

Checkable::RemoveAllDowntimes

icinga2/lib/icinga/checkable-downtime.cpp

Lines 12 to 17 in 4167624

void Checkable::RemoveAllDowntimes()

{

for (const Downtime::Ptr& downtime : GetDowntimes()) {

Downtime::RemoveDowntime(downtime->GetName(), true, true);

}

}

Downtime::DowntimesExpireTimerHandler

icinga2/lib/icinga/downtime.cpp

Lines 441 to 445 in 4167624

for (const Downtime::Ptr& downtime : downtimes) {

/* Only remove downtimes which are activated after daemon start. */

if (downtime->IsActive() && (downtime->IsExpired() || !downtime->HasValidConfigOwner()))

RemoveDowntime(downtime->GetName(), false, true);

}

Yeah, I've seen them before. But since we throw the exception as long as the downtime has not expired and is currently used by another object, it should not be a problem with the two callers, because the parameter expiered is set to true when the Downtime::RemoveDowntime() is called.

julianbrost

Commit history obviously still needs cleanup and once you say you're done, I'll spin this up in my test setup for my final review.

In general, can you give some explanation on why you added the individual members of invalid_downtime_removal_error? I'd be perfectly fine with it only having the constructor it actually uses (i.e. with parameter String). There's probably no reason for allowing to construct an exception of this type without a message. Also, is there any particular reason for listing the copy constructor and operator=, or just to make it explicit?

lib/base/exception.cpp

lib/icinga/apiactions.cpp

lib/base/exception.cpp

lib/icinga/externalcommandprocessor.cpp

yhabteab · 2020-11-30T11:26:14Z

Also, is there any particular reason for listing the copy constructor and operator=, or just to make it explicit?

No, there is no special reason for this, but since this exception type is thrown with BOOST_THROW_EXCEPTION(), I wanted the copy constructor to be declared explicitly. If you do not like it, I can remove it right away!

julianbrost

See comments. Sorry for requesting changes yet again, I missed this during previous reviews.

lib/icinga/downtime.cpp

lib/icinga/externalcommandprocessor.cpp

Al2Klimov · 2020-12-01T15:49:20Z

Why to discuss w/ team Web? Have a look at these (onSuccess). They don't even care about errors.

julianbrost · 2020-12-01T16:29:40Z

Sure, you can also answer questions regarding the current behavior by trying it out or reading the source.

Al2Klimov

Rebase against master
Reproduce this statement of @julianbrost:

Somehow it now returns an error code of 400 in the JSON wrapped in a HTTP response with code 500

yhabteab · 2020-12-03T10:24:26Z

2. Reproduce this statement of @julianbrost:

< HTTP/1.1 500 Internal Server Error
< Server: Icinga/v2.12.0-rc1-54-g198177cc5
< Content-Type: application/json
< Content-Length: 248
< 
{
    "results": [
        {
            "code": 400.0,
            "status": "Cannot remove downtime 'ha-master!ping4!8912c953-19b5-4cd6-93d0-65fc98c5bf80'. It is owned by scheduled downtime object 'ha-master!ping4!run-downtime'"
        }
    ]
* Connection #0 to host localhost left intact
}

Al2Klimov · 2020-12-03T10:26:12Z

Good! Now have a look at the code and tell where this behavior comes from.

yhabteab · 2020-12-03T10:34:39Z

As @julianbrost already mentioned above, everything that is not 200 is listed in the HTTP response as internal server error with 500 error code and comes from here.

icinga2/lib/remote/actionshandler.cpp Lines 94 to 103 in 653c6f4

int statusCode = 500;

for (const Dictionary::Ptr& res : results) {
if (res->Contains("code") && res->Get("code") == 200) {
statusCode = 200;
break;
}
}

response.result(statusCode);

Al2Klimov · 2020-12-03T10:35:52Z

Any ideas how we could do this better?

yhabteab · 2020-12-03T10:41:50Z

Isn't it obvious 🙄 ! You just have to add an else if condition in the range based for loop.

Al2Klimov · 2020-12-03T10:47:04Z

No. Make a separate PR for that code:

Collect all unique status codes
If there's only one: take it
If there's only one non-2xx: take it
Else: take 500

Al2Klimov · 2020-12-14T14:30:17Z

Blocked by

HTTP: Do not override status codes that are not 200 #8532

julianbrost · 2021-01-14T14:05:38Z

In fact this didn't change what Web 2 reports when trying to delete a downtime. But I guess in the end this error page is better than silently ignoring an error. I'll open an issue there.

yhabteab requested a review from Al2Klimov July 9, 2020 13:40

Al2Klimov requested changes Jul 9, 2020

View reviewed changes

lib/icinga/apiactions.cpp Outdated Show resolved Hide resolved

Al2Klimov requested changes Jul 9, 2020

View reviewed changes

yhabteab force-pushed the bugfix/remove-downtime-returns-wrong-status-7408 branch from f4a3f85 to 9ce579d Compare July 9, 2020 14:07

yhabteab requested a review from Al2Klimov July 9, 2020 14:07

Al2Klimov requested changes Jul 9, 2020

View reviewed changes

yhabteab requested a review from Al2Klimov July 9, 2020 14:39

Al2Klimov approved these changes Jul 9, 2020

View reviewed changes

Al2Klimov added this to the 2.13.0 milestone Jul 9, 2020

Al2Klimov added area/api REST API bug Something isn't working labels Jul 9, 2020

Al2Klimov requested a review from julianbrost November 3, 2020 16:19

julianbrost reviewed Nov 12, 2020

View reviewed changes

Al2Klimov assigned yhabteab Nov 12, 2020

yhabteab force-pushed the bugfix/remove-downtime-returns-wrong-status-7408 branch 2 times, most recently from 8643cbc to 4167624 Compare November 15, 2020 10:49

yhabteab requested review from julianbrost and Al2Klimov November 16, 2020 11:01

julianbrost requested changes Nov 25, 2020

View reviewed changes

yhabteab requested a review from julianbrost November 26, 2020 15:45

julianbrost requested changes Nov 30, 2020

View reviewed changes

lib/base/exception.cpp Outdated Show resolved Hide resolved

lib/icinga/apiactions.cpp Outdated Show resolved Hide resolved

lib/base/exception.cpp Show resolved Hide resolved

lib/icinga/externalcommandprocessor.cpp Outdated Show resolved Hide resolved

yhabteab force-pushed the bugfix/remove-downtime-returns-wrong-status-7408 branch 2 times, most recently from 7e62648 to e630dd1 Compare November 30, 2020 11:39

yhabteab requested a review from julianbrost November 30, 2020 11:40

julianbrost requested changes Nov 30, 2020

View reviewed changes

lib/icinga/downtime.cpp Outdated Show resolved Hide resolved

lib/icinga/externalcommandprocessor.cpp Outdated Show resolved Hide resolved

Al2Klimov removed the needs feedback We'll only proceed once we hear from you again label Dec 1, 2020

yhabteab force-pushed the bugfix/remove-downtime-returns-wrong-status-7408 branch from ff11e34 to 3cae544 Compare December 1, 2020 16:25

yhabteab force-pushed the bugfix/remove-downtime-returns-wrong-status-7408 branch from 3cae544 to 3ea1289 Compare December 1, 2020 16:30

yhabteab requested a review from Al2Klimov December 2, 2020 17:04

Al2Klimov requested changes Dec 2, 2020

View reviewed changes

yhabteab mentioned this pull request Dec 3, 2020

HTTP: Do not override status codes that are not 200 #8532

Merged

yhabteab force-pushed the bugfix/remove-downtime-returns-wrong-status-7408 branch from 198177c to 572af6a Compare December 3, 2020 15:12

API: Display a correct status code when removing a scheduled downtime

dd02e3b

yhabteab force-pushed the bugfix/remove-downtime-returns-wrong-status-7408 branch from 572af6a to dd02e3b Compare December 7, 2020 12:20

yhabteab requested a review from Al2Klimov December 7, 2020 12:20

Al2Klimov removed their request for review December 14, 2020 14:30

Al2Klimov marked this pull request as draft December 14, 2020 14:30

Al2Klimov requested a review from julianbrost January 14, 2021 09:32

Al2Klimov unassigned yhabteab Jan 14, 2021

Al2Klimov marked this pull request as ready for review January 14, 2021 09:32

julianbrost approved these changes Jan 14, 2021

View reviewed changes

julianbrost mentioned this pull request Jan 14, 2021

Handle errors when removing downtimes Icinga/icingaweb2#4303

Closed

Al2Klimov merged commit 70b438a into master Jan 14, 2021

icinga-probot bot deleted the bugfix/remove-downtime-returns-wrong-status-7408 branch January 14, 2021 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Display a correct status when removing a downtime #8104

API: Display a correct status when removing a downtime #8104

yhabteab commented Jul 9, 2020

Al2Klimov left a comment •

edited by yhabteab

Loading

Al2Klimov left a comment

yhabteab commented Jul 9, 2020

julianbrost left a comment

julianbrost left a comment

yhabteab commented Nov 25, 2020

julianbrost left a comment

yhabteab commented Nov 30, 2020

julianbrost left a comment

Al2Klimov commented Dec 1, 2020

julianbrost commented Dec 1, 2020

Al2Klimov left a comment

yhabteab commented Dec 3, 2020 •

edited

Loading

Al2Klimov commented Dec 3, 2020

yhabteab commented Dec 3, 2020

Al2Klimov commented Dec 3, 2020

yhabteab commented Dec 3, 2020

Al2Klimov commented Dec 3, 2020

Al2Klimov commented Dec 14, 2020 •

edited

Loading

julianbrost commented Jan 14, 2021

	void Checkable::RemoveAllDowntimes()
	{
	for (const Downtime::Ptr& downtime : GetDowntimes()) {
	Downtime::RemoveDowntime(downtime->GetName(), true, true);
	}
	}

	for (const Downtime::Ptr& downtime : downtimes) {
	/* Only remove downtimes which are activated after daemon start. */
	if (downtime->IsActive() && (downtime->IsExpired() \|\| !downtime->HasValidConfigOwner()))
	RemoveDowntime(downtime->GetName(), false, true);
	}

API: Display a correct status when removing a downtime #8104

API: Display a correct status when removing a downtime #8104

Conversation

yhabteab commented Jul 9, 2020

Al2Klimov left a comment • edited by yhabteab Loading

Choose a reason for hiding this comment

Al2Klimov left a comment

Choose a reason for hiding this comment

yhabteab commented Jul 9, 2020

julianbrost left a comment

Choose a reason for hiding this comment

julianbrost left a comment

Choose a reason for hiding this comment

yhabteab commented Nov 25, 2020

julianbrost left a comment

Choose a reason for hiding this comment

yhabteab commented Nov 30, 2020

julianbrost left a comment

Choose a reason for hiding this comment

Al2Klimov commented Dec 1, 2020

julianbrost commented Dec 1, 2020

Al2Klimov left a comment

Choose a reason for hiding this comment

yhabteab commented Dec 3, 2020 • edited Loading

Al2Klimov commented Dec 3, 2020

yhabteab commented Dec 3, 2020

Al2Klimov commented Dec 3, 2020

yhabteab commented Dec 3, 2020

Al2Klimov commented Dec 3, 2020

Al2Klimov commented Dec 14, 2020 • edited Loading

Blocked by

julianbrost commented Jan 14, 2021

Al2Klimov left a comment •

edited by yhabteab

Loading

yhabteab commented Dec 3, 2020 •

edited

Loading

Al2Klimov commented Dec 14, 2020 •

edited

Loading