-
Notifications
You must be signed in to change notification settings - Fork 389
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
Properly handle recurring downtimes definitions #1092
Conversation
…e child recurrence downtime.
thanks @armcburney! This makes sense to me, except for this part:
From a customer perspective I don't think this is what we want - the provider should not be showing a diff when this is working as expected. As well, if customers use Some other options:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 couple of questions, but this looks good to me. Definitely an improvement!
gotestsum --hide-summary skipped --format testname --debug --packages $(TEST) -- $(TESTARGS) -timeout=30s | ||
|
||
# Run acceptance tests (this runs integration CRUD tests through the terraform test framework) | ||
testacc: get-test-deps fmtcheck lint | ||
testacc: get-test-deps fmtcheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw there was a slack conversation about this, @therve can you just confirm this is what the recommendation was?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we'll come back to fix it later.
docs/resources/downtime.md
Outdated
@@ -50,6 +50,7 @@ resource "datadog_downtime" "foo" { | |||
|
|||
### Optional | |||
|
|||
- **active_child_id** (Number) The id corresponding to the downtime object definition of the active child for the original parent recurring downtime. This field will only exist on recurring downtimes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we include this, I think it should be actually read only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumping on this, is this part auto generated or a copy/paste?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc is auto generated. But to avoid this, we should remove the line below since the attribute is read only:
Optional: true, |
…curring downtimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - we may need to update the PR title for the change log
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Overview
This PR implements a workaround for handling recurring downtimes with the Datadog Terraform provider. Recurring downtimes differ from regular 'one-off' downtimes in that subsequent recurrences are scheduled as new downtimes from the previous parent definition. Conceptually, we can think of this as a "linked list" of downtimes where each subsequent downtime is scheduled from the previous scheduled recurrence downtime definition. All fields are copied over from the previous parent with the exception of certain fields like
start
andend
, which are calculated off of therecurrence
attribute.The Datadog Terraform provider keeps reference to this original parent downtime ID. Since subsequent recurrence downtimes are scheduled as new downtimes (with a new ID), updates in the UI/API to the existing 'child' downtime corresponding to the original recurrence would not previously be recognized when comparing the downtime's state with what we store in Terraform. Moreover, after downtimes expire, we delete them from our database after a certain period of time. This behavior was recently changed so that we don't delete downtimes after they expire if they are the first downtime in the recurrence chain (i.e., the original parent downtime).
Instead, we now return that downtime with a new
active_child
field in ourGET /api/v1/downtime
API - which we use to compare state with Terraform. This way updates from the UI/API will be propagated back to Terraform. Additionally, when making updates through Terraform, we call thePUT /api/v1/downtime/{downtime_id}
endpoint with theactive_child
definition on theactive_child
's ID, so that changes from Terraform will be made to the current active recurrence downtime.Caveats
Caveat One
UPDATE: We don't check the
start
/end
boundaries for changes if the recurring downtime is a child to prevent superfluous diffs every time the downtime is rescheduled. The con to this approach is that if thestart
/end
values are changed in the UI on a child recurring downtime, the diff will not be picked up by Terraform. We plan to iterate on this solution to address this shortcoming, but feel the benefits of the child/parent references being handled by Terraform are worth merging in.Since new downtimes are scheduled each time a recurrence is rescheduled, fields likestart
andend
will perpetually differ after the first schedule when runningterraform plan
/terraform apply
.I recommend we update the documentation around recurring downtimes to document this problem and recommended an approach to mitigating this (i.e., usingignore_changes
to ignore thestart
andend
updates).Caveat Two
Recurring downtimes created before 2021-05-13 using Terraform will need to be deleted and recreated for the references to work with the newest version of the Terraform provider. All recurring downtimes created after 2021-05-13 will have the updated parent/child references, ensuring they’ll work as expected with the latest version of the provider. We apologize for this inconvenience.
Fixes