-
Notifications
You must be signed in to change notification settings - Fork 988
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
Do not set default namespace for replication controller and deployment pod templates #275
Do not set default namespace for replication controller and deployment pod templates #275
Conversation
Note: StatefulSet are not affected because they do not reuse the |
Acceptance tests results with GKE 1.11.6-gke.2 :
Only some unrelated |
Those errors were caused by expired Application Default Credentials from November 2016 (fixed with All acceptance tests with GKE 1.11.6-gke.2 now pass:
|
@alexsomesan What do you think about that part? |
d1b0b0e
to
00a265a
Compare
00a265a
to
7ff8ee6
Compare
CHANGELOG.md
Outdated
@@ -1,4 +1,9 @@ | |||
## 1.6.0 (Unreleased) | |||
|
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.
Just stumbled upon this:
To keep the CHANGELOG up to date, we recommend updating the CHANGELOG after every set of commits that change the project. Never include CHANGELOG updates in a pull request. If you do, it will very likely cause a merge conflict with other pull requests. Instead, make the pull request without CHANGELOG updates, and add to the CHANGELOG only after merge.
@alexsomesan Should I drop that change from the PR?
7ff8ee6
to
d038dfd
Compare
d038dfd
to
b75b5cd
Compare
b75b5cd
to
ad7f1a1
Compare
ad7f1a1
to
bda0d2e
Compare
Work-aroundDefine
|
@alexsomesan as there's a not so shocking work-around, should I close this PR? |
The right thing to do would probably be to address my earlier comment and converge with the other workloads. |
@pdecat In principle I think that's the right approach. I do however have to investigate a little bit to rule out any corner-cases before I fully stand by any decision. I'm working on Pod affinity now, so I can have look at this right after I'm done with that. |
bda0d2e
to
c0de91c
Compare
Rebased on master. |
I should probably close this one unless you confirm me you'd like to look at this. |
I'm trying to get to it. |
c0de91c
to
26e9267
Compare
9039c74
to
d2918ae
Compare
Rebased on master and removed CHANGELOG.md update. |
d2918ae
to
679fe98
Compare
679fe98
to
473b979
Compare
Looks like this PR is still needed, @alexsomesan could you please have a look at it? |
Note: the work-around with an empty namespace described in #275 (comment) is no longer current, the
|
473b979
to
5e48e81
Compare
Rebased on master. |
Can someone approve this? Please @alexsomesan !! Thanks! |
I’ll take care of it in day or two.
On Fri 1. Nov 2019 at 16:21, Fernanda Martins ***@***.***> wrote:
Can someone approve this? Please @alexsomesan
<https://github.com/alexsomesan> !! Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#275>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIL5G7U63YEV5EG7YTQSJTQRRCP7ANCNFSM4GQCCX2A>
.
--
— Sent from my phone.
|
Please, can anybody take a look on this? |
5e48e81
to
43ba3a0
Compare
rebased on master |
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.
Change looks good to me. Running in CI after rebase. Will merge on green
It's almost there! OMG! Please merge! |
Tests still pass. OK to merge. |
Thanks @alexsomesan! |
Hi @pdecat (cc @alexsomesan), After updating to v1.10.0 of the provider, which includes this change, I'm now experiencing a recreation of Deployments on every plan, due to the provider wanting to set the previous
Is that expected? I know I could update my config to now just include 'default' as the namespace, but that also seems counter-intuitive, as these Deployments (and thus the Pods they start) are not located in the Would you suggest updating the config is the best course of action or should this value now also be computed when it exists already? (I'm not sure if that is the correct term, but I think that's how similar issues have been solved in other providers). |
Hi @tdmalone, I was also surprised, but if you check your Deployment raw yaml, you should see it actually has that |
BTW, to avoid complete recreation of the resources, I corrected my Deployments by editing them with Also, the fact that the terraform provider wants to recreate Deployments on such changes is tracked by #201 |
In fact, #201 should probably be closed and another issue be created when the changes affect the metadata of pod templates in ReplicationController and Deployments (they're the only two resources using |
To follow up on @pdecat and @tdmalone , at least on terraform v0.11.14, this change in default behaviour not only forces delete and recreation, but also fails during recreation. The destroy "finishes" before the deployment is actually gone, so an error of "Failed to create deployment: object is being deleted: deployments.apps "deployment" already exists" is easily reproducible during the upgrade to 1.10.0. However, rolling back to 1.9.0 after 1.10.0 changes are applied and fixed does NOT have the same issue. |
Hi @TylerWanner, first of all, as the author of this PR, sorry for any inconvenience this may have caused to your workloads. You should have been able to actually set the I believe the second issue about deletion timing out and re-creation happening too early is unrelated to this specific change. Do you still have the logs of the plan and apply runs? |
Replication controller and deployment pod templates currently have their
namespace
field defaulting todefault
.This causes issues when importing replication controllers and deployments with the
namespace
field not set on the pod spec. On the nextterraform plan
, a recreation of the controllers is triggered.This PR changes that.
Terraform Version
Affected Resource(s)
kubernetes_deployment
kubernetes_replication_controller
Plan output